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