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

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

[Kai Tietz]

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)
>     {



-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

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

[Richard Guenther]

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)
>>     {

Please add testcases.  Why does your patch make a difference?
lhs is just the gimplified lvalue.

Richard.

>
>
> --
> |  (\_/) This is Bunny. Copy and paste
> | (='.'=) Bunny into your signature to help
> | (")_(") him gain world domination

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

[Kai Tietz]

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.

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

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

[Richard Guenther]

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.

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

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

[Richard Guenther]

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

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

[Kai Tietz]

2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
> 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.

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

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

[Richard Guenther]

On Wed, Jan 11, 2012 at 11:19 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>> 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.
>
> 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

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

[Kai Tietz]

2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>
> 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.

Thanks for explaination.  I tried to flip order for lhs/rhs in
gimplify_modify_expr & co.  Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:

typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;

void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
{
  union {
    _Jv_Utf8Const *signature;
    uaddr signature_bits;
  };
  signature = s;
  special = signature_bits & 1;
  signature_bits -= special;
  s = signature;
}

So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:

tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;

ChangeLog

2012-02-08  Kai Tietz  <ktietz@redhat.com>

	PR middle-end/48814
	* gimplify.c (gimplify_self_mod_expr): Move for
	postfix-inc/dec the modification in pre and return
	temporary with origin value.

2012-02-08  Kai Tietz  <ktietz@redhat.com>

	* gcc.c-torture/execute/pr48814-1.c: New test.
	* gcc.c-torture/execute/pr48814-2.c: New test.
	* gcc.dg/tree-ssa/assign-1.c: New test.
	* gcc.dg/tree-ssa/assign-2.c: New test.

I did boostrap for all languages (including Ada and Obj-C++) and
regression tests on host x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

Index: gcc/gcc/gimplify.c
===================================================================
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
 			bool want_value)
 {
   enum tree_code code;
-  tree lhs, lvalue, rhs, t1;
+  tree lhs, lvalue, rhs, t1, t2;
   gimple_seq post = NULL, *orig_post_p = post_p;
   bool postfix;
   enum tree_code arith_code;
@@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi
       arith_code = POINTER_PLUS_EXPR;
     }

-  t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
-
-  if (postfix)
-    {
-      gimplify_assign (lvalue, t1, orig_post_p);
-      gimplify_seq_add_seq (orig_post_p, post);
-      *expr_p = lhs;
-      return GS_ALL_DONE;
-    }
-  else
+  if (!postfix)
     {
+      t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
       *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
       return GS_OK;
     }
