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

  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.


	Jakub
  

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

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
  

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.