* [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] @ 2023-03-28 8:25 Jakub Jelinek 2023-03-28 8:57 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Jakub Jelinek @ 2023-03-28 8:25 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] 2023-03-28 8:25 [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] Jakub Jelinek @ 2023-03-28 8:57 ` Richard Biener 2023-03-28 9:15 ` Jakub Jelinek 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2023-03-28 8:57 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] 2023-03-28 8:57 ` Richard Biener @ 2023-03-28 9:15 ` Jakub Jelinek 2023-03-28 9:23 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Jakub Jelinek @ 2023-03-28 9:15 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] 2023-03-28 9:15 ` Jakub Jelinek @ 2023-03-28 9:23 ` Richard Biener 2023-03-28 10:07 ` [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301] Jakub Jelinek 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2023-03-28 9:23 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301] 2023-03-28 9:23 ` Richard Biener @ 2023-03-28 10:07 ` Jakub Jelinek 2023-03-28 10:53 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Jakub Jelinek @ 2023-03-28 10:07 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301] 2023-03-28 10:07 ` [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301] Jakub Jelinek @ 2023-03-28 10:53 ` Richard Biener 0 siblings, 0 replies; 6+ messages in thread From: Richard Biener @ 2023-03-28 10:53 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-28 10:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-28 8:25 [PATCH] match.pd: Fix up sqrt (sqrt (x)) simplification [PR109301] Jakub Jelinek 2023-03-28 8:57 ` Richard Biener 2023-03-28 9:15 ` Jakub Jelinek 2023-03-28 9:23 ` Richard Biener 2023-03-28 10:07 ` [PATCH] tree-ssa-math-opts: Move PROP_gimple_opt_math from sincos pass to powcabs [PR109301] Jakub Jelinek 2023-03-28 10:53 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).