[PATCH] Fix PR80281

[PATCH] Fix PR80281

[Richard Biener]

The following fixes PR80281.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Richard.

2017-04-03  Richard Biener  <rguenther@suse.de>

	PR middle-end/80281
	* match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
	arithmetic done for the negate or the plus.
	* fold-const.c (split_tree): Make sure to not negate pointers.

	* gcc.dg/torture/pr80281.c: New testcase.

Index: gcc/match.pd
===================================================================
--- gcc/match.pd	(revision 246642)
+++ gcc/match.pd	(working copy)
@@ -1153,7 +1153,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
        && tree_nop_conversion_p (type, TREE_TYPE (@1))
        && !TYPE_OVERFLOW_SANITIZED (type))
-   (minus (convert @0) (convert @1))))
+   (with
+    {
+     tree t1 = type;
+     if (INTEGRAL_TYPE_P (type)
+	 && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
+       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
+    }
+    (convert (minus (convert:t1 @0) (convert:t1 @1))))))
  /* A - (-B) -> A + B */
  (simplify
   (minus (convert1? @0) (convert2? (negate @1)))
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 246642)
+++ gcc/fold-const.c	(working copy)
@@ -831,8 +831,12 @@ split_tree (location_t loc, tree in, tre
       /* Now do any needed negations.  */
       if (neg_litp_p)
 	*minus_litp = *litp, *litp = 0;
-      if (neg_conp_p)
-	*conp = negate_expr (*conp);
+      if (neg_conp_p && *conp)
+	{
+	  /* Convert to TYPE before negating.  */
+	  *conp = fold_convert_loc (loc, type, *conp);
+	  *conp = negate_expr (*conp);
+	}
       if (neg_var_p && var)
 	{
 	  /* Convert to TYPE before negating.  */
@@ -859,7 +863,12 @@ split_tree (location_t loc, tree in, tre
 	*minus_litp = *litp, *litp = 0;
       else if (*minus_litp)
 	*litp = *minus_litp, *minus_litp = 0;
-      *conp = negate_expr (*conp);
+      if (*conp)
+	{
+	  /* Convert to TYPE before negating.  */
+	  *conp = fold_convert_loc (loc, type, *conp);
+	  *conp = negate_expr (*conp);
+	}
       if (var)
 	{
 	  /* Convert to TYPE before negating.  */
Index: gcc/testsuite/gcc.dg/torture/pr80281.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr80281.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr80281.c	(working copy)
@@ -0,0 +1,14 @@
+/* { dg-run } */
+/* { dg-require-effective-target int32plus } */
+
+int
+main ()
+{
+  volatile int a = 0;
+  long long b = 2147483648LL;
+  int c = a % 2;
+  int x = ((int) -b + c) % -2147483647;
+  if (x != -1)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] Fix PR80281

[Marc Glisse]

On Mon, 3 Apr 2017, Richard Biener wrote:

> --- gcc/match.pd	(revision 246642)
> +++ gcc/match.pd	(working copy)
> @@ -1153,7 +1153,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>   (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
>        && tree_nop_conversion_p (type, TREE_TYPE (@1))
>        && !TYPE_OVERFLOW_SANITIZED (type))
> -   (minus (convert @0) (convert @1))))
> +   (with
> +    {
> +     tree t1 = type;
> +     if (INTEGRAL_TYPE_P (type)
> +	 && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
> +       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
> +    }
> +    (convert (minus (convert:t1 @0) (convert:t1 @1))))))

Why do we match
(plus:c (convert1? @0) (convert2? (negate @1)))
instead of the simpler
(plus:c @0 (convert? (negate @1)))

>  /* A - (-B) -> A + B */

Actually, I believe this transformation has issues as well, it seems 
possible to transform X-INT_MIN into X+INT_MIN if INT_MIN comes as 
(int)(-(unsigned)INT_MIN).

-- 
Marc Glisse

Re: [PATCH] Fix PR80281

[Richard Biener]

On Mon, 3 Apr 2017, Marc Glisse wrote:

> On Mon, 3 Apr 2017, Richard Biener wrote:
> 
> > --- gcc/match.pd	(revision 246642)
> > +++ gcc/match.pd	(working copy)
> > @@ -1153,7 +1153,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >   (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
> >        && tree_nop_conversion_p (type, TREE_TYPE (@1))
> >        && !TYPE_OVERFLOW_SANITIZED (type))
> > -   (minus (convert @0) (convert @1))))
> > +   (with
> > +    {
> > +     tree t1 = type;
> > +     if (INTEGRAL_TYPE_P (type)
> > +	 && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE
> > (@1)))
> > +       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
> > +    }
> > +    (convert (minus (convert:t1 @0) (convert:t1 @1))))))
> 
> Why do we match
> (plus:c (convert1? @0) (convert2? (negate @1)))
> instead of the simpler
> (plus:c @0 (convert? (negate @1)))

