This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]