[PATCH] match.pd: Replace incorrect simplifications into copysign [PR90248]

[PATCH] match.pd: Replace incorrect simplifications into copysign [PR90248]

[Jakub Jelinek]

Hi!

In the PR Andrew said he has implemented a simplification that has been
added to LLVM, but that actually is not true, what is in there are
X * (X cmp 0.0 ? +-1.0 : -+1.0) simplifications into +-abs(X)
but what has been added into GCC are (X cmp 0.0 ? +-1.0 : -+1.0)
simplifications into copysign(1, +-X) and then
X * copysign (1, +-X) into +-abs (X).
The problem is with the (X cmp 0.0 ? +-1.0 : -+1.0) simplifications,
they don't work correctly when X is zero.
E.g.
(X > 0.0 ? 1.0 : -1.0)
is -1.0 when X is either -0.0 or 0.0, but copysign will make it return
1.0 for 0.0 and -1.0 only for -0.0.
(X >= 0.0 ? 1.0 : -1.0)
is 1.0 when X is either -0.0 or 0.0, but copysign will make it return
still 1.0 for 0.0 and -1.0 for -0.0.
The simplifications were guarded on !HONOR_SIGNED_ZEROS, but as discussed in
the PR, that option doesn't mean that -0.0 will not ever appear as operand
of some operation, it is hard to guarantee that without compiler adding
canonicalizations of -0.0 to 0.0 after most of the operations and thus
making it very slow, but that the user asserts that he doesn't care if the result
of operations will be 0.0 or -0.0.  Not to mention that some of the
transformations are incorrect even for positive 0.0.

So, instead of those simplifications this patch recognizes patterns where
those ?: expressions are multiplied by X, directly into +-abs.
That works fine even for 0.0 and -0.0 (as long as we don't care about
whether the result is exactly 0.0 or -0.0 in those cases), because
whether the result of copysign is -1.0 or 1.0 doesn't matter when it is
multiplied by 0.0 or -0.0.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

As a follow-up, maybe we should add the simplification mentioned in the PR,
in particular doing copysign by hand through
VIEW_CONVERT_EXPR <int, float_X> < 0 ? -float_constant : float_constant
into copysign (float_constant, float_X).  But I think that would need to be
done in phiopt.

