2012/1/11 Richard Guenther <richard.guent...@gmail.com>: > On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther > <richard.guent...@gmail.com> wrote: >> On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <ktiet...@googlemail.com> wrote: >>> 2012/1/10 Richard Guenther <richard.guent...@gmail.com>: >>>> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <ktiet...@googlemail.com> >>>> wrote: >>>>> Ping >>>>> >>>>> 2012/1/8 Kai Tietz <ktiet...@googlemail.com>: >>>>>> Hi, >>>>>> >>>>>> this patch makes sure that for increment of >>>>>> postfix-increment/decrement we use also orignal lvalue instead of tmp >>>>>> lhs value for increment. This fixes reported issue about sequence >>>>>> point in PR/48814 >>>>>> >>>>>> ChangeLog >>>>>> >>>>>> 2012-01-08 Kai Tietz <kti...@redhat.com> >>>>>> >>>>>> PR middle-end/48814 >>>>>> * gimplify.c (gimplify_self_mod_expr): Use for >>>>>> postfix-inc/dec lvalue instead of temporary >>>>>> lhs. >>>>>> >>>>>> Regression tested for x86_64-unknown-linux-gnu for all languages >>>>>> (including Ada and Obj-C++). Ok for apply? >>>>>> >>>>>> Regards, >>>>>> Kai >>>>>> >>>>>> Index: gimplify.c >>>>>> =================================================================== >>>>>> --- gimplify.c (revision 182720) >>>>>> +++ gimplify.c (working copy) >>>>>> @@ -2258,7 +2258,7 @@ >>>>>> arith_code = POINTER_PLUS_EXPR; >>>>>> } >>>>>> >>>>>> - t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs); >>>>>> + t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs); >>>>>> >>>>>> if (postfix) >>>>>> { >>> >>> Hi Richard, >>> >>>> Please add testcases. Why does your patch make a difference? >>>> lhs is just the gimplified lvalue. >>> >>> yes, exactly this makes a big difference for post-inc/dec. I show you >>> gimple-dump to illustrate this in more detail. I used here -O2 option >>> with using attached patch. >>> >>> gcc without that patch produces following gimple for main: >>> >>> main () >>> { >>> int count.0; >>> int count.1; >>> int D.2721; >>> int D.2725; >>> int D.2726; >>> >>> count.0 = count; <-- here we store orginal value 'count' for having >>> array-access-index >>> D.2721 = incr (); <- within that function count gets modified >>> arr[count.0] = D.2721; >>> count.1 = count.0 + 1; <- Here happens the issue. We increment the >>> saved value of 'count' >>> count = count.1; <- By this the modification of count in incr() gets void. >>> ... >>> >>> By the change we make sure to use count's value instead its saved temporary. >>> >>> Patched gcc produces this gimple: >>> >>> main () >>> { >>> int count.0; >>> int count.1; >>> int D.1718; >>> int D.1722; >>> int D.1723; >>> >>> count.0 = count; >>> D.1718 = incr (); >>> arr[count.0] = D.1718; >>> count.0 = count; <-- Reload count.0 for post-inc/dec to use count's >>> current value >>> count.1 = count.0 + 1; >>> count = count.1; >>> count.0 = count; >>> >>> Ok, here is the patch with adusted testcase from PR. >> >> With your patch we generate wrong code for >> >> volatile int count; >> int arr[4]; >> void foo() >> { >> arr[count++] = 0; >> } >> >> as we load from count two times instead of once. Similar for >> >> volatile int count; >> void bar(int); >> void foo() >> { >> bar(count++); >> } >> >> Please add those as testcases for any revised patch you produce. > > I think a proper fix involves changing the order we gimplify the > lhs/rhs for calls.
Hmm, I don't see actual wrong code here. But I might missed something in spec. For exampl1 we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; count.0 = count; arr[count.0] = 0; count.1 = count; count.2 = count.1 + 1; count = count.2; } and here is no wrong code AFAICS. For second example we get: foo () { volatile int count.0; volatile int count.1; volatile int count.2; volatile int count.3; count.0 = count; count.3 = count.0; count.1 = count; count.2 = count.1 + 1; count = count.2; bar (count.3); } Here we don't have wrong code either AFAICS. Passed argument to bar is the value before increment, and count get incremented by 1 before call of bar, which is right. Thanks for more details, Kai