[PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]

[PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]

[Jakub Jelinek]

Hi!

The following testcase ICEs since the sincos and vect pass order has
been swapped.  It is not valid to replace vector sqrt (sqrt (x)) with
pow (x, 0.25) because build_real on vector type is invalid (could be
handled by using build_uniform_cst and adjusting type passed to
build_real) but more importantly because nothing checks if we can
actually do vector pow.
While we have pow_optab, apparently no target defines it, so it doesn't
seem to be worth bothering with for now and the patch just punts on
non-scalar sqrts.
I think the other simplifications next to it are fine, as they mostly
use CBRT which doesn't even have internal function (so is a builtin
only and therefore always scalar), or have already pow in the IL (which
doesn't have optab and shouldn't be thus vector either).
It is true that with <bits/math-vector.h> we do vectorize some calls to
pow or cbrt (but don't handle others strangely), but those aren't using
internal functions but simd clones and so match.pd doesn't know anything
about those (at least for now).

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

2023-03-28  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/109301
	* match.pd (sqrt (sqrt (x)) -> pow (x, 0.25)): Only simplify for
	scalar floating point types.

	* gcc.dg/pr109301.c: New test.

--- gcc/match.pd.jj	2023-03-27 10:25:40.171676370 +0200
+++ gcc/match.pd	2023-03-27 21:03:38.816171522 +0200
@@ -6820,7 +6820,8 @@ (define_operator_list SYNC_FETCH_AND_AND
   /* sqrt(sqrt(x)) -> pow(x,1/4).  */
   (simplify
    (sqrts (sqrts @0))
-   (pows @0 { build_real (type, dconst_quarter ()); }))
+   (if (SCALAR_FLOAT_TYPE_P (type))
+    (pows @0 { build_real (type, dconst_quarter ()); })))
   /* sqrt(cbrt(x)) -> pow(x,1/6).  */
   (simplify
    (sqrts (cbrts @0))
--- gcc/testsuite/gcc.dg/pr109301.c.jj	2023-03-27 21:06:22.635806234 +0200
+++ gcc/testsuite/gcc.dg/pr109301.c	2023-03-27 21:06:05.413054906 +0200
@@ -0,0 +1,13 @@
+/* PR tree-optimization/109301 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -ffast-math" } */
+
+double x[256];
+
+void
+foo (void)
+{
+  for (int i = 0; i < 256; ++i)
+    for (int j = 0; j < 8; ++j)
+      x[i] = __builtin_pow (x[i], 0.5);
+}

	Jakub

Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]

[Richard Biener]

On Tue, 28 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase ICEs since the sincos and vect pass order has
> been swapped.  It is not valid to replace vector sqrt (sqrt (x)) with
> pow (x, 0.25) because build_real on vector type is invalid (could be
> handled by using build_uniform_cst and adjusting type passed to
> build_real) but more importantly because nothing checks if we can
> actually do vector pow.
> While we have pow_optab, apparently no target defines it, so it doesn't
> seem to be worth bothering with for now and the patch just punts on
> non-scalar sqrts.
> I think the other simplifications next to it are fine, as they mostly
> use CBRT which doesn't even have internal function (so is a builtin
> only and therefore always scalar), or have already pow in the IL (which
> doesn't have optab and shouldn't be thus vector either).
> It is true that with <bits/math-vector.h> we do vectorize some calls to
> pow or cbrt (but don't handle others strangely), but those aren't using
> internal functions but simd clones and so match.pd doesn't know anything
> about those (at least for now).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Hmm, but canonicalize_math_p () should be false after vectorization?

When we moved the pass we should have made sure to put the
PROP_gimple_opt_math property set to pass_expand_powcabs instead.

Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is
probably invalid anyway, not sure if we can add a user-written
vector sqrt testcase that would trigger during the canonicalization
phase.  There are other uses of build_real that have the same
issue - what's your conclusion this is never a problem there?

Thanks,
Richard.

> 2023-03-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/109301
> 	* match.pd (sqrt (sqrt (x)) -> pow (x, 0.25)): Only simplify for
> 	scalar floating point types.
> 
> 	* gcc.dg/pr109301.c: New test.
> 
> --- gcc/match.pd.jj	2023-03-27 10:25:40.171676370 +0200
> +++ gcc/match.pd	2023-03-27 21:03:38.816171522 +0200
> @@ -6820,7 +6820,8 @@ (define_operator_list SYNC_FETCH_AND_AND
>    /* sqrt(sqrt(x)) -> pow(x,1/4).  */
>    (simplify
>     (sqrts (sqrts @0))
> -   (pows @0 { build_real (type, dconst_quarter ()); }))
> +   (if (SCALAR_FLOAT_TYPE_P (type))
> +    (pows @0 { build_real (type, dconst_quarter ()); })))
>    /* sqrt(cbrt(x)) -> pow(x,1/6).  */
>    (simplify
>     (sqrts (cbrts @0))
> --- gcc/testsuite/gcc.dg/pr109301.c.jj	2023-03-27 21:06:22.635806234 +0200
> +++ gcc/testsuite/gcc.dg/pr109301.c	2023-03-27 21:06:05.413054906 +0200
> @@ -0,0 +1,13 @@
> +/* PR tree-optimization/109301 */
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -ffast-math" } */
> +
> +double x[256];
> +
> +void
> +foo (void)
> +{
> +  for (int i = 0; i < 256; ++i)
> +    for (int j = 0; j < 8; ++j)
> +      x[i] = __builtin_pow (x[i], 0.5);
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]

[Jakub Jelinek]

On Tue, Mar 28, 2023 at 08:57:12AM +0000, Richard Biener wrote:
> Hmm, but canonicalize_math_p () should be false after vectorization?
> 
> When we moved the pass we should have made sure to put the
> PROP_gimple_opt_math property set to pass_expand_powcabs instead.

Which pass is the one that actually canonicalizes the math such
that we want to keep its choices for later?
I must say I don't know the details why the sincos path has been
even moved.

> Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is
> probably invalid anyway, not sure if we can add a user-written
> vector sqrt testcase that would trigger during the canonicalization

How do we write user written vector sqrt?

> phase.  There are other uses of build_real that have the same
> issue - what's your conclusion this is never a problem there?

Looking through build_real* calls in match.pd, others are
either on simplifications of some expressions with REAL_CST operand
(so can't be vector then), or using
LOG*/EXP*/CBRT*/SIN*/ATAN*/COS*/POW*/CABS*/HYPOT*/POWI*/SIGNBIT*
calls in the expression being simplified, or this case.
I think no target provides vector optabs for those or they don't
have internal fns at all.
Maybe
(simplify
 /* signbit(x) -> x<0 if x doesn't have signed zeros.  */
 (SIGNBIT @0)
 (if (!HONOR_SIGNED_ZEROS (@0))
  (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); }))))