+
+  /* Assign lhs to temporary variable.  */
+  t2 = create_tmp_var (TREE_TYPE (lhs), NULL);
+  gimplify_assign (t2, lhs, pre_p);
+  /* Do increment and assign it to lvalue.  */
+  t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs);
+  gimplify_assign (lvalue, t1, pre_p);
+
+  gimplify_seq_add_seq (orig_post_p, post);
+  *expr_p = t2;
+  return GS_ALL_DONE;
 }

 /* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR.  */
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.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: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int
+incr (void)
+{
+  return ++count;
+}
+
+int main()
+{
+  arr[count++] = incr ();
+  if (count != 2 || arr[count] != 3)
+    abort ();
+  return 0;
+}
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+volatile int count;
+void bar(int);
+void foo()
+{
+ bar(count++);
+}
+
+/* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+volatile int count;
+int arr[4];
+void foo()
+{
+ arr[count++] = 0;
+}
+
+/* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+

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

[Richard Guenther]

On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>
>> 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.
>
> Thanks for explaination.  I tried to flip order for lhs/rhs in
> gimplify_modify_expr & co.  Issue here is that for some cases we are
> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
> like:
>
> typedef const unsigned char _Jv_Utf8Const;
> typedef __SIZE_TYPE__ uaddr;
>
> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
> {
>  union {
>    _Jv_Utf8Const *signature;
>    uaddr signature_bits;
>  };
>  signature = s;
>  special = signature_bits & 1;
>  signature_bits -= special;
>  s = signature;
> }
>
> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
> following sequence
> and add it to pre_p for it:
>
> tmp = lhs;
> lvalue = tmp (+/-) rhs
> *expr_p = tmp;

As I explained this is the wrong place to fix the PR.  The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.

Richard.

> ChangeLog
>
> 2012-02-08  Kai Tietz  <ktietz@redhat.com>
>
>        PR middle-end/48814
>        * gimplify.c (gimplify_self_mod_expr): Move for
>        postfix-inc/dec the modification in pre and return
>        temporary with origin value.
>
> 2012-02-08  Kai Tietz  <ktietz@redhat.com>
>
>        * gcc.c-torture/execute/pr48814-1.c: New test.
>        * gcc.c-torture/execute/pr48814-2.c: New test.
>        * gcc.dg/tree-ssa/assign-1.c: New test.
>        * gcc.dg/tree-ssa/assign-2.c: New test.
>
> I did boostrap for all languages (including Ada and Obj-C++) and
> regression tests on host x86_64-unknown-linux-gnu.  Ok for apply?
>
> Regards,
> Kai
>
> Index: gcc/gcc/gimplify.c
> ===================================================================
> --- gcc.orig/gcc/gimplify.c
> +++ gcc/gcc/gimplify.c
> @@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
>                        bool want_value)
>  {
>   enum tree_code code;
> -  tree lhs, lvalue, rhs, t1;
> +  tree lhs, lvalue, rhs, t1, t2;
>   gimple_seq post = NULL, *orig_post_p = post_p;
>   bool postfix;
>   enum tree_code arith_code;
> @@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi
>       arith_code = POINTER_PLUS_EXPR;
>     }
>
> -  t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
> -
> -  if (postfix)
> -    {
> -      gimplify_assign (lvalue, t1, orig_post_p);
> -      gimplify_seq_add_seq (orig_post_p, post);
> -      *expr_p = lhs;
> -      return GS_ALL_DONE;
> -    }
> -  else
> +  if (!postfix)
>     {
> +      t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
>       *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
>       return GS_OK;
>     }
> +
> +  /* Assign lhs to temporary variable.  */
> +  t2 = create_tmp_var (TREE_TYPE (lhs), NULL);
> +  gimplify_assign (t2, lhs, pre_p);
> +  /* Do increment and assign it to lvalue.  */
> +  t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs);
> +  gimplify_assign (lvalue, t1, pre_p);
> +
> +  gimplify_seq_add_seq (orig_post_p, post);
> +  *expr_p = t2;
> +  return GS_ALL_DONE;
>  }
>
>  /* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR.  */
> Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.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: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
> @@ -0,0 +1,18 @@
> +extern void abort (void);
> +
> +int arr[] = {1,2,3,4};
> +int count = 0;
> +
> +int
> +incr (void)
> +{
> +  return ++count;
> +}
> +
> +int main()
> +{
> +  arr[count++] = incr ();
> +  if (count != 2 || arr[count] != 3)
> +    abort ();
> +  return 0;
> +}
> Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +volatile int count;
> +void bar(int);
> +void foo()
> +{
> + bar(count++);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +volatile int count;
> +int arr[4];
> +void foo()
> +{
> + arr[count++] = 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> +

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

[Richard Guenther]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>
>>> 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.
>>
>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>> like:
>>
>> typedef const unsigned char _Jv_Utf8Const;
>> typedef __SIZE_TYPE__ uaddr;
>>
>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>> {
>>  union {
>>    _Jv_Utf8Const *signature;
>>    uaddr signature_bits;
>>  };
>>  signature = s;
>>  special = signature_bits & 1;
>>  signature_bits -= special;
>>  s = signature;
>> }
>>
>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>> following sequence
>> and add it to pre_p for it:
>>
>> tmp = lhs;
>> lvalue = tmp (+/-) rhs
>> *expr_p = tmp;
>
> As I explained this is the wrong place to fix the PR.  The issue is not
> about self-modifying expressions but about evaluating call argument
> side-effects before side-effects of the lhs.

I am testing the attached instead.

Richard.

2012-02-09  Richard Guenther  <rguenther@suse.de>

        PR middle-end/48814
        * gimplify.c (gimplify_modify_expr): Perform side-effects of
        the RHS before those of the LHS.

2012-02-08  Kai Tietz  <ktietz@redhat.com>

        * gcc.c-torture/execute/pr48814-1.c: New test.
        * gcc.c-torture/execute/pr48814-2.c: New test.
        * gcc.dg/tree-ssa/assign-1.c: New test.
        * gcc.dg/tree-ssa/assign-2.c: New test.

[-- Attachment #2: fix-pr48814 --]
[-- Type: application/octet-stream, Size: 4217 bytes --]

2012-02-09  Richard Guenther  <rguenther@suse.de>

	PR middle-end/48814
	* gimplify.c (gimplify_modify_expr): Perform side-effects of
	the RHS before those of the LHS.

2012-02-08  Kai Tietz  <ktietz@redhat.com>

	* gcc.c-torture/execute/pr48814-1.c: New test.
	* gcc.c-torture/execute/pr48814-2.c: New test.
	* gcc.dg/tree-ssa/assign-1.c: New test.
	* gcc.dg/tree-ssa/assign-2.c: New test.

Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c	(revision 184040)
--- gcc/gimplify.c	(working copy)
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4580,4585 ****
--- 4580,4586 ----
    enum gimplify_status ret = GS_UNHANDLED;
    gimple assign;
    location_t loc = EXPR_LOCATION (*expr_p);
+   gimple_seq lhs_pre = NULL;
  
    gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
  	      || TREE_CODE (*expr_p) == INIT_EXPR);
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4630,4636 ****
       that is what we must do here.  */
    maybe_with_size_expr (from_p);
  
!   ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
    if (ret == GS_ERROR)
      return ret;
  
--- 4631,4637 ----
       that is what we must do here.  */
    maybe_with_size_expr (from_p);
  
!   ret = gimplify_expr (to_p, &lhs_pre, post_p, is_gimple_lvalue, fb_lvalue);
    if (ret == GS_ERROR)
      return ret;
  
*************** gimplify_modify_expr (tree *expr_p, gimp
*** 4651,4656 ****
--- 4652,4665 ----
    if (ret == GS_ERROR)
      return ret;
  
+   /* Perform side-effects of the LHS after side-effects of the RHS
+      which might be a call for example.  */
+   if (lhs_pre)
+     {
+       gimple_stmt_iterator gsi = gsi_last (*pre_p);
+       gsi_insert_seq_after_without_update (&gsi, lhs_pre, GSI_CONTINUE_LINKING);
+     }
+ 
    /* Now see if the above changed *from_p to something we handle specially.  */
    ret = gimplify_modify_expr_rhs (expr_p, from_p, to_p, pre_p, post_p,
  				  want_value);
Index: gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr48814-1.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr48814-1.c	(revision 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: gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr48814-2.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr48814-2.c	(revision 0)
***************
*** 0 ****
--- 1,18 ----
+ extern void abort (void);
+ 
+ int arr[] = {1,2,3,4};
+ int count = 0;
+ 
+ int
+ incr (void)
+ {
+   return ++count;
+ }
+ 
+ int main()
+ {
+   arr[count++] = incr ();
+   if (count != 2 || arr[count] != 3)
+     abort ();
+   return 0;
+ }
Index: gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/assign-1.c	(revision 0)
--- gcc/testsuite/gcc.dg/tree-ssa/assign-1.c	(revision 0)
***************
*** 0 ****
--- 1,12 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ volatile int count;
+ void bar(int);
+ void foo()
+ {
+  bar(count++);
+ }
+ 
+ /* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+ /* { dg-final { cleanup-tree-dump "optimized" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/assign-2.c	(revision 0)
--- gcc/testsuite/gcc.dg/tree-ssa/assign-2.c	(revision 0)
***************
*** 0 ****
--- 1,13 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ volatile int count;
+ int arr[4];
+ void foo()
+ {
+  arr[count++] = 0;
+ }
+ 
+ /* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+ /* { dg-final { cleanup-tree-dump "optimized" } } */
+ 

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

[Richard Guenther]

On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>
>>>> 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.
>>>
>>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>> like:
>>>
>>> typedef const unsigned char _Jv_Utf8Const;
>>> typedef __SIZE_TYPE__ uaddr;
>>>
>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>> {
>>>  union {
>>>    _Jv_Utf8Const *signature;
>>>    uaddr signature_bits;
>>>  };
>>>  signature = s;
>>>  special = signature_bits & 1;
>>>  signature_bits -= special;
>>>  s = signature;
>>> }
>>>
>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>> following sequence
>>> and add it to pre_p for it:
>>>
>>> tmp = lhs;
>>> lvalue = tmp (+/-) rhs
>>> *expr_p = tmp;
>>
>> As I explained this is the wrong place to fix the PR.  The issue is not
>> about self-modifying expressions but about evaluating call argument
>> side-effects before side-effects of the lhs.
>
> I am testing the attached instead.

Doesn't work.  Btw, Kai, your patch surely breaks things if you put
the lvalue update into the pre queue.

Consider a simple

 a[i++] = i;

you gimplify that to

  i.0 = i;
  D.1709 = i.0;
  i = D.1709 + 1;
  a[D.1709] = i;

which is wrong.

Seems we are lacking some basic pre-/post-modify testcases ...

Richard.

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

[Kai Tietz]

2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>>
>>>>> 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.
>>>>
>>>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>>>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>>> like:
>>>>
>>>> typedef const unsigned char _Jv_Utf8Const;
>>>> typedef __SIZE_TYPE__ uaddr;
>>>>
>>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>>> {
>>>>  union {
>>>>    _Jv_Utf8Const *signature;
>>>>    uaddr signature_bits;
>>>>  };
>>>>  signature = s;
>>>>  special = signature_bits & 1;
>>>>  signature_bits -= special;
>>>>  s = signature;
>>>> }
>>>>
>>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>>> following sequence
>>>> and add it to pre_p for it:
>>>>
>>>> tmp = lhs;
>>>> lvalue = tmp (+/-) rhs
>>>> *expr_p = tmp;
>>>
>>> As I explained this is the wrong place to fix the PR.  The issue is not
>>> about self-modifying expressions but about evaluating call argument
>>> side-effects before side-effects of the lhs.
>>
>> I am testing the attached instead.
>
> Doesn't work.  Btw, Kai, your patch surely breaks things if you put
> the lvalue update into the pre queue.
>
> Consider a simple
>
>  a[i++] = i;
>
> you gimplify that to
>
>  i.0 = i;
>  D.1709 = i.0;
>  i = D.1709 + 1;
>  a[D.1709] = i;
>
> which is wrong.
>
> Seems we are lacking some basic pre-/post-modify testcases ...
>
> Richard.

Why, this should be wrong?  In fact C specification just says that the
post-inc has to happen at least before next sequence-point.  It
doesn't say that the increment has to happen after evaluation of rhs.

The produced gimple for the following C-code

int arr[128];

void foo (int i)
{
  arr[i++] = i;
}

is:

foo (int i)
{
  int D.1364;

  D.1364 = i;
  i = D.1364 + 1;
  arr[D.1364] = i;
}

which looks to me from description of the C-specification correct.

Kai

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

[Richard Guenther]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>>>>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>>>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>>>> like:
>>>>>
>>>>> typedef const unsigned char _Jv_Utf8Const;
>>>>> typedef __SIZE_TYPE__ uaddr;
>>>>>
>>>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>>>> {
>>>>>  union {
>>>>>    _Jv_Utf8Const *signature;
>>>>>    uaddr signature_bits;
>>>>>  };
>>>>>  signature = s;
>>>>>  special = signature_bits & 1;
>>>>>  signature_bits -= special;
>>>>>  s = signature;
>>>>> }
>>>>>
>>>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>>>> following sequence
>>>>> and add it to pre_p for it:
>>>>>
>>>>> tmp = lhs;
>>>>> lvalue = tmp (+/-) rhs
>>>>> *expr_p = tmp;
>>>>
>>>> As I explained this is the wrong place to fix the PR.  The issue is not
>>>> about self-modifying expressions but about evaluating call argument
>>>> side-effects before side-effects of the lhs.
>>>
>>> I am testing the attached instead.
>>
>> Doesn't work.  Btw, Kai, your patch surely breaks things if you put
>> the lvalue update into the pre queue.
>>
>> Consider a simple
>>
>>  a[i++] = i;
>>
>> you gimplify that to
>>
>>  i.0 = i;
>>  D.1709 = i.0;
>>  i = D.1709 + 1;
>>  a[D.1709] = i;
>>
>> which is wrong.
>>
>> Seems we are lacking some basic pre-/post-modify testcases ...
>>
>> Richard.
>
> Why, this should be wrong?  In fact C specification just says that the
> post-inc has to happen at least before next sequence-point.  It
> doesn't say that the increment has to happen after evaluation of rhs.
>
> The produced gimple for the following C-code
>
> int arr[128];
>
> void foo (int i)
> {
>  arr[i++] = i;
> }
>
> is:
>
> foo (int i)
> {
>  int D.1364;
>
>  D.1364 = i;
>  i = D.1364 + 1;
>  arr[D.1364] = i;
> }
>
> which looks to me from description of the C-specification correct.

Hm, indeed.  I'll test the following shorter patch and add the struct-return
volatile testcase.

Richard.

[-- Attachment #2: fix-pr48814 --]
[-- Type: application/octet-stream, Size: 4640 bytes --]

2012-02-09  Richard Guenther  <rguenther@suse.de>
	Kai Tietz  <ktietz@redhat.com>

	PR middle-end/48814
	* gimplify.c (gimplify_self_mod_expr): Evaluate postfix
	side-effects completely in the pre-queue and use a temporary
	for the result.

	* gcc.c-torture/execute/pr48814-1.c: New test.
	* gcc.c-torture/execute/pr48814-2.c: New test.
	* gcc.dg/tree-ssa/assign-1.c: New test.
	* gcc.dg/tree-ssa/assign-2.c: New test.
	* gcc.dg/tree-ssa/assign-3.c: New test.

Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c	(revision 184040)
--- gcc/gimplify.c	(working copy)
*************** gimplify_self_mod_expr (tree *expr_p, gi
*** 2264,2280 ****
        arith_code = POINTER_PLUS_EXPR;
      }
  
-   t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
  
    if (postfix)
      {
!       gimplify_assign (lvalue, t1, orig_post_p);
        gimplify_seq_add_seq (orig_post_p, post);
!       *expr_p = lhs;
        return GS_ALL_DONE;
      }
    else
      {
        *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
        return GS_OK;
      }
--- 2264,2282 ----
        arith_code = POINTER_PLUS_EXPR;
      }
  
  
    if (postfix)
      {
!       tree t2 = get_initialized_tmp_var (lhs, pre_p, NULL);
!       t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs);
!       gimplify_assign (lvalue, t1, pre_p);
        gimplify_seq_add_seq (orig_post_p, post);
!       *expr_p = t2;
        return GS_ALL_DONE;
      }
    else
      {
+       t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
        *expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
        return GS_OK;
      }
Index: gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr48814-1.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr48814-1.c	(revision 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: gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr48814-2.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr48814-2.c	(revision 0)
***************
*** 0 ****
--- 1,18 ----
+ extern void abort (void);
+ 
+ int arr[] = {1,2,3,4};
+ int count = 0;
+ 
+ int
+ incr (void)
+ {
+   return ++count;
+ }
+ 
+ int main()
+ {
+   arr[count++] = incr ();
+   if (count != 2 || arr[count] != 3)
+     abort ();
+   return 0;
+ }
Index: gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/assign-1.c	(revision 0)
--- gcc/testsuite/gcc.dg/tree-ssa/assign-1.c	(revision 0)
***************
*** 0 ****
--- 1,12 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ volatile int count;
+ void bar(int);
+ void foo()
+ {
+  bar(count++);
+ }
+ 
+ /* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+ /* { dg-final { cleanup-tree-dump "optimized" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/assign-2.c	(revision 0)
--- gcc/testsuite/gcc.dg/tree-ssa/assign-2.c	(revision 0)
***************
*** 0 ****
--- 1,13 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ volatile int count;
+ int arr[4];
+ void foo()
+ {
+  arr[count++] = 0;
+ }
+ 
+ /* { dg-final { scan-tree-dump-times "count =" 1 "optimized"} } */
+ /* { dg-final { cleanup-tree-dump "optimized" } } */
+ 
Index: gcc/testsuite/gcc.dg/tree-ssa/assign-3.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/assign-3.c	(revision 0)
--- gcc/testsuite/gcc.dg/tree-ssa/assign-3.c	(revision 0)
***************
*** 0 ****
--- 1,24 ----
+ /* { dg-do run } */
+ /* { dg-options "-O2 -fdump-tree-gimple" } */
+ 
+ extern void abort (void);
+ struct S { int i; };
+ struct S arr[32];
+ volatile int count = 0;
+ 
+ struct S __attribute__((noinline))
+ incr ()
+ {
+   ++count;
+ }
+ 
+ int main()
+ {
+   arr[count++] = incr ();
+   if (count != 2)
+     abort ();
+   return 0;
+ }
+ 
+ /* { dg-final { scan-tree-dump-times " = count;" 3 "gimple" } } */
+ /* { dg-final { cleanup-tree-dump "gimple" } } */

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

[Richard Guenther]

On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>>>>>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>>>>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>>>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>>>>> like:
>>>>>>
>>>>>> typedef const unsigned char _Jv_Utf8Const;
>>>>>> typedef __SIZE_TYPE__ uaddr;
>>>>>>
>>>>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>>>>> {
>>>>>>  union {
>>>>>>    _Jv_Utf8Const *signature;
>>>>>>    uaddr signature_bits;
>>>>>>  };
>>>>>>  signature = s;
>>>>>>  special = signature_bits & 1;
>>>>>>  signature_bits -= special;
>>>>>>  s = signature;
>>>>>> }
>>>>>>
>>>>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>>>>> following sequence
>>>>>> and add it to pre_p for it:
>>>>>>
>>>>>> tmp = lhs;
>>>>>> lvalue = tmp (+/-) rhs
>>>>>> *expr_p = tmp;
>>>>>
>>>>> As I explained this is the wrong place to fix the PR.  The issue is not
>>>>> about self-modifying expressions but about evaluating call argument
>>>>> side-effects before side-effects of the lhs.
>>>>
>>>> I am testing the attached instead.
>>>
>>> Doesn't work.  Btw, Kai, your patch surely breaks things if you put
>>> the lvalue update into the pre queue.
>>>
>>> Consider a simple
>>>
>>>  a[i++] = i;
>>>
>>> you gimplify that to
>>>
>>>  i.0 = i;
>>>  D.1709 = i.0;
>>>  i = D.1709 + 1;
>>>  a[D.1709] = i;
>>>
>>> which is wrong.
>>>
>>> Seems we are lacking some basic pre-/post-modify testcases ...
>>>
>>> Richard.
>>
>> Why, this should be wrong?  In fact C specification just says that the
>> post-inc has to happen at least before next sequence-point.  It
>> doesn't say that the increment has to happen after evaluation of rhs.
>>
>> The produced gimple for the following C-code
>>
>> int arr[128];
>>
>> void foo (int i)
>> {
>>  arr[i++] = i;
>> }
>>
>> is:
>>
>> foo (int i)
>> {
>>  int D.1364;
>>
>>  D.1364 = i;
>>  i = D.1364 + 1;
>>  arr[D.1364] = i;
>> }
>>
>> which looks to me from description of the C-specification correct.
>
> Hm, indeed.  I'll test the following shorter patch and add the struct-return
> volatile testcase.

Works apart from

Running target unix/
FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test

Maybe invalid testcases.  Who knows ... same fails happen with your patch.

Richard.

> Richard.

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

[Kai Tietz]

2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
> On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>>>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>>>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> Thanks for explaination.  I tried to flip order for lhs/rhs in
>>>>>>> gimplify_modify_expr & co.  Issue here is that for some cases we are
>>>>>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>>>>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>>>>>> like:
>>>>>>>
>>>>>>> typedef const unsigned char _Jv_Utf8Const;
>>>>>>> typedef __SIZE_TYPE__ uaddr;
>>>>>>>
>>>>>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>>>>>> {
>>>>>>>  union {
>>>>>>>    _Jv_Utf8Const *signature;
>>>>>>>    uaddr signature_bits;
>>>>>>>  };
>>>>>>>  signature = s;
>>>>>>>  special = signature_bits & 1;
>>>>>>>  signature_bits -= special;
>>>>>>>  s = signature;
>>>>>>> }
>>>>>>>
>>>>>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>>>>>> following sequence
>>>>>>> and add it to pre_p for it:
>>>>>>>
>>>>>>> tmp = lhs;
>>>>>>> lvalue = tmp (+/-) rhs
>>>>>>> *expr_p = tmp;
>>>>>>
>>>>>> As I explained this is the wrong place to fix the PR.  The issue is not
>>>>>> about self-modifying expressions but about evaluating call argument
>>>>>> side-effects before side-effects of the lhs.
>>>>>
>>>>> I am testing the attached instead.
>>>>
>>>> Doesn't work.  Btw, Kai, your patch surely breaks things if you put
>>>> the lvalue update into the pre queue.
>>>>
>>>> Consider a simple
>>>>
>>>>  a[i++] = i;
>>>>
>>>> you gimplify that to
>>>>
>>>>  i.0 = i;
>>>>  D.1709 = i.0;
>>>>  i = D.1709 + 1;
>>>>  a[D.1709] = i;
>>>>
>>>> which is wrong.
>>>>
>>>> Seems we are lacking some basic pre-/post-modify testcases ...
>>>>
>>>> Richard.
>>>
>>> Why, this should be wrong?  In fact C specification just says that the
>>> post-inc has to happen at least before next sequence-point.  It
>>> doesn't say that the increment has to happen after evaluation of rhs.
>>>
>>> The produced gimple for the following C-code
>>>
>>> int arr[128];
>>>
>>> void foo (int i)
>>> {
>>>  arr[i++] = i;
>>> }
>>>
>>> is:
>>>
>>> foo (int i)
>>> {
>>>  int D.1364;
>>>
>>>  D.1364 = i;
>>>  i = D.1364 + 1;
>>>  arr[D.1364] = i;
>>> }
>>>
>>> which looks to me from description of the C-specification correct.
>>
>> Hm, indeed.  I'll test the following shorter patch and add the struct-return
>> volatile testcase.
>
> Works apart from
>
> Running target unix/
> FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
> FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
>
> Maybe invalid testcases.  Who knows ... same fails happen with your patch.
>
> Richard.
>
>> Richard.

Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
part of this failure.

This might lead here to this failure.  I am not sure, if such
constructs having fixed behavior for C++, but it looks to me like
undefined behavior.

A C-testcase for the issue would be:

int *foo (int *p)
{
  *p++ = *p;
  return p;
}

which produces with patch now:

foo (int * p)
{
  int * D.1363;
  int D.1364;
  int * D.1365;

  D.1363 = p;
  p = D.1363 + 4;
  D.1364 = *p;
  *D.1363 = D.1364;
  D.1365 = p;
  return D.1365;
}

but in old variant we were producing:

foo (int * p)
{
  int D.1363;
  int * D.1364;

  D.1363 = *p;
  *p = D.1363;
  p = p + 4;
  D.1364 = p;
  return D.1364;
}

So, maybe the real solution for this issue might be to swap for
assignment gimplification the order for lhs/rhs gimplification
instead.


Regards,
Kai

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

[Richard Guenther]

On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>> Works apart from
>>
>> Running target unix/
>> FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
>> FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
>>
>> Maybe invalid testcases.  Who knows ... same fails happen with your patch.
>>
>> Richard.
>>
>>> Richard.
>
> Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
> use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
> part of this failure.
>
> This might lead here to this failure.  I am not sure, if such
> constructs having fixed behavior for C++, but it looks to me like
> undefined behavior.
>
> A C-testcase for the issue would be:
>
> int *foo (int *p)
> {
>  *p++ = *p;
>  return p;
> }
>
> which produces with patch now:
>
> foo (int * p)
> {
>  int * D.1363;
>  int D.1364;
>  int * D.1365;
>
>  D.1363 = p;
>  p = D.1363 + 4;
>  D.1364 = *p;
>  *D.1363 = D.1364;
>  D.1365 = p;
>  return D.1365;
> }
>
> but in old variant we were producing:
>
> foo (int * p)
> {
>  int D.1363;
>  int * D.1364;
>
>  D.1363 = *p;
>  *p = D.1363;
>  p = p + 4;
>  D.1364 = p;
>  return D.1364;
> }
>
> So, maybe the real solution for this issue might be to swap for
> assignment gimplification the order for lhs/rhs gimplification
> instead.

Well, that would certainly not be suitable for stage4 (though I have a working
patch for that as well).  The post-modify gimplification change looks better
as it also fixes the volatile aggregate-return case which would not be fixed
by re-ordering of the gimplification.

libstdc++ folks - can you investigate the testsuite failure?

The patch in question is at
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814

Note that the _Mutable_ForwardIteratorConcept isn't the problem I think,
it's not executed code.

Thanks,
Richard.

>
> Regards,
> Kai

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

[Andreas Schwab]

Kai Tietz <ktietz70@googlemail.com> writes:

> Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
> use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
> part of this failure.

I don't think those __constraints functions are ever executed.  They are
only present to assert the presense of certain operations at compile
time.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Jonathan Wakely]

On 10 February 2012 10:35, Richard Guenther wrote:
>>> Works apart from
>>>
>>> Running target unix/
>>> FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
>>> FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test

What does libstdc++.log show for those failures?

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

[Richard Guenther]

On Fri, Feb 10, 2012 at 2:12 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 10 February 2012 10:35, Richard Guenther wrote:
>>>> Works apart from
>>>>
>>>> Running target unix/
>>>> FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
>>>> FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
>
> What does libstdc++.log show for those failures?

spawn [open ...]^M
<?xml version = "1.0"?>
<test>
<sd value = "1328880244">
</sd>
<n value = "5000">
</n>
<m value = "10000">
</m>
<tp value = "0.2">
</tp>
<ip value = "0.6">
</ip>
<ep value = "0.2">
</ep>
<cp value = "0.001">
</cp>
<mp value = "0.25">
</mp>
</test>
<cntnr name = "pat_trie_map">
<desc>
<type value = "trie">
<Tag value = "pat_trie_tag">
</Tag>
<Node_Update value = "null_node_update">
</Node_Update>
</type>
</desc>
<progress>
----------------------------------------
*FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test

spawn [open ...]^M
<?xml version = "1.0"?>
<test>
<sd value = "1328880487">
</sd>
<n value = "5000">
</n>
<m value = "10000">
</m>
<tp value = "0.2">
</tp>
<ip value = "0.6">
</ip>
<ep value = "0.2">
</ep>
<cp value = "0.001">
</cp>
<mp value = "0.25">
</mp>
</test>
<cntnr name = "pat_trie_set">
<desc>
<type value = "trie">
<Tag value = "pat_trie_tag">
</Tag>
<Node_Update value = "null_node_update">
</Node_Update>
</type>
</desc>
<progress>
----------------------------------------
**FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test

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

[Kai Tietz]

2012/2/10 Richard Guenther <richard.guenther@gmail.com>:
> On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>>> Works apart from
>>>
>>> Running target unix/
>>> FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
>>> FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
>>>
>>> Maybe invalid testcases.  Who knows ... same fails happen with your patch.
>>>
>>> Richard.
>>>
>>>> Richard.
>>
>> Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
>> use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
>> part of this failure.
>>
>> This might lead here to this failure.  I am not sure, if such
>> constructs having fixed behavior for C++, but it looks to me like
>> undefined behavior.
>>
>> A C-testcase for the issue would be:
>>
>> int *foo (int *p)
>> {
>>  *p++ = *p;
>>  return p;
>> }
>>
>> which produces with patch now:
>>
>> foo (int * p)
>> {
>>  int * D.1363;
>>  int D.1364;
>>  int * D.1365;
>>
>>  D.1363 = p;
>>  p = D.1363 + 4;
>>  D.1364 = *p;
>>  *D.1363 = D.1364;
>>  D.1365 = p;
>>  return D.1365;
>> }
>>
>> but in old variant we were producing:
>>
>> foo (int * p)
>> {
>>  int D.1363;
>>  int * D.1364;
>>
>>  D.1363 = *p;
>>  *p = D.1363;
>>  p = p + 4;
>>  D.1364 = p;
>>  return D.1364;
>> }
>>
>> So, maybe the real solution for this issue might be to swap for
>> assignment gimplification the order for lhs/rhs gimplification
>> instead.
>
> Well, that would certainly not be suitable for stage4 (though I have a working
> patch for that as well).  The post-modify gimplification change looks better
> as it also fixes the volatile aggregate-return case which would not be fixed
> by re-ordering of the gimplification.
>
> libstdc++ folks - can you investigate the testsuite failure?
>
> The patch in question is at
> http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814
>
> Note that the _Mutable_ForwardIteratorConcept isn't the problem I think,
> it's not executed code.
>
> Thanks,
> Richard.

Hmm, we might need here lhs/rhs flip plus the post-modify.  At least
we would avoid by this code differences for common cases that lhs has
post-inc/dec to current behavior?

But you might be right that this patch is not suitable for stage 4.

Regards,
Kai

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

[Kai Tietz]

Richard,

ping.  I think now could be a good time for applying the patch you
have for this issue as we are in stage 1.

Regards,
Kai

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

[Richard Guenther]

On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Richard,
>
> ping.  I think now could be a good time for applying the patch you
> have for this issue as we are in stage 1.

It will still regress the two libstdc++ testcases (well, I guess so at least).

Jonathan - you didn't answer my reply to your question?  Would it be ok
to apply this patch with leaving the regressions in-place, to be investigated
by libstdc++ folks?

Thanks,
Richard.

> Regards,
> Kai

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

[Jonathan Wakely]

On 15 March 2012 15:40, Richard Guenther wrote:
> On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> Richard,
>>
>> ping.  I think now could be a good time for applying the patch you
>> have for this issue as we are in stage 1.
>
> It will still regress the two libstdc++ testcases (well, I guess so at least).
>
> Jonathan - you didn't answer my reply to your question?  Would it be ok
> to apply this patch with leaving the regressions in-place, to be investigated
> by libstdc++ folks?

Sorry, I've either forgotten or missed the reply - but if you think
the problem is in libstdc++ then certainly go ahead and apply it, I'll
investigate the libstdc++ problems (and ask for help if they defeat
me!)

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

[Richard Guenther]

On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 15 March 2012 15:40, Richard Guenther wrote:
>> On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Richard,
>>>
>>> ping.  I think now could be a good time for applying the patch you
>>> have for this issue as we are in stage 1.
>>
>> It will still regress the two libstdc++ testcases (well, I guess so at least).
>>
>> Jonathan - you didn't answer my reply to your question?  Would it be ok
>> to apply this patch with leaving the regressions in-place, to be investigated
>> by libstdc++ folks?
>
> Sorry, I've either forgotten or missed the reply - but if you think
> the problem is in libstdc++ then certainly go ahead and apply it, I'll
> investigate the libstdc++ problems (and ask for help if they defeat
> me!)

Ok.  I'll do so after re-testing the patch.

Richard.

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

[Benjamin De Kosnik]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Fri, 16 Mar 2012 11:10:48 +0100
Richard Guenther <richard.guenther@gmail.com> wrote:

> On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely
> <jwakely.gcc@gmail.com> wrote:
> > On 15 March 2012 15:40, Richard Guenther wrote:
> >> On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz
> >> <ktietz70@googlemail.com> wrote:
> >>> Richard,
> >>>
> >>> ping.  I think now could be a good time for applying the patch you
> >>> have for this issue as we are in stage 1.
> >>
> >> It will still regress the two libstdc++ testcases (well, I guess
> >> so at least).
> >>
> >> Jonathan - you didn't answer my reply to your question?  Would it
> >> be ok to apply this patch with leaving the regressions in-place,
> >> to be investigated by libstdc++ folks?
> >
> > Sorry, I've either forgotten or missed the reply - but if you think
> > the problem is in libstdc++ then certainly go ahead and apply it,
> > I'll investigate the libstdc++ problems (and ask for help if they
> > defeat me!)
> 
> Ok.  I'll do so after re-testing the patch.

FYI, here is the patch for the new libstdc++ fails.

-benjamin


[-- Attachment #2: 20120319-2.patch --]
[-- Type: text/x-patch, Size: 2274 bytes --]

2012-03-19  Benjamin Kosnik  <bkoz@redhat.com>

	    * include/ext/pb_ds/detail/pat_trie_/
	    constructors_destructor_fn_imps.hpp: Increment after recursion.
	    * include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp: Convert
	    node_type markup from brief.


diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
index 8370a2e..c5748ec 100644
--- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
+++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
@@ -1,6 +1,6 @@
  // -*- C++ -*-
 
-// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
+// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
 // Free Software Foundation, Inc.
 //
 // This file is part of the GNU ISO C++ Library.  This library is free
@@ -188,7 +188,11 @@ recursive_copy_node(node_const_pointer p_ncp)
   __try
     {
       while (child_it != p_icp->end())
-	a_p_children[child_i++] = recursive_copy_node(*(child_it++));
+	{
+	  a_p_children[child_i] = recursive_copy_node(*(child_it));
+	  child_i++;
+	  child_it++;
+	}
       p_ret = s_inode_allocator.allocate(1);
     }
   __catch(...)
diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
index b7eb024..0a763b5 100644
--- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
+++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
@@ -1,6 +1,6 @@
 // -*- C++ -*-
 
-// Copyright (C) 2005, 2006, 2009, 2011 Free Software Foundation, Inc.
+// Copyright (C) 2005, 2006, 2009, 2011, 2012 Free Software Foundation, Inc.
 //
 // This file is part of the GNU ISO C++ Library.  This library is free
 // software; you can redistribute it and/or modify it under the terms
@@ -50,7 +50,11 @@ namespace __gnu_pbds
     /// Base type for PATRICIA trees.
     struct pat_trie_base
     {
-      /// Three types of nodes.
+      /**
+       *  @brief  Three types of nodes.
+       *
+       *  i_node is used by _Inode, leaf_node by _Leaf, and head_node by _Head.
+       */
       enum node_type
 	{
 	  i_node,