2012/1/11 Richard Guenther <[email protected]>:
> On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther
> <[email protected]> wrote:
>> On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz <[email protected]> wrote:
>>> 2012/1/10 Richard Guenther <[email protected]>:
>>>> On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz <[email protected]>
>>>> wrote:
>>>>> Ping
>>>>>
>>>>> 2012/1/8 Kai Tietz <[email protected]>:
>>>>>> 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 <[email protected]>
>>>>>>
>>>>>> 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
