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.
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;
> +}