?

	Jakub

Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301]

[Richard Biener]

On Tue, 28 Mar 2023, Jakub Jelinek wrote:

> On Tue, Mar 28, 2023 at 08:57:12AM +0000, Richard Biener wrote:
> > Hmm, but canonicalize_math_p () should be false after vectorization?
> > 
> > When we moved the pass we should have made sure to put the
> > PROP_gimple_opt_math property set to pass_expand_powcabs instead.
> 
> Which pass is the one that actually canonicalizes the math such
> that we want to keep its choices for later?
> I must say I don't know the details why the sincos path has been
> even moved.

The pass was split into sincos + pass_expand_powcabs - canonicalization
happens through folding and pass_expand_powcabs expands
pow (x, 2) to x * x.  Folding would make x * x to pow (x, 2)
so it's important to set the property after pass_expand_powcabs.

I think we moved sincos because vectorizing cexpi never materialized(?)
but maybe I misremember.

> > Now, the sqrt (sqrt ()) canonicalization to pow (.., 1./4) is
> > probably invalid anyway, not sure if we can add a user-written
> > vector sqrt testcase that would trigger during the canonicalization
> 
> How do we write user written vector sqrt?

I'm not sure ;)

> > phase.  There are other uses of build_real that have the same
> > issue - what's your conclusion this is never a problem there?
> 
> Looking through build_real* calls in match.pd, others are
> either on simplifications of some expressions with REAL_CST operand
> (so can't be vector then), or using
> LOG*/EXP*/CBRT*/SIN*/ATAN*/COS*/POW*/CABS*/HYPOT*/POWI*/SIGNBIT*
> calls in the expression being simplified, or this case.
> I think no target provides vector optabs for those or they don't
> have internal fns at all.
> Maybe
> (simplify
>  /* signbit(x) -> x<0 if x doesn't have signed zeros.  */
>  (SIGNBIT @0)
>  (if (!HONOR_SIGNED_ZEROS (@0))
>   (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); }))))
> ?

Maybe.

But as said, the fix is probably to move the pass property.

Richard.

[PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301]

[Jakub Jelinek]

Hi!

On Tue, Mar 28, 2023 at 09:23:23AM +0000, Richard Biener wrote:
> But as said, the fix is probably to move the pass property.

After looking into it in more detail I agree with you.
powcabs is a pass in the spot sincos was happening before, so the
only change was defer the sin+cos simplification into cexpi to a later
new pass (except for the name moving with it) and none of the
canonicalize_math_p () guarded simplification in match.pd seem to rely
on those sin+cos -> cexpi simplifications and canonicalize_math_p is
the only user of this property.

The following patch fixes the testcase, ok for trunk if it passes
bootstrap/regtest?

Or should I add the match.pd change as well?

2023-03-28  Jakub Jelinek  <jakub@redhat.com>
	    Richard Biener  <rguenther@suse.de>

	PR tree-optimization/109301
	* tree-ssa-math-opts.cc (pass_data_cse_sincos): Change
	properties_provided from PROP_gimple_opt_math to 0.
	(pass_data_expand_powcabs): Change properties_provided from 0 to
	PROP_gimple_opt_math.

	* gcc.dg/pr109301.c: New test.

