Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result

[Richard Guenther <richard dot guenther at gmail dot com>]

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