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.
> Thanks,
> Richard.
>
>>
>> ChangeLog
>>
>> 2012-01-10 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 all languages (including Ada and Obj-C++). Ok for
>> apply?
>>
>> Regards,
>> Kai
>>
>> 2012-01-10 Kai Tietz <kti...@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;
>> +}
