Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, 16 Mar 2012 11:10:48 +0100 Richard Guenther richard.guent...@gmail.com wrote: On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely jwakely@gmail.com wrote: On 15 March 2012 15:40, Richard Guenther wrote: On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz ktiet...@googlemail.com wrote: Richard, ping. I think now could be a good time for applying the patch you have for this issue as we are in stage 1. It will still regress the two libstdc++ testcases (well, I guess so at least). Jonathan - you didn't answer my reply to your question? Would it be ok to apply this patch with leaving the regressions in-place, to be investigated by libstdc++ folks? Sorry, I've either forgotten or missed the reply - but if you think the problem is in libstdc++ then certainly go ahead and apply it, I'll investigate the libstdc++ problems (and ask for help if they defeat me!) Ok. I'll do so after re-testing the patch. FYI, here is the patch for the new libstdc++ fails. -benjamin 2012-03-19 Benjamin Kosnik b...@redhat.com * include/ext/pb_ds/detail/pat_trie_/ constructors_destructor_fn_imps.hpp: Increment after recursion. * include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp: Convert node_type markup from brief. diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp index 8370a2e..c5748ec 100644 --- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp +++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp @@ -1,6 +1,6 @@ // -*- C++ -*- -// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011 +// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 // Free Software Foundation, Inc. // // This file is part of the GNU ISO C++ Library. This library is free @@ -188,7 +188,11 @@ recursive_copy_node(node_const_pointer p_ncp) __try { while (child_it != p_icp-end()) - a_p_children[child_i++] = recursive_copy_node(*(child_it++)); + { + a_p_children[child_i] = recursive_copy_node(*(child_it)); + child_i++; + child_it++; + } p_ret = s_inode_allocator.allocate(1); } __catch(...) diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp index b7eb024..0a763b5 100644 --- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp +++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp @@ -1,6 +1,6 @@ // -*- C++ -*- -// Copyright (C) 2005, 2006, 2009, 2011 Free Software Foundation, Inc. +// Copyright (C) 2005, 2006, 2009, 2011, 2012 Free Software Foundation, Inc. // // This file is part of the GNU ISO C++ Library. This library is free // software; you can redistribute it and/or modify it under the terms @@ -50,7 +50,11 @@ namespace __gnu_pbds /// Base type for PATRICIA trees. struct pat_trie_base { - /// Three types of nodes. + /** + * @brief Three types of nodes. + * + * i_node is used by _Inode, leaf_node by _Leaf, and head_node by _Head. + */ enum node_type { i_node,
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely jwakely@gmail.com wrote: On 15 March 2012 15:40, Richard Guenther wrote: On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz ktiet...@googlemail.com wrote: Richard, ping. I think now could be a good time for applying the patch you have for this issue as we are in stage 1. It will still regress the two libstdc++ testcases (well, I guess so at least). Jonathan - you didn't answer my reply to your question? Would it be ok to apply this patch with leaving the regressions in-place, to be investigated by libstdc++ folks? Sorry, I've either forgotten or missed the reply - but if you think the problem is in libstdc++ then certainly go ahead and apply it, I'll investigate the libstdc++ problems (and ask for help if they defeat me!) Ok. I'll do so after re-testing the patch. Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On 15 March 2012 15:40, Richard Guenther wrote: On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz ktiet...@googlemail.com wrote: Richard, ping. I think now could be a good time for applying the patch you have for this issue as we are in stage 1. It will still regress the two libstdc++ testcases (well, I guess so at least). Jonathan - you didn't answer my reply to your question? Would it be ok to apply this patch with leaving the regressions in-place, to be investigated by libstdc++ folks? Sorry, I've either forgotten or missed the reply - but if you think the problem is in libstdc++ then certainly go ahead and apply it, I'll investigate the libstdc++ problems (and ask for help if they defeat me!)
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/2/9 Richard Guenther richard.guent...@gmail.com: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test Maybe invalid testcases. Who knows ... same fails happen with your patch. Richard. Richard. Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause part of this failure. This might lead here to this failure. I am not sure, if such constructs having fixed behavior for C++, but it looks to me like undefined behavior. A C-testcase for the issue would be: int *foo (int *p) { *p++ = *p; return p; } which produces with patch now: foo (int * p) { int * D.1363; int D.1364; int * D.1365; D.1363 = p; p = D.1363 + 4; D.1364 = *p; *D.1363 = D.1364; D.1365 = p; return D.1365; } but in old variant we were producing: foo (int * p) { int D.1363; int * D.1364; D.1363 = *p; *p = D.1363; p = p + 4; D.1364 = p; return D.1364; } So, maybe the real solution for this issue might be to swap for assignment gimplification the order for lhs/rhs gimplification instead. Well, that would certainly not be suitable for stage4 (though I have a working patch for that as well). The post-modify gimplification change looks better as it also fixes the volatile aggregate-return case which would not be fixed by re-ordering of the gimplification. libstdc++ folks - can you investigate the testsuite failure? The patch in question is at http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814 Note that the _Mutable_ForwardIteratorConcept isn't the problem I think, it's not executed code. Thanks, Richard. Regards, Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
Kai Tietz ktiet...@googlemail.com writes: Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause part of this failure. I don't think those __constraints functions are ever executed. They are only present to assert the presense of certain operations at compile time. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On 10 February 2012 10:35, Richard Guenther wrote: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test What does libstdc++.log show for those failures?
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Feb 10, 2012 at 2:12 PM, Jonathan Wakely jwakely@gmail.com wrote: On 10 February 2012 10:35, Richard Guenther wrote: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test What does libstdc++.log show for those failures? spawn [open ...]^M ?xml version = 1.0? test sd value = 1328880244 /sd n value = 5000 /n m value = 1 /m tp value = 0.2 /tp ip value = 0.6 /ip ep value = 0.2 /ep cp value = 0.001 /cp mp value = 0.25 /mp /test cntnr name = pat_trie_map desc type value = trie Tag value = pat_trie_tag /Tag Node_Update value = null_node_update /Node_Update /type /desc progress *FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test spawn [open ...]^M ?xml version = 1.0? test sd value = 1328880487 /sd n value = 5000 /n m value = 1 /m tp value = 0.2 /tp ip value = 0.6 /ip ep value = 0.2 /ep cp value = 0.001 /cp mp value = 0.25 /mp /test cntnr name = pat_trie_set desc type value = trie Tag value = pat_trie_tag /Tag Node_Update value = null_node_update /Node_Update /type /desc progress **FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/2/10 Richard Guenther richard.guent...@gmail.com: On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/2/9 Richard Guenther richard.guent...@gmail.com: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test Maybe invalid testcases. Who knows ... same fails happen with your patch. Richard. Richard. Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause part of this failure. This might lead here to this failure. I am not sure, if such constructs having fixed behavior for C++, but it looks to me like undefined behavior. A C-testcase for the issue would be: int *foo (int *p) { *p++ = *p; return p; } which produces with patch now: foo (int * p) { int * D.1363; int D.1364; int * D.1365; D.1363 = p; p = D.1363 + 4; D.1364 = *p; *D.1363 = D.1364; D.1365 = p; return D.1365; } but in old variant we were producing: foo (int * p) { int D.1363; int * D.1364; D.1363 = *p; *p = D.1363; p = p + 4; D.1364 = p; return D.1364; } So, maybe the real solution for this issue might be to swap for assignment gimplification the order for lhs/rhs gimplification instead. Well, that would certainly not be suitable for stage4 (though I have a working patch for that as well). The post-modify gimplification change looks better as it also fixes the volatile aggregate-return case which would not be fixed by re-ordering of the gimplification. libstdc++ folks - can you investigate the testsuite failure? The patch in question is at http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814 Note that the _Mutable_ForwardIteratorConcept isn't the problem I think, it's not executed code. Thanks, Richard. Hmm, we might need here lhs/rhs flip plus the post-modify. At least we would avoid by this code differences for common cases that lhs has post-inc/dec to current behavior? But you might be right that this patch is not suitable for stage 4. Regards, Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; As I explained this is the wrong place to fix the PR. The issue is not about self-modifying expressions but about evaluating call argument side-effects before side-effects of the lhs. Richard. ChangeLog 2012-02-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Move for postfix-inc/dec the modification in pre and return temporary with origin value. 2012-02-08 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814-1.c: New test. * gcc.c-torture/execute/pr48814-2.c: New test. * gcc.dg/tree-ssa/assign-1.c: New test. * gcc.dg/tree-ssa/assign-2.c: New test. I did boostrap for all languages (including Ada and Obj-C++) and regression tests on host x86_64-unknown-linux-gnu. Ok for apply? Regards, Kai Index: gcc/gcc/gimplify.c === --- gcc.orig/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi bool want_value) { enum tree_code code; - tree lhs, lvalue, rhs, t1; + tree lhs, lvalue, rhs, t1, t2; gimple_seq post = NULL, *orig_post_p = post_p; bool postfix; enum tree_code arith_code; @@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); - - if (postfix) - { - gimplify_assign (lvalue, t1, orig_post_p); - gimplify_seq_add_seq (orig_post_p, post); - *expr_p = lhs; - return GS_ALL_DONE; - } - else + if (!postfix) { + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1); return GS_OK; } + + /* Assign lhs to temporary variable. */ + t2 = create_tmp_var (TREE_TYPE (lhs), NULL); + gimplify_assign (t2, lhs, pre_p); + /* Do increment and assign it to lvalue. */ + t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs); + gimplify_assign (lvalue, t1, pre_p); + + gimplify_seq_add_seq (orig_post_p, post); + *expr_p = t2; + return GS_ALL_DONE; } /* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR. */ Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int __attribute__((noinline)) +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) + abort (); + return 0; +} Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) + abort (); + return 0; +} Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options -O2 -fdump-tree-optimized } */ + +volatile int count; +void bar(int); +void foo() +{ + bar(count++); +} + +/* { dg-final { scan-tree-dump-times count = 1 optimized} } */ +/* { dg-final { cleanup-tree-dump optimized } } */ Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c === ---
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther richard.guent...@gmail.com wrote: On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; As I explained this is the wrong place to fix the PR. The issue is not about self-modifying expressions but about evaluating call argument side-effects before side-effects of the lhs. I am testing the attached instead. Richard. 2012-02-09 Richard Guenther rguent...@suse.de PR middle-end/48814 * gimplify.c (gimplify_modify_expr): Perform side-effects of the RHS before those of the LHS. 2012-02-08 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814-1.c: New test. * gcc.c-torture/execute/pr48814-2.c: New test. * gcc.dg/tree-ssa/assign-1.c: New test. * gcc.dg/tree-ssa/assign-2.c: New test. fix-pr48814 Description: Binary data
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther richard.guent...@gmail.com wrote: On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther richard.guent...@gmail.com wrote: On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; As I explained this is the wrong place to fix the PR. The issue is not about self-modifying expressions but about evaluating call argument side-effects before side-effects of the lhs. I am testing the attached instead. Doesn't work. Btw, Kai, your patch surely breaks things if you put the lvalue update into the pre queue. Consider a simple a[i++] = i; you gimplify that to i.0 = i; D.1709 = i.0; i = D.1709 + 1; a[D.1709] = i; which is wrong. Seems we are lacking some basic pre-/post-modify testcases ... Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/2/9 Richard Guenther richard.guent...@gmail.com: On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther richard.guent...@gmail.com wrote: On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther richard.guent...@gmail.com wrote: On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; As I explained this is the wrong place to fix the PR. The issue is not about self-modifying expressions but about evaluating call argument side-effects before side-effects of the lhs. I am testing the attached instead. Doesn't work. Btw, Kai, your patch surely breaks things if you put the lvalue update into the pre queue. Consider a simple a[i++] = i; you gimplify that to i.0 = i; D.1709 = i.0; i = D.1709 + 1; a[D.1709] = i; which is wrong. Seems we are lacking some basic pre-/post-modify testcases ... Richard. Why, this should be wrong? In fact C specification just says that the post-inc has to happen at least before next sequence-point. It doesn't say that the increment has to happen after evaluation of rhs. The produced gimple for the following C-code int arr[128]; void foo (int i) { arr[i++] = i; } is: foo (int i) { int D.1364; D.1364 = i; i = D.1364 + 1; arr[D.1364] = i; } which looks to me from description of the C-specification correct. Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther richard.guent...@gmail.com wrote: On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/2/9 Richard Guenther richard.guent...@gmail.com: On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther richard.guent...@gmail.com wrote: On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther richard.guent...@gmail.com wrote: On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; As I explained this is the wrong place to fix the PR. The issue is not about self-modifying expressions but about evaluating call argument side-effects before side-effects of the lhs. I am testing the attached instead. Doesn't work. Btw, Kai, your patch surely breaks things if you put the lvalue update into the pre queue. Consider a simple a[i++] = i; you gimplify that to i.0 = i; D.1709 = i.0; i = D.1709 + 1; a[D.1709] = i; which is wrong. Seems we are lacking some basic pre-/post-modify testcases ... Richard. Why, this should be wrong? In fact C specification just says that the post-inc has to happen at least before next sequence-point. It doesn't say that the increment has to happen after evaluation of rhs. The produced gimple for the following C-code int arr[128]; void foo (int i) { arr[i++] = i; } is: foo (int i) { int D.1364; D.1364 = i; i = D.1364 + 1; arr[D.1364] = i; } which looks to me from description of the C-specification correct. Hm, indeed. I'll test the following shorter patch and add the struct-return volatile testcase. Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test Maybe invalid testcases. Who knows ... same fails happen with your patch. Richard. Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/11 Richard Guenther richard.guent...@gmail.com: count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. Thanks for explaination. I tried to flip order for lhs/rhs in gimplify_modify_expr co. Issue here is that for some cases we are relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++ like: typedef const unsigned char _Jv_Utf8Const; typedef __SIZE_TYPE__ uaddr; void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special) { union { _Jv_Utf8Const *signature; uaddr signature_bits; }; signature = s; special = signature_bits 1; signature_bits -= special; s = signature; } So I modified gimplify_self_mod_expr for post-inc/dec so that we use following sequence and add it to pre_p for it: tmp = lhs; lvalue = tmp (+/-) rhs *expr_p = tmp; ChangeLog 2012-02-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Move for postfix-inc/dec the modification in pre and return temporary with origin value. 2012-02-08 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814-1.c: New test. * gcc.c-torture/execute/pr48814-2.c: New test. * gcc.dg/tree-ssa/assign-1.c: New test. * gcc.dg/tree-ssa/assign-2.c: New test. I did boostrap for all languages (including Ada and Obj-C++) and regression tests on host x86_64-unknown-linux-gnu. Ok for apply? Regards, Kai Index: gcc/gcc/gimplify.c === --- gcc.orig/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi bool want_value) { enum tree_code code; - tree lhs, lvalue, rhs, t1; + tree lhs, lvalue, rhs, t1, t2; gimple_seq post = NULL, *orig_post_p = post_p; bool postfix; enum tree_code arith_code; @@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); - - if (postfix) -{ - gimplify_assign (lvalue, t1, orig_post_p); - gimplify_seq_add_seq (orig_post_p, post); - *expr_p = lhs; - return GS_ALL_DONE; -} - else + if (!postfix) { + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1); return GS_OK; } + + /* Assign lhs to temporary variable. */ + t2 = create_tmp_var (TREE_TYPE (lhs), NULL); + gimplify_assign (t2, lhs, pre_p); + /* Do increment and assign it to lvalue. */ + t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs); + gimplify_assign (lvalue, t1, pre_p); + + gimplify_seq_add_seq (orig_post_p, post); + *expr_p = t2; + return GS_ALL_DONE; } /* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR. */ Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int __attribute__((noinline)) +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) +abort (); + return 0; +} Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) +abort (); + return 0; +} Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options -O2 -fdump-tree-optimized } */ + +volatile int count; +void bar(int); +void foo() +{ + bar(count++); +} + +/* { dg-final { scan-tree-dump-times count = 1 optimized} } */ +/* { dg-final { cleanup-tree-dump optimized } } */ Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options -O2 -fdump-tree-optimized } */ + +volatile int count; +int arr[4]; +void foo() +{ + arr[count++] = 0; +} + +/* { dg-final { scan-tree-dump-times count = 1 optimized} } */ +/* { dg-final { cleanup-tree-dump optimized } } */ +
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/10 Richard Guenther richard.guent...@gmail.com: On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Hi Richard, Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. yes, exactly this makes a big difference for post-inc/dec. I show you gimple-dump to illustrate this in more detail. I used here -O2 option with using attached patch. gcc without that patch produces following gimple for main: main () { int count.0; int count.1; int D.2721; int D.2725; int D.2726; count.0 = count; -- here we store orginal value 'count' for having array-access-index D.2721 = incr (); - within that function count gets modified arr[count.0] = D.2721; count.1 = count.0 + 1; - Here happens the issue. We increment the saved value of 'count' count = count.1; - By this the modification of count in incr() gets void. ... By the change we make sure to use count's value instead its saved temporary. Patched gcc produces this gimple: main () { int count.0; int count.1; int D.1718; int D.1722; int D.1723; count.0 = count; D.1718 = incr (); arr[count.0] = D.1718; count.0 = count; -- Reload count.0 for post-inc/dec to use count's current value count.1 = count.0 + 1; count = count.1; count.0 = count; Ok, here is the patch with adusted testcase from PR. With your patch we generate wrong code for volatile int count; int arr[4]; void foo() { arr[count++] = 0; } as we load from count two times instead of once. Similar for volatile int count; void bar(int); void foo() { bar(count++); } Please add those as testcases for any revised patch you produce. Thanks, Richard. ChangeLog 2012-01-10 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai 2012-01-10 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814.c: New test. Index: gcc/gcc/gimplify.c === --- gcc.orig/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int __attribute__((noinline)) +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) + abort (); + return 0; +}
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther richard.guent...@gmail.com wrote: On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/10 Richard Guenther richard.guent...@gmail.com: On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Hi Richard, Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. yes, exactly this makes a big difference for post-inc/dec. I show you gimple-dump to illustrate this in more detail. I used here -O2 option with using attached patch. gcc without that patch produces following gimple for main: main () { int count.0; int count.1; int D.2721; int D.2725; int D.2726; count.0 = count; -- here we store orginal value 'count' for having array-access-index D.2721 = incr (); - within that function count gets modified arr[count.0] = D.2721; count.1 = count.0 + 1; - Here happens the issue. We increment the saved value of 'count' count = count.1; - By this the modification of count in incr() gets void. ... By the change we make sure to use count's value instead its saved temporary. Patched gcc produces this gimple: main () { int count.0; int count.1; int D.1718; int D.1722; int D.1723; count.0 = count; D.1718 = incr (); arr[count.0] = D.1718; count.0 = count; -- Reload count.0 for post-inc/dec to use count's current value count.1 = count.0 + 1; count = count.1; count.0 = count; Ok, here is the patch with adusted testcase from PR. With your patch we generate wrong code for volatile int count; int arr[4]; void foo() { arr[count++] = 0; } as we load from count two times instead of once. Similar for volatile int count; void bar(int); void foo() { bar(count++); } Please add those as testcases for any revised patch you produce. I think a proper fix involves changing the order we gimplify the lhs/rhs for calls. Thanks, Richard. ChangeLog 2012-01-10 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai 2012-01-10 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814.c: New test. Index: gcc/gcc/gimplify.c === --- gcc.orig/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int __attribute__((noinline)) +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) + abort (); + return 0; +}
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/11 Richard Guenther richard.guent...@gmail.com: On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther richard.guent...@gmail.com wrote: On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/10 Richard Guenther richard.guent...@gmail.com: On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Hi Richard, Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. yes, exactly this makes a big difference for post-inc/dec. I show you gimple-dump to illustrate this in more detail. I used here -O2 option with using attached patch. gcc without that patch produces following gimple for main: main () { int count.0; int count.1; int D.2721; int D.2725; int D.2726; count.0 = count; -- here we store orginal value 'count' for having array-access-index D.2721 = incr (); - within that function count gets modified arr[count.0] = D.2721; count.1 = count.0 + 1; - Here happens the issue. We increment the saved value of 'count' count = count.1; - By this the modification of count in incr() gets void. ... By the change we make sure to use count's value instead its saved temporary. Patched gcc produces this gimple: main () { int count.0; int count.1; int D.1718; int D.1722; int D.1723; count.0 = count; D.1718 = incr (); arr[count.0] = D.1718; count.0 = count; -- Reload count.0 for post-inc/dec to use count's current value count.1 = count.0 + 1; count = count.1; count.0 = count; Ok, here is the patch with adusted testcase from PR. With your patch we generate wrong code for volatile int count; int arr[4]; void foo() { arr[count++] = 0; } as we load from count two times instead of once. Similar for volatile int count; void bar(int); void foo() { bar(count++); } Please add those as testcases for any revised patch you produce. I think a proper fix involves changing the order we gimplify the lhs/rhs for calls. Hmm, I don't see actual wrong code here. But I might missed something in spec. For exampl1 we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; count.0 = count; arr[count.0] = 0; count.1 = count; count.2 = count.1 + 1; count = count.2; } and here is no wrong code AFAICS. For second example we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; volatile int count.3; count.0 = count; count.3 = count.0; count.1 = count; count.2 = count.1 + 1; count = count.2; bar (count.3); } Here we don't have wrong code either AFAICS. Passed argument to bar is the value before increment, and count get incremented by 1 before call of bar, which is right. Thanks for more details, Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Jan 11, 2012 at 11:19 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/11 Richard Guenther richard.guent...@gmail.com: On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther richard.guent...@gmail.com wrote: On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote: 2012/1/10 Richard Guenther richard.guent...@gmail.com: On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Hi Richard, Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. yes, exactly this makes a big difference for post-inc/dec. I show you gimple-dump to illustrate this in more detail. I used here -O2 option with using attached patch. gcc without that patch produces following gimple for main: main () { int count.0; int count.1; int D.2721; int D.2725; int D.2726; count.0 = count; -- here we store orginal value 'count' for having array-access-index D.2721 = incr (); - within that function count gets modified arr[count.0] = D.2721; count.1 = count.0 + 1; - Here happens the issue. We increment the saved value of 'count' count = count.1; - By this the modification of count in incr() gets void. ... By the change we make sure to use count's value instead its saved temporary. Patched gcc produces this gimple: main () { int count.0; int count.1; int D.1718; int D.1722; int D.1723; count.0 = count; D.1718 = incr (); arr[count.0] = D.1718; count.0 = count; -- Reload count.0 for post-inc/dec to use count's current value count.1 = count.0 + 1; count = count.1; count.0 = count; Ok, here is the patch with adusted testcase from PR. With your patch we generate wrong code for volatile int count; int arr[4]; void foo() { arr[count++] = 0; } as we load from count two times instead of once. Similar for volatile int count; void bar(int); void foo() { bar(count++); } Please add those as testcases for any revised patch you produce. I think a proper fix involves changing the order we gimplify the lhs/rhs for calls. Hmm, I don't see actual wrong code here. But I might missed something in spec. For exampl1 we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; count.0 = count; arr[count.0] = 0; count.1 = count; count.2 = count.1 + 1; count = count.2; } count despite being declared volatile and only loaded once in the source is loaded twice in gimple. If it were a HW register which destroys the device after the 2nd load without an intervening store you'd wrecked the device ;) Richard. and here is no wrong code AFAICS. For second example we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; volatile int count.3; count.0 = count; count.3 = count.0; count.1 = count; count.2 = count.1 + 1; count = count.2; bar (count.3); } Here we don't have wrong code either AFAICS. Passed argument to bar is the value before increment, and count get incremented by 1 before call of bar, which is right. Thanks for more details, Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. Richard. -- | (\_/) This is Bunny. Copy and paste | (='.'=) Bunny into your signature to help | ()_() him gain world domination
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/10 Richard Guenther richard.guent...@gmail.com: On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote: Ping 2012/1/8 Kai Tietz ktiet...@googlemail.com: Hi, this patch makes sure that for increment of postfix-increment/decrement we use also orignal lvalue instead of tmp lhs value for increment. This fixes reported issue about sequence point in PR/48814 ChangeLog 2012-01-08 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for x86_64-unknown-linux-gnu for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai Index: gimplify.c === --- gimplify.c (revision 182720) +++ gimplify.c (working copy) @@ -2258,7 +2258,7 @@ arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Hi Richard, Please add testcases. Why does your patch make a difference? lhs is just the gimplified lvalue. yes, exactly this makes a big difference for post-inc/dec. I show you gimple-dump to illustrate this in more detail. I used here -O2 option with using attached patch. gcc without that patch produces following gimple for main: main () { int count.0; int count.1; int D.2721; int D.2725; int D.2726; count.0 = count; -- here we store orginal value 'count' for having array-access-index D.2721 = incr (); - within that function count gets modified arr[count.0] = D.2721; count.1 = count.0 + 1; - Here happens the issue. We increment the saved value of 'count' count = count.1; - By this the modification of count in incr() gets void. ... By the change we make sure to use count's value instead its saved temporary. Patched gcc produces this gimple: main () { int count.0; int count.1; int D.1718; int D.1722; int D.1723; count.0 = count; D.1718 = incr (); arr[count.0] = D.1718; count.0 = count; -- Reload count.0 for post-inc/dec to use count's current value count.1 = count.0 + 1; count = count.1; count.0 = count; Ok, here is the patch with adusted testcase from PR. ChangeLog 2012-01-10 Kai Tietz kti...@redhat.com PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Use for postfix-inc/dec lvalue instead of temporary lhs. Regression tested for all languages (including Ada and Obj-C++). Ok for apply? Regards, Kai 2012-01-10 Kai Tietz kti...@redhat.com * gcc.c-torture/execute/pr48814.c: New test. Index: gcc/gcc/gimplify.c === --- gcc.orig/gcc/gimplify.c +++ gcc/gcc/gimplify.c @@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi arith_code = POINTER_PLUS_EXPR; } - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); if (postfix) { Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c === --- /dev/null +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c @@ -0,0 +1,18 @@ +extern void abort (void); + +int arr[] = {1,2,3,4}; +int count = 0; + +int __attribute__((noinline)) +incr (void) +{ + return ++count; +} + +int main() +{ + arr[count++] = incr (); + if (count != 2 || arr[count] != 3) +abort (); + return 0; +}