2021-01-22  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/90248
	* match.pd (X cmp 0.0 ? 1.0 : -1.0 -> copysign(1, +-X),
	X cmp 0.0 ? -1.0 : +1.0 -> copysign(1, -+X): Remove
	simplifications.
	(X * (X cmp 0.0 ? 1.0 : -1.0) -> +-abs(X),
	X * (X cmp 0.0 ? -1.0 : 1.0) -> +-abs(X)): New simplifications.

	* gcc.dg/tree-ssa/copy-sign-1.c: Don't expect any copysign
	builtins.
	* gcc.dg/pr90248.c: New test.

--- gcc/match.pd.jj	2021-01-20 21:48:35.868156687 +0100
+++ gcc/match.pd	2021-01-21 19:12:01.750982756 +0100
@@ -253,36 +253,22 @@ (define_operator_list COND_TERNARY
 (for cmp (gt ge lt le)
      outp (convert convert negate negate)
      outn (negate negate convert convert)
- /* Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X). */
- /* Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X). */
- /* Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
- /* Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
+ /* Transform X * (X > 0.0 ? 1.0 : -1.0) into abs(X). */
+ /* Transform X * (X >= 0.0 ? 1.0 : -1.0) into abs(X). */
+ /* Transform X * (X < 0.0 ? 1.0 : -1.0) into -abs(X). */
+ /* Transform X * (X <= 0.0 ? 1.0 : -1.0) into -abs(X). */
  (simplify
-  (cond (cmp @0 real_zerop) real_onep@1 real_minus_onep)
-  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
-       && types_match (type, TREE_TYPE (@0)))
-   (switch
-    (if (types_match (type, float_type_node))
-     (BUILT_IN_COPYSIGNF @1 (outp @0)))
-    (if (types_match (type, double_type_node))
-     (BUILT_IN_COPYSIGN @1 (outp @0)))
-    (if (types_match (type, long_double_type_node))
-     (BUILT_IN_COPYSIGNL @1 (outp @0))))))
- /* Transform (X > 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
- /* Transform (X >= 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
- /* Transform (X < 0.0 ? -1.0 : 1.0) into copysign(1,X). */
- /* Transform (X <= 0.0 ? -1.0 : 1.0) into copysign(1,X). */
+  (mult:c @0 (cond (cmp @0 real_zerop) real_onep@1 real_minus_onep))
+  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
+   (outp (abs @0))))
+ /* Transform X * (X > 0.0 ? -1.0 : 1.0) into -abs(X). */
+ /* Transform X * (X >= 0.0 ? -1.0 : 1.0) into -abs(X). */
+ /* Transform X * (X < 0.0 ? -1.0 : 1.0) into abs(X). */
+ /* Transform X * (X <= 0.0 ? -1.0 : 1.0) into abs(X). */
  (simplify
-  (cond (cmp @0 real_zerop) real_minus_onep real_onep@1)
-  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
-       && types_match (type, TREE_TYPE (@0)))
-   (switch
-    (if (types_match (type, float_type_node))
-     (BUILT_IN_COPYSIGNF @1 (outn @0)))
-    (if (types_match (type, double_type_node))
-     (BUILT_IN_COPYSIGN @1 (outn @0)))
-    (if (types_match (type, long_double_type_node))
-     (BUILT_IN_COPYSIGNL @1 (outn @0)))))))
+  (mult:c @0 (cond (cmp @0 real_zerop) real_minus_onep real_onep@1))
+  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
+   (outn (abs @0)))))
 
 /* Transform X * copysign (1.0, X) into abs(X). */
 (simplify
--- gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c.jj	2020-01-12 11:54:37.586395711 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c	2021-01-21 19:16:29.811957680 +0100
@@ -33,4 +33,4 @@ float i1(float x)
 {
   return (x <= 0.f ? 1.f : -1.f);
 }
-/* { dg-final { scan-tree-dump-times "copysign" 8 "gimple"} } */
+/* { dg-final { scan-tree-dump-not "copysign" "gimple"} } */
--- gcc/testsuite/gcc.dg/pr90248.c.jj	2021-01-21 19:35:14.576260732 +0100
+++ gcc/testsuite/gcc.dg/pr90248.c	2021-01-21 19:34:23.801837461 +0100
@@ -0,0 +1,73 @@
+/* PR tree-optimization/90248 */
+/* { dg-do run } */
+/* { dg-options "-Ofast" } */
+
+volatile float b1 = -1.f;
+volatile float b2 = 0.f;
+
+__attribute__((noipa)) float
+f1 (float x)
+{
+  return x > 0 ? 1.f : -1.f;
+}
+
+__attribute__((noipa)) float
+f2 (float x)
+{
+  return x >= 0 ? 1.f : -1.f;
+}
+
+__attribute__((noipa)) float
+f3 (float x)
+{
+  return x < 0 ? 1.f : -1.f;
+}
+
+__attribute__((noipa)) float
+f4 (float x)
+{
+  return x <= 0 ? 1.f : -1.f;
+}
+
+__attribute__((noipa)) float
+f5 (float x)
+{
+  return x > 0 ? -1.f : 1.f;
+}
+
+__attribute__((noipa)) float
+f6 (float x)
+{
+  return x >= 0 ? -1.f : 1.f;
+}
+
+__attribute__((noipa)) float
+f7 (float x)
+{
+  return x < 0 ? -1.f : 1.f;
+}
+
+__attribute__((noipa)) float
+f8 (float x)
+{
+  return x <= 0 ? -1.f : 1.f;
+}
+
+int
+main ()
+{
+  float a = 0.f;
+  float b = b1 * b2;
+  float c = 2.f;
+  float d = -2.f;
+  if (f1 (a) != -1.f || f1 (b) != -1.f || f1 (c) != 1.f || f1 (d) != -1.f
+      || f2 (a) != 1.f || f2 (b) != 1.f || f2 (c) != 1.f || f2 (d) != -1.f
+      || f3 (a) != -1.f || f3 (b) != -1.f || f3 (c) != -1.f || f3 (d) != 1.f
+      || f4 (a) != 1.f || f4 (b) != 1.f || f4 (c) != -1.f || f4 (d) != 1.f
+      || f5 (a) != 1.f || f5 (b) != 1.f || f5 (c) != -1.f || f5 (d) != 1.f
+      || f6 (a) != -1.f || f6 (b) != -1.f || f6 (c) != -1.f || f6 (d) != 1.f
+      || f7 (a) != 1.f || f7 (b) != 1.f || f7 (c) != 1.f || f7 (d) != -1.f
+      || f8 (a) != -1.f || f8 (b) != -1.f || f8 (c) != 1.f || f8 (d) != -1.f)
+    __builtin_abort ();
+  return 0;
+}

	Jakub

Re: [PATCH] match.pd: Replace incorrect simplifications into copysign [PR90248]

[Richard Biener]

On Fri, 22 Jan 2021, Jakub Jelinek wrote:

> Hi!
> 
> In the PR Andrew said he has implemented a simplification that has been
> added to LLVM, but that actually is not true, what is in there are
> X * (X cmp 0.0 ? +-1.0 : -+1.0) simplifications into +-abs(X)
> but what has been added into GCC are (X cmp 0.0 ? +-1.0 : -+1.0)
> simplifications into copysign(1, +-X) and then
> X * copysign (1, +-X) into +-abs (X).
> The problem is with the (X cmp 0.0 ? +-1.0 : -+1.0) simplifications,
> they don't work correctly when X is zero.
> E.g.
> (X > 0.0 ? 1.0 : -1.0)
> is -1.0 when X is either -0.0 or 0.0, but copysign will make it return
> 1.0 for 0.0 and -1.0 only for -0.0.
> (X >= 0.0 ? 1.0 : -1.0)
> is 1.0 when X is either -0.0 or 0.0, but copysign will make it return
> still 1.0 for 0.0 and -1.0 for -0.0.
> The simplifications were guarded on !HONOR_SIGNED_ZEROS, but as discussed in
> the PR, that option doesn't mean that -0.0 will not ever appear as operand
> of some operation, it is hard to guarantee that without compiler adding
> canonicalizations of -0.0 to 0.0 after most of the operations and thus
> making it very slow, but that the user asserts that he doesn't care if the result
> of operations will be 0.0 or -0.0.  Not to mention that some of the
> transformations are incorrect even for positive 0.0.
> 
> So, instead of those simplifications this patch recognizes patterns where
> those ?: expressions are multiplied by X, directly into +-abs.
> That works fine even for 0.0 and -0.0 (as long as we don't care about
> whether the result is exactly 0.0 or -0.0 in those cases), because
> whether the result of copysign is -1.0 or 1.0 doesn't matter when it is
> multiplied by 0.0 or -0.0.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> As a follow-up, maybe we should add the simplification mentioned in the PR,
> in particular doing copysign by hand through
> VIEW_CONVERT_EXPR <int, float_X> < 0 ? -float_constant : float_constant
> into copysign (float_constant, float_X).  But I think that would need to be
> done in phiopt.
> 
> 2021-01-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/90248
> 	* match.pd (X cmp 0.0 ? 1.0 : -1.0 -> copysign(1, +-X),
> 	X cmp 0.0 ? -1.0 : +1.0 -> copysign(1, -+X): Remove
> 	simplifications.
> 	(X * (X cmp 0.0 ? 1.0 : -1.0) -> +-abs(X),
> 	X * (X cmp 0.0 ? -1.0 : 1.0) -> +-abs(X)): New simplifications.
> 
> 	* gcc.dg/tree-ssa/copy-sign-1.c: Don't expect any copysign
> 	builtins.
> 	* gcc.dg/pr90248.c: New test.
> 
> --- gcc/match.pd.jj	2021-01-20 21:48:35.868156687 +0100
> +++ gcc/match.pd	2021-01-21 19:12:01.750982756 +0100
> @@ -253,36 +253,22 @@ (define_operator_list COND_TERNARY
>  (for cmp (gt ge lt le)
>       outp (convert convert negate negate)
>       outn (negate negate convert convert)
> - /* Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> - /* Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> - /* Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> - /* Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> + /* Transform X * (X > 0.0 ? 1.0 : -1.0) into abs(X). */
> + /* Transform X * (X >= 0.0 ? 1.0 : -1.0) into abs(X). */
> + /* Transform X * (X < 0.0 ? 1.0 : -1.0) into -abs(X). */
> + /* Transform X * (X <= 0.0 ? 1.0 : -1.0) into -abs(X). */
>   (simplify
> -  (cond (cmp @0 real_zerop) real_onep@1 real_minus_onep)
> -  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
> -       && types_match (type, TREE_TYPE (@0)))
> -   (switch
> -    (if (types_match (type, float_type_node))
> -     (BUILT_IN_COPYSIGNF @1 (outp @0)))
> -    (if (types_match (type, double_type_node))
> -     (BUILT_IN_COPYSIGN @1 (outp @0)))
> -    (if (types_match (type, long_double_type_node))
> -     (BUILT_IN_COPYSIGNL @1 (outp @0))))))
> - /* Transform (X > 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
> - /* Transform (X >= 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
> - /* Transform (X < 0.0 ? -1.0 : 1.0) into copysign(1,X). */
> - /* Transform (X <= 0.0 ? -1.0 : 1.0) into copysign(1,X). */
> +  (mult:c @0 (cond (cmp @0 real_zerop) real_onep@1 real_minus_onep))
> +  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
> +   (outp (abs @0))))
> + /* Transform X * (X > 0.0 ? -1.0 : 1.0) into -abs(X). */
> + /* Transform X * (X >= 0.0 ? -1.0 : 1.0) into -abs(X). */
> + /* Transform X * (X < 0.0 ? -1.0 : 1.0) into abs(X). */
> + /* Transform X * (X <= 0.0 ? -1.0 : 1.0) into abs(X). */
>   (simplify
> -  (cond (cmp @0 real_zerop) real_minus_onep real_onep@1)
> -  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
> -       && types_match (type, TREE_TYPE (@0)))
> -   (switch
> -    (if (types_match (type, float_type_node))
> -     (BUILT_IN_COPYSIGNF @1 (outn @0)))
> -    (if (types_match (type, double_type_node))
> -     (BUILT_IN_COPYSIGN @1 (outn @0)))
> -    (if (types_match (type, long_double_type_node))
> -     (BUILT_IN_COPYSIGNL @1 (outn @0)))))))
> +  (mult:c @0 (cond (cmp @0 real_zerop) real_minus_onep real_onep@1))
> +  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
> +   (outn (abs @0)))))
>  
>  /* Transform X * copysign (1.0, X) into abs(X). */
>  (simplify
> --- gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c.jj	2020-01-12 11:54:37.586395711 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c	2021-01-21 19:16:29.811957680 +0100
> @@ -33,4 +33,4 @@ float i1(float x)
>  {
>    return (x <= 0.f ? 1.f : -1.f);
>  }
> -/* { dg-final { scan-tree-dump-times "copysign" 8 "gimple"} } */
> +/* { dg-final { scan-tree-dump-not "copysign" "gimple"} } */
> --- gcc/testsuite/gcc.dg/pr90248.c.jj	2021-01-21 19:35:14.576260732 +0100
> +++ gcc/testsuite/gcc.dg/pr90248.c	2021-01-21 19:34:23.801837461 +0100
> @@ -0,0 +1,73 @@
> +/* PR tree-optimization/90248 */
> +/* { dg-do run } */
> +/* { dg-options "-Ofast" } */
> +
> +volatile float b1 = -1.f;
> +volatile float b2 = 0.f;
> +
> +__attribute__((noipa)) float
> +f1 (float x)
> +{
> +  return x > 0 ? 1.f : -1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f2 (float x)
> +{
> +  return x >= 0 ? 1.f : -1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f3 (float x)
> +{
> +  return x < 0 ? 1.f : -1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f4 (float x)
> +{
> +  return x <= 0 ? 1.f : -1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f5 (float x)
> +{
> +  return x > 0 ? -1.f : 1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f6 (float x)
> +{
> +  return x >= 0 ? -1.f : 1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f7 (float x)
> +{
> +  return x < 0 ? -1.f : 1.f;
> +}
> +
> +__attribute__((noipa)) float
> +f8 (float x)
> +{
> +  return x <= 0 ? -1.f : 1.f;
> +}
> +
> +int
> +main ()
> +{
> +  float a = 0.f;
> +  float b = b1 * b2;
> +  float c = 2.f;
> +  float d = -2.f;
> +  if (f1 (a) != -1.f || f1 (b) != -1.f || f1 (c) != 1.f || f1 (d) != -1.f
> +      || f2 (a) != 1.f || f2 (b) != 1.f || f2 (c) != 1.f || f2 (d) != -1.f
> +      || f3 (a) != -1.f || f3 (b) != -1.f || f3 (c) != -1.f || f3 (d) != 1.f
> +      || f4 (a) != 1.f || f4 (b) != 1.f || f4 (c) != -1.f || f4 (d) != 1.f
> +      || f5 (a) != 1.f || f5 (b) != 1.f || f5 (c) != -1.f || f5 (d) != 1.f
> +      || f6 (a) != -1.f || f6 (b) != -1.f || f6 (c) != -1.f || f6 (d) != 1.f
> +      || f7 (a) != 1.f || f7 (b) != 1.f || f7 (c) != 1.f || f7 (d) != -1.f
> +      || f8 (a) != -1.f || f8 (b) != -1.f || f8 (c) != 1.f || f8 (d) != -1.f)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)