Hmm, historic I believe - matching forwprop code 1:1.  The convert on 
@0 is not needed indeed.

> >  /* A - (-B) -> A + B */
> 
> Actually, I believe this transformation has issues as well, it seems possible
> to transform X-INT_MIN into X+INT_MIN if INT_MIN comes as
> (int)(-(unsigned)INT_MIN).

True.  So the same fix applies to this variant.

I'm re-testing the following variant.

Richard.

2017-04-03  Richard Biener  <rguenther@suse.de>

	PR middle-end/80281
	* match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
	arithmetic done for the negate or the plus.  Simplify.
	(A - (-B) -> A + B): Likewise.
	* fold-const.c (split_tree): Make sure to not negate pointers.

	* gcc.dg/torture/pr80281.c: New testcase.

Index: gcc/match.pd
===================================================================
--- gcc/match.pd	(revision 246648)
+++ gcc/match.pd	(working copy)
@@ -1148,19 +1148,31 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  /* Contract negates.  */
  /* A + (-B) -> A - B */
  (simplify
-  (plus:c (convert1? @0) (convert2? (negate @1)))
-  /* Apply STRIP_NOPS on @0 and the negate.  */
-  (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
-       && tree_nop_conversion_p (type, TREE_TYPE (@1))
+  (plus:c @0 (convert? (negate @1)))
+  /* Apply STRIP_NOPS on the negate.  */
+  (if (tree_nop_conversion_p (type, TREE_TYPE (@1))
        && !TYPE_OVERFLOW_SANITIZED (type))
-   (minus (convert @0) (convert @1))))
+   (with
+    {
+     tree t1 = type;
+     if (INTEGRAL_TYPE_P (type)
+	 && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
+       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
+    }
+    (convert (minus (convert:t1 @0) (convert:t1 @1))))))
  /* A - (-B) -> A + B */
  (simplify
-  (minus (convert1? @0) (convert2? (negate @1)))
-  (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
-       && tree_nop_conversion_p (type, TREE_TYPE (@1))
+  (minus @0 (convert? (negate @1)))
+  (if (tree_nop_conversion_p (type, TREE_TYPE (@1))
        && !TYPE_OVERFLOW_SANITIZED (type))
-   (plus (convert @0) (convert @1))))
+   (with
+    {
+     tree t1 = type;
+     if (INTEGRAL_TYPE_P (type)
+	 && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
+       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
+    }
+    (convert (plus (convert:t1 @0) (convert:t1 @1))))))
  /* -(-A) -> A */
  (simplify
   (negate (convert? (negate @1)))
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 246648)
+++ gcc/fold-const.c	(working copy)
@@ -831,8 +831,12 @@ split_tree (location_t loc, tree in, tre
       /* Now do any needed negations.  */
       if (neg_litp_p)
 	*minus_litp = *litp, *litp = 0;
-      if (neg_conp_p)
-	*conp = negate_expr (*conp);
+      if (neg_conp_p && *conp)
+	{
+	  /* Convert to TYPE before negating.  */
+	  *conp = fold_convert_loc (loc, type, *conp);
+	  *conp = negate_expr (*conp);
+	}
       if (neg_var_p && var)
 	{
 	  /* Convert to TYPE before negating.  */
@@ -859,7 +863,12 @@ split_tree (location_t loc, tree in, tre
 	*minus_litp = *litp, *litp = 0;
       else if (*minus_litp)
 	*litp = *minus_litp, *minus_litp = 0;
-      *conp = negate_expr (*conp);
+      if (*conp)
+	{
+	  /* Convert to TYPE before negating.  */
+	  *conp = fold_convert_loc (loc, type, *conp);
+	  *conp = negate_expr (*conp);
+	}
       if (var)
 	{
 	  /* Convert to TYPE before negating.  */
Index: gcc/testsuite/gcc.dg/torture/pr80281.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr80281.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr80281.c	(working copy)
@@ -0,0 +1,14 @@
+/* { dg-run } */
+/* { dg-require-effective-target int32plus } */
+
+int
+main ()
+{
+  volatile int a = 0;
+  long long b = 2147483648LL;
+  int c = a % 2;
+  int x = ((int) -b + c) % -2147483647;
+  if (x != -1)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] Fix PR80281

[Markus Trippelsdorf]

On 2017.04.03 at 15:20 +0200, Richard Biener wrote:
> I'm re-testing the following variant.
> 
> Richard.
> 
> 2017-04-03  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/80281
> 	* match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
> 	arithmetic done for the negate or the plus.  Simplify.
> 	(A - (-B) -> A + B): Likewise.
> 	* fold-const.c (split_tree): Make sure to not negate pointers.
> 
> 	* gcc.dg/torture/pr80281.c: New testcase.

gcc.dg/tree-ssa/pr40921.c started to fail with -march=skylake:

 % gcc -march=skylake -c -O2 -fdump-tree-optimized -ffast-math -c gcc.dg/tree-ssa/pr40921.c
 % cat pr40921.i.227t.optimized | grep "\-y"
   _3 = -y_4(D);

-- 
Markus

Re: [PATCH] Fix PR80281

[Bin.Cheng]

On Wed, Apr 5, 2017 at 12:38 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2017.04.03 at 15:20 +0200, Richard Biener wrote:
>> I'm re-testing the following variant.
>>
>> Richard.
>>
>> 2017-04-03  Richard Biener  <rguenther@suse.de>
>>
>>       PR middle-end/80281
>>       * match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
>>       arithmetic done for the negate or the plus.  Simplify.
>>       (A - (-B) -> A + B): Likewise.
>>       * fold-const.c (split_tree): Make sure to not negate pointers.
>>
>>       * gcc.dg/torture/pr80281.c: New testcase.
>
> gcc.dg/tree-ssa/pr40921.c started to fail with -march=skylake:
>
>  % gcc -march=skylake -c -O2 -fdump-tree-optimized -ffast-math -c gcc.dg/tree-ssa/pr40921.c
>  % cat pr40921.i.227t.optimized | grep "\-y"
>    _3 = -y_4(D);
Also on AArch64.

Thanks,
bin
>
> --
> Markus

Re: [PATCH] Fix PR80281

[Christophe Lyon]

On 5 April 2017 at 13:41, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Apr 5, 2017 at 12:38 PM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
>> On 2017.04.03 at 15:20 +0200, Richard Biener wrote:
>>> I'm re-testing the following variant.
>>>
>>> Richard.
>>>
>>> 2017-04-03  Richard Biener  <rguenther@suse.de>
>>>
>>>       PR middle-end/80281
>>>       * match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
>>>       arithmetic done for the negate or the plus.  Simplify.
>>>       (A - (-B) -> A + B): Likewise.
>>>       * fold-const.c (split_tree): Make sure to not negate pointers.
>>>
>>>       * gcc.dg/torture/pr80281.c: New testcase.
>>
>> gcc.dg/tree-ssa/pr40921.c started to fail with -march=skylake:
>>
>>  % gcc -march=skylake -c -O2 -fdump-tree-optimized -ffast-math -c gcc.dg/tree-ssa/pr40921.c
>>  % cat pr40921.i.227t.optimized | grep "\-y"
>>    _3 = -y_4(D);
> Also on AArch64.
>

And on some arm configurations, if that's easier to reproduce:
* -mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
* --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
* --with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8

Thanks,

Christophe

> Thanks,
> bin
>>
>> --
>> Markus

Re: [PATCH] Fix PR80281

[Richard Biener]

On Wed, 5 Apr 2017, Christophe Lyon wrote:

> On 5 April 2017 at 13:41, Bin.Cheng <amker.cheng@gmail.com> wrote:
> > On Wed, Apr 5, 2017 at 12:38 PM, Markus Trippelsdorf
> > <markus@trippelsdorf.de> wrote:
> >> On 2017.04.03 at 15:20 +0200, Richard Biener wrote:
> >>> I'm re-testing the following variant.
> >>>
> >>> Richard.
> >>>
> >>> 2017-04-03  Richard Biener  <rguenther@suse.de>
> >>>
> >>>       PR middle-end/80281
> >>>       * match.pd (A + (-B) -> A - B): Make sure to preserve unsigned
> >>>       arithmetic done for the negate or the plus.  Simplify.
> >>>       (A - (-B) -> A + B): Likewise.
> >>>       * fold-const.c (split_tree): Make sure to not negate pointers.
> >>>
> >>>       * gcc.dg/torture/pr80281.c: New testcase.
> >>
> >> gcc.dg/tree-ssa/pr40921.c started to fail with -march=skylake:
> >>
> >>  % gcc -march=skylake -c -O2 -fdump-tree-optimized -ffast-math -c gcc.dg/tree-ssa/pr40921.c
> >>  % cat pr40921.i.227t.optimized | grep "\-y"
> >>    _3 = -y_4(D);
> > Also on AArch64.
> >
> 
> And on some arm configurations, if that's easier to reproduce:
> * -mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
> * --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
> * --with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8

These are all spurious -- when you allow FMAs to be detected there'll
be an unary minus but which SSA name is negated depends on SSA name
allocation.

It's somewhat hard to fortify the testcase against the FMA case so
the following simply turns off FMA detection.

Tested on x86_64-unknown-linux-gnu, applied.

Richard.

2017-04-06  Richard Biener  <rguenther@suse.de>

	PR middle-end/80281
	* gcc.dg/tree-ssa/pr40921.c: Add -fp-contract=off.

Index: gcc/testsuite/gcc.dg/tree-ssa/pr40921.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr40921.c	(revision 246725)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr40921.c	(working copy)
@@ -1,26 +1,24 @@
-
 /* { dg-do compile } */
-/* { dg-options "-O2  -fdump-tree-optimized -ffast-math" } */
+/* { dg-options "-O2  -fdump-tree-optimized -ffast-math -ffp-contract=off" } */
 
 unsigned int foo (unsigned int x, unsigned int y, unsigned int z)
 {
-      return x + (-y * z * z);
+  return x + (-y * z * z);
 }
 
 float bar (float x, float y, float z)
 {
-      return x + (-y * z * z);
+  return x + (-y * z * z);
 }
 
 float bar2 (float x, float y, float z)
 {
-      return x + (-y * z * z * 5.0f);
+  return x + (-y * z * z * 5.0f);
 }
 
 float bar3 (float x, float y, float z)
 {
-      return x + (-y * x * -z);
+  return x + (-y * x * -z);
 }
 
-
 /* { dg-final { scan-tree-dump-times "_* = -y_" 0 "optimized" } } */