* Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
@ 2012-01-10 9:59 Kai Tietz
2012-01-10 10:11 ` Richard Guenther
0 siblings, 1 reply; 25+ messages in thread
From: Kai Tietz @ 2012-01-10 9:59 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Guenther
Ping
2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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)
> {
--
| (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-10 9:59 Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result Kai Tietz @ 2012-01-10 10:11 ` Richard Guenther 2012-01-10 10:58 ` Kai Tietz 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-01-10 10:11 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: > Ping > > 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-10 10:11 ` Richard Guenther @ 2012-01-10 10:58 ` Kai Tietz 2012-01-11 10:05 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-01-10 10:58 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 2012/1/10 Richard Guenther <richard.guenther@gmail.com>: > On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >> Ping >> >> 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 <ktietz@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 <ktietz@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; +} ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-10 10:58 ` Kai Tietz @ 2012-01-11 10:05 ` Richard Guenther 2012-01-11 10:07 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-01-11 10:05 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/1/10 Richard Guenther <richard.guenther@gmail.com>: >> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> Ping >>> >>> 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 <ktietz@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 <ktietz@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; > +} ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-11 10:05 ` Richard Guenther @ 2012-01-11 10:07 ` Richard Guenther 2012-01-11 10:19 ` Kai Tietz 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-01-11 10:07 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2012/1/10 Richard Guenther <richard.guenther@gmail.com>: >>> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> Ping >>>> >>>> 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 <ktietz@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 <ktietz@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; >> +} ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-11 10:07 ` Richard Guenther @ 2012-01-11 10:19 ` Kai Tietz 2012-01-11 10:29 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-01-11 10:19 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 2012/1/11 Richard Guenther <richard.guenther@gmail.com>: > On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> 2012/1/10 Richard Guenther <richard.guenther@gmail.com>: >>>> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>> Ping >>>>> >>>>> 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-11 10:19 ` Kai Tietz @ 2012-01-11 10:29 ` Richard Guenther 2012-02-08 21:31 ` Kai Tietz 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-01-11 10:29 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches On Wed, Jan 11, 2012 at 11:19 AM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2012/1/11 Richard Guenther <richard.guenther@gmail.com>: >> On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> 2012/1/10 Richard Guenther <richard.guenther@gmail.com>: >>>>> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>>>> Ping >>>>>> >>>>>> 2012/1/8 Kai Tietz <ktietz70@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 <ktietz@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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-01-11 10:29 ` Richard Guenther @ 2012-02-08 21:31 ` Kai Tietz 2012-02-09 10:32 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-02-08 21:31 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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; 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" } } */ + ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-08 21:31 ` Kai Tietz @ 2012-02-09 10:32 ` Richard Guenther 2012-02-09 11:17 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-02-09 10:32 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches 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" } } */ > + ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 10:32 ` Richard Guenther @ 2012-02-09 11:17 ` Richard Guenther 2012-02-09 13:23 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-02-09 11:17 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches [-- Attachment #1: Type: text/plain, Size: 2054 bytes --] 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. [-- Attachment #2: fix-pr48814 --] [-- Type: application/octet-stream, Size: 4217 bytes --] 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. Index: gcc/gimplify.c =================================================================== *** gcc/gimplify.c (revision 184040) --- gcc/gimplify.c (working copy) *************** gimplify_modify_expr (tree *expr_p, gimp *** 4580,4585 **** --- 4580,4586 ---- enum gimplify_status ret = GS_UNHANDLED; gimple assign; location_t loc = EXPR_LOCATION (*expr_p); + gimple_seq lhs_pre = NULL; gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR || TREE_CODE (*expr_p) == INIT_EXPR); *************** gimplify_modify_expr (tree *expr_p, gimp *** 4630,4636 **** that is what we must do here. */ maybe_with_size_expr (from_p); ! ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue); if (ret == GS_ERROR) return ret; --- 4631,4637 ---- that is what we must do here. */ maybe_with_size_expr (from_p); ! ret = gimplify_expr (to_p, &lhs_pre, post_p, is_gimple_lvalue, fb_lvalue); if (ret == GS_ERROR) return ret; *************** gimplify_modify_expr (tree *expr_p, gimp *** 4651,4656 **** --- 4652,4665 ---- if (ret == GS_ERROR) return ret; + /* Perform side-effects of the LHS after side-effects of the RHS + which might be a call for example. */ + if (lhs_pre) + { + gimple_stmt_iterator gsi = gsi_last (*pre_p); + gsi_insert_seq_after_without_update (&gsi, lhs_pre, GSI_CONTINUE_LINKING); + } + /* Now see if the above changed *from_p to something we handle specially. */ ret = gimplify_modify_expr_rhs (expr_p, from_p, to_p, pre_p, post_p, want_value); Index: gcc/testsuite/gcc.c-torture/execute/pr48814-1.c =================================================================== *** gcc/testsuite/gcc.c-torture/execute/pr48814-1.c (revision 0) --- gcc/testsuite/gcc.c-torture/execute/pr48814-1.c (revision 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/testsuite/gcc.c-torture/execute/pr48814-2.c =================================================================== *** gcc/testsuite/gcc.c-torture/execute/pr48814-2.c (revision 0) --- gcc/testsuite/gcc.c-torture/execute/pr48814-2.c (revision 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/testsuite/gcc.dg/tree-ssa/assign-1.c =================================================================== *** gcc/testsuite/gcc.dg/tree-ssa/assign-1.c (revision 0) --- gcc/testsuite/gcc.dg/tree-ssa/assign-1.c (revision 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/testsuite/gcc.dg/tree-ssa/assign-2.c =================================================================== *** gcc/testsuite/gcc.dg/tree-ssa/assign-2.c (revision 0) --- gcc/testsuite/gcc.dg/tree-ssa/assign-2.c (revision 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" } } */ + ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 11:17 ` Richard Guenther @ 2012-02-09 13:23 ` Richard Guenther 2012-02-09 13:54 ` Kai Tietz 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-02-09 13:23 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 13:23 ` Richard Guenther @ 2012-02-09 13:54 ` Kai Tietz 2012-02-09 14:52 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-02-09 13:54 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 13:54 ` Kai Tietz @ 2012-02-09 14:52 ` Richard Guenther 2012-02-09 16:19 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-02-09 14:52 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches [-- Attachment #1: Type: text/plain, Size: 2980 bytes --] 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. [-- Attachment #2: fix-pr48814 --] [-- Type: application/octet-stream, Size: 4640 bytes --] 2012-02-09 Richard Guenther <rguenther@suse.de> Kai Tietz <ktietz@redhat.com> PR middle-end/48814 * gimplify.c (gimplify_self_mod_expr): Evaluate postfix side-effects completely in the pre-queue and use a temporary for the result. * 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. * gcc.dg/tree-ssa/assign-3.c: New test. Index: gcc/gimplify.c =================================================================== *** gcc/gimplify.c (revision 184040) --- gcc/gimplify.c (working copy) *************** gimplify_self_mod_expr (tree *expr_p, gi *** 2264,2280 **** 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 { *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1); return GS_OK; } --- 2264,2282 ---- arith_code = POINTER_PLUS_EXPR; } if (postfix) { ! tree t2 = get_initialized_tmp_var (lhs, pre_p, NULL); ! 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; } else { + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1); return GS_OK; } Index: gcc/testsuite/gcc.c-torture/execute/pr48814-1.c =================================================================== *** gcc/testsuite/gcc.c-torture/execute/pr48814-1.c (revision 0) --- gcc/testsuite/gcc.c-torture/execute/pr48814-1.c (revision 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/testsuite/gcc.c-torture/execute/pr48814-2.c =================================================================== *** gcc/testsuite/gcc.c-torture/execute/pr48814-2.c (revision 0) --- gcc/testsuite/gcc.c-torture/execute/pr48814-2.c (revision 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/testsuite/gcc.dg/tree-ssa/assign-1.c =================================================================== *** gcc/testsuite/gcc.dg/tree-ssa/assign-1.c (revision 0) --- gcc/testsuite/gcc.dg/tree-ssa/assign-1.c (revision 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/testsuite/gcc.dg/tree-ssa/assign-2.c =================================================================== *** gcc/testsuite/gcc.dg/tree-ssa/assign-2.c (revision 0) --- gcc/testsuite/gcc.dg/tree-ssa/assign-2.c (revision 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" } } */ + Index: gcc/testsuite/gcc.dg/tree-ssa/assign-3.c =================================================================== *** gcc/testsuite/gcc.dg/tree-ssa/assign-3.c (revision 0) --- gcc/testsuite/gcc.dg/tree-ssa/assign-3.c (revision 0) *************** *** 0 **** --- 1,24 ---- + /* { dg-do run } */ + /* { dg-options "-O2 -fdump-tree-gimple" } */ + + extern void abort (void); + struct S { int i; }; + struct S arr[32]; + volatile int count = 0; + + struct S __attribute__((noinline)) + incr () + { + ++count; + } + + int main() + { + arr[count++] = incr (); + if (count != 2) + abort (); + return 0; + } + + /* { dg-final { scan-tree-dump-times " = count;" 3 "gimple" } } */ + /* { dg-final { cleanup-tree-dump "gimple" } } */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 14:52 ` Richard Guenther @ 2012-02-09 16:19 ` Richard Guenther 2012-02-10 9:44 ` Kai Tietz 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-02-09 16:19 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-09 16:19 ` Richard Guenther @ 2012-02-10 9:44 ` Kai Tietz 2012-02-10 11:09 ` Richard Guenther 2012-02-10 13:12 ` Andreas Schwab 0 siblings, 2 replies; 25+ messages in thread From: Kai Tietz @ 2012-02-10 9:44 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 9:44 ` Kai Tietz @ 2012-02-10 11:09 ` Richard Guenther 2012-02-10 13:28 ` Jonathan Wakely 2012-02-10 15:20 ` Kai Tietz 2012-02-10 13:12 ` Andreas Schwab 1 sibling, 2 replies; 25+ messages in thread From: Richard Guenther @ 2012-02-10 11:09 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches, libstdc++ 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 11:09 ` Richard Guenther @ 2012-02-10 13:28 ` Jonathan Wakely 2012-02-10 13:31 ` Richard Guenther 2012-02-10 15:20 ` Kai Tietz 1 sibling, 1 reply; 25+ messages in thread From: Jonathan Wakely @ 2012-02-10 13:28 UTC (permalink / raw) To: Richard Guenther; +Cc: Kai Tietz, GCC Patches, libstdc++ 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? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 13:28 ` Jonathan Wakely @ 2012-02-10 13:31 ` Richard Guenther 0 siblings, 0 replies; 25+ messages in thread From: Richard Guenther @ 2012-02-10 13:31 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Kai Tietz, GCC Patches, libstdc++ 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 11:09 ` Richard Guenther 2012-02-10 13:28 ` Jonathan Wakely @ 2012-02-10 15:20 ` Kai Tietz 2012-03-15 15:22 ` Kai Tietz 1 sibling, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-02-10 15:20 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches, libstdc++ 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 15:20 ` Kai Tietz @ 2012-03-15 15:22 ` Kai Tietz 2012-03-15 15:41 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Kai Tietz @ 2012-03-15 15:22 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches, libstdc++ 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-03-15 15:22 ` Kai Tietz @ 2012-03-15 15:41 ` Richard Guenther 2012-03-16 0:29 ` Jonathan Wakely 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-03-15 15:41 UTC (permalink / raw) To: Kai Tietz; +Cc: GCC Patches, libstdc++, Jonathan Wakely 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-03-15 15:41 ` Richard Guenther @ 2012-03-16 0:29 ` Jonathan Wakely 2012-03-16 10:11 ` Richard Guenther 0 siblings, 1 reply; 25+ messages in thread From: Jonathan Wakely @ 2012-03-16 0:29 UTC (permalink / raw) To: Richard Guenther; +Cc: Kai Tietz, GCC Patches, libstdc++ 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!) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-03-16 0:29 ` Jonathan Wakely @ 2012-03-16 10:11 ` Richard Guenther 2012-03-19 21:43 ` Benjamin De Kosnik 0 siblings, 1 reply; 25+ messages in thread From: Richard Guenther @ 2012-03-16 10:11 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Kai Tietz, GCC Patches, libstdc++ 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-03-16 10:11 ` Richard Guenther @ 2012-03-19 21:43 ` Benjamin De Kosnik 0 siblings, 0 replies; 25+ messages in thread From: Benjamin De Kosnik @ 2012-03-19 21:43 UTC (permalink / raw) To: Richard Guenther; +Cc: Jonathan Wakely, Kai Tietz, GCC Patches, libstdc++ [-- Attachment #1: Type: text/plain, Size: 1116 bytes --] On Fri, 16 Mar 2012 11:10:48 +0100 Richard Guenther <richard.guenther@gmail.com> wrote: > 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. FYI, here is the patch for the new libstdc++ fails. -benjamin [-- Attachment #2: 20120319-2.patch --] [-- Type: text/x-patch, Size: 2274 bytes --] 2012-03-19 Benjamin Kosnik <bkoz@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, ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result 2012-02-10 9:44 ` Kai Tietz 2012-02-10 11:09 ` Richard Guenther @ 2012-02-10 13:12 ` Andreas Schwab 1 sibling, 0 replies; 25+ messages in thread From: Andreas Schwab @ 2012-02-10 13:12 UTC (permalink / raw) To: Kai Tietz; +Cc: Richard Guenther, GCC Patches 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. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2012-03-19 21:43 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-10 9:59 Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result Kai Tietz 2012-01-10 10:11 ` Richard Guenther 2012-01-10 10:58 ` Kai Tietz 2012-01-11 10:05 ` Richard Guenther 2012-01-11 10:07 ` Richard Guenther 2012-01-11 10:19 ` Kai Tietz 2012-01-11 10:29 ` Richard Guenther 2012-02-08 21:31 ` Kai Tietz 2012-02-09 10:32 ` Richard Guenther 2012-02-09 11:17 ` Richard Guenther 2012-02-09 13:23 ` Richard Guenther 2012-02-09 13:54 ` Kai Tietz 2012-02-09 14:52 ` Richard Guenther 2012-02-09 16:19 ` Richard Guenther 2012-02-10 9:44 ` Kai Tietz 2012-02-10 11:09 ` Richard Guenther 2012-02-10 13:28 ` Jonathan Wakely 2012-02-10 13:31 ` Richard Guenther 2012-02-10 15:20 ` Kai Tietz 2012-03-15 15:22 ` Kai Tietz 2012-03-15 15:41 ` Richard Guenther 2012-03-16 0:29 ` Jonathan Wakely 2012-03-16 10:11 ` Richard Guenther 2012-03-19 21:43 ` Benjamin De Kosnik 2012-02-10 13:12 ` Andreas Schwab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).