Message ID | ZCKkbo4zAg36w3wI@tucnak |
---|---|
State | New |
Headers | show |
Series | match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] | expand |
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.
--- 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); +}