| Message ID | CAEwic4YWJriFkeTfyxn-SO4mxunbNkUzod2+UrRHAoHoU65LSg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/1/11 Richard Guenther <richard.guenther@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 <ktietz@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 <ktietz@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" } } */ > +
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2012/1/11 Richard Guenther <richard.guenther@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 <rguenther@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 <ktietz@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.
On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> 2012/1/11 Richard Guenther <richard.guenther@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.
2012/2/9 Richard Guenther <richard.guenther@gmail.com>: > On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> 2012/1/11 Richard Guenther <richard.guenther@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
On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/2/9 Richard Guenther <richard.guenther@gmail.com>: >> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>> 2012/1/11 Richard Guenther <richard.guenther@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. Richard.
On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>: >>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther >>>> <richard.guenther@gmail.com> wrote: >>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>>> 2012/1/11 Richard Guenther <richard.guenther@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.
2012/2/9 Richard Guenther <richard.guenther@gmail.com>: > On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>: >>>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther >>>> <richard.guenther@gmail.com> wrote: >>>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther >>>>> <richard.guenther@gmail.com> wrote: >>>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>>>> 2012/1/11 Richard Guenther <richard.guenther@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. 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. Regards, Kai
On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/2/9 Richard Guenther <richard.guenther@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
Kai Tietz <ktietz70@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.
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?
On Fri, Feb 10, 2012 at 2:12 PM, Jonathan Wakely <jwakely.gcc@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 = "10000"> </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 = "10000"> </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
2012/2/10 Richard Guenther <richard.guenther@gmail.com>: > On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2012/2/9 Richard Guenther <richard.guenther@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
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. Regards, Kai
On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@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? Thanks, Richard. > Regards, > Kai
On 15 March 2012 15:40, Richard Guenther wrote: > On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@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!)
On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: > On 15 March 2012 15:40, Richard Guenther wrote: >> On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@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.
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" } } */ +