--- gcc/tree-ssa-math-opts.cc.jj	2023-03-12 22:36:06.355178083 +0100
+++ gcc/tree-ssa-math-opts.cc	2023-03-28 11:57:36.576699748 +0200
@@ -2237,7 +2237,7 @@ const pass_data pass_data_cse_sincos =
   OPTGROUP_NONE, /* optinfo_flags */
   TV_TREE_SINCOS, /* tv_id */
   PROP_ssa, /* properties_required */
-  PROP_gimple_opt_math, /* properties_provided */
+  0, /* properties_provided */
   0, /* properties_destroyed */
   0, /* todo_flags_start */
   TODO_update_ssa, /* todo_flags_finish */
@@ -2331,7 +2331,7 @@ const pass_data pass_data_expand_powcabs
   OPTGROUP_NONE, /* optinfo_flags */
   TV_TREE_POWCABS, /* tv_id */
   PROP_ssa, /* properties_required */
-  0, /* properties_provided */
+  PROP_gimple_opt_math, /* properties_provided */
   0, /* properties_destroyed */
   0, /* todo_flags_start */
   TODO_update_ssa, /* todo_flags_finish */
--- gcc/testsuite/gcc.dg/pr109301.c.jj	2023-03-28 11:56:34.130615683 +0200
+++ gcc/testsuite/gcc.dg/pr109301.c	2023-03-28 11:56:34.130615683 +0200
@@ -0,0 +1,13 @@
+/* PR tree-optimization/109301 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -ffast-math" } */
+
+double x[256];
+
+void
+foo (void)
+{
+  for (int i = 0; i < 256; ++i)
+    for (int j = 0; j < 8; ++j)
+      x[i] = __builtin_pow (x[i], 0.5);
+}

	Jakub

Re: [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301]

[Richard Biener]

On Tue, 28 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> On Tue, Mar 28, 2023 at 09:23:23AM +0000, Richard Biener wrote:
> > But as said, the fix is probably to move the pass property.
> 
> After looking into it in more detail I agree with you.
> powcabs is a pass in the spot sincos was happening before, so the
> only change was defer the sin+cos simplification into cexpi to a later
> new pass (except for the name moving with it) and none of the
> canonicalize_math_p () guarded simplification in match.pd seem to rely
> on those sin+cos -> cexpi simplifications and canonicalize_math_p is
> the only user of this property.
> 
> The following patch fixes the testcase, ok for trunk if it passes
> bootstrap/regtest?

OK.

> Or should I add the match.pd change as well?

I think it shouldn't be necessary.

Thanks,
Richard.

> 2023-03-28  Jakub Jelinek  <jakub@redhat.com>
> 	    Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/109301
> 	* tree-ssa-math-opts.cc (pass_data_cse_sincos): Change
> 	properties_provided from PROP_gimple_opt_math to 0.
> 	(pass_data_expand_powcabs): Change properties_provided from 0 to
> 	PROP_gimple_opt_math.
> 
> 	* gcc.dg/pr109301.c: New test.
> 
> --- gcc/tree-ssa-math-opts.cc.jj	2023-03-12 22:36:06.355178083 +0100
> +++ gcc/tree-ssa-math-opts.cc	2023-03-28 11:57:36.576699748 +0200
> @@ -2237,7 +2237,7 @@ const pass_data pass_data_cse_sincos =
>    OPTGROUP_NONE, /* optinfo_flags */
>    TV_TREE_SINCOS, /* tv_id */
>    PROP_ssa, /* properties_required */
> -  PROP_gimple_opt_math, /* properties_provided */
> +  0, /* properties_provided */
>    0, /* properties_destroyed */
>    0, /* todo_flags_start */
>    TODO_update_ssa, /* todo_flags_finish */
> @@ -2331,7 +2331,7 @@ const pass_data pass_data_expand_powcabs
>    OPTGROUP_NONE, /* optinfo_flags */
>    TV_TREE_POWCABS, /* tv_id */
>    PROP_ssa, /* properties_required */
> -  0, /* properties_provided */
> +  PROP_gimple_opt_math, /* properties_provided */
>    0, /* properties_destroyed */
>    0, /* todo_flags_start */
>    TODO_update_ssa, /* todo_flags_finish */
> --- gcc/testsuite/gcc.dg/pr109301.c.jj	2023-03-28 11:56:34.130615683 +0200
> +++ gcc/testsuite/gcc.dg/pr109301.c	2023-03-28 11:56:34.130615683 +0200
> @@ -0,0 +1,13 @@
> +/* PR tree-optimization/109301 */
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -ffast-math" } */
> +
> +double x[256];
> +
> +void
> +foo (void)
> +{
> +  for (int i = 0; i < 256; ++i)
> +    for (int j = 0; j < 8; ++j)
> +      x[i] = __builtin_pow (x[i], 0.5);
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)