From: Richard Biener <rguenther@suse.de>
To: Andrew Pinski <pinskia@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH][RFC] tree-optimization/88540 - FP x > y ? x : y if-conversion without -ffast-math
Date: Mon, 17 Jul 2023 09:30:48 +0000 (UTC) [thread overview]
Message-ID: <nycvar.YFH.7.77.849.2307170924310.4723@jbgna.fhfr.qr> (raw)
In-Reply-To: <CA+=Sn1=8vVob7M=dyUwxCSiZD9drmNf7oGK5LNqcMuYYjAks9g@mail.gmail.com>
On Fri, 14 Jul 2023, Andrew Pinski wrote:
> On Thu, Jul 13, 2023 at 2:54?AM Richard Biener via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > The following makes sure that FP x > y ? x : y style max/min operations
> > are if-converted at the GIMPLE level. While we can neither match
> > it to MAX_EXPR nor .FMAX as both have different semantics with IEEE
> > than the ternary ?: operation we can make sure to maintain this form
> > as a COND_EXPR so backends have the chance to match this to instructions
> > their ISA offers.
> >
> > The patch does this in phiopt where we recognize min/max and instead
> > of giving up when we have to honor NaNs we alter the generated code
> > to a COND_EXPR.
> >
> > This resolves PR88540 and we can then SLP vectorize the min operation
> > for its testcase. It also resolves part of the regressions observed
> > with the change matching bit-inserts of bit-field-refs to vec_perm.
> >
> > Expansion from a COND_EXPR rather than from compare-and-branch
> > regresses gcc.target/i386/pr54855-13.c and gcc.target/i386/pr54855-9.c
> > by producing extra moves while the corresponding min/max operations
> > are now already synthesized by RTL expansion, register selection
> > isn't optimal. This can be also provoked without this change by
> > altering the operand order in the source.
> >
> > It regresses gcc.target/i386/pr110170.c where we end up CSEing the
> > condition which makes RTL expansion no longer produce the min/max
> > directly and code generation is obfuscated enough to confuse
> > RTL if-conversion.
> >
> > It also regresses gcc.target/i386/ssefp-[12].c where oddly one
> > variant isn't if-converted and ix86_expand_fp_movcc doesn't
> > match directly (the FP constants get expanded twice). A fix
> > could be in emit_conditional_move where both prepare_cmp_insn
> > and emit_conditional_move_1 force the constants to (different)
> > registers.
> >
> > Otherwise bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > PR tree-optimization/88540
> > * tree-ssa-phiopt.cc (minmax_replacement): Do not give up
> > with NaNs but handle the simple case by if-converting to a
> > COND_EXPR.
>
> One thing which I was thinking about adding to phiopt is having the
> last pass do the conversion to COND_EXPR if the target supports a
> conditional move for that expression. That should fix this one right?
> This was one of things I was working towards with the moving to use
> match-and-simplify too.
Note the if-conversion has to happen before BB SLP but the last
phiopt is too late for this (yes, BB SLP could also be enhanced
to handle conditionals and do if-conversion on-the-fly). For
BB SLP there's also usually jump threading making a mess of
same condition chain of if-convertible ops ...
As for the min + max case that regresses due
to CSE (gcc.target/i386/pr110170.c) I wonder whether pre-expanding
_1 = _2 < _3;
_4 = _1 ? _2 : _3;
_5 = _1 ? _3 : _2;
to something more clever would be appropriate anyway. We could
adjust this to either duplicate _1 or expand the COND_EXPRs back
to a single CFG diamond. I suppose force-duplicating non-vector
compares of COND_EXPRs to make TER work again would fix similar
regressions we might already observe (but I'm not aware of many
COND_EXPR generators).
Richard.
> Thanks,
> Andrew
>
> >
> > * gcc.target/i386/pr88540.c: New testcase.
> > * gcc.target/i386/pr54855-12.c: Adjust.
> > * gcc.target/i386/pr54855-13.c: Likewise.
> > ---
> > gcc/testsuite/gcc.target/i386/pr54855-12.c | 2 +-
> > gcc/testsuite/gcc.target/i386/pr54855-13.c | 2 +-
> > gcc/testsuite/gcc.target/i386/pr88540.c | 10 ++++++++++
> > gcc/tree-ssa-phiopt.cc | 21 ++++++++++++++++-----
> > 4 files changed, 28 insertions(+), 7 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/i386/pr88540.c
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr54855-12.c b/gcc/testsuite/gcc.target/i386/pr54855-12.c
> > index 2f8af392c83..09e8ab8ae39 100644
> > --- a/gcc/testsuite/gcc.target/i386/pr54855-12.c
> > +++ b/gcc/testsuite/gcc.target/i386/pr54855-12.c
> > @@ -1,6 +1,6 @@
> > /* { dg-do compile } */
> > /* { dg-options "-O2 -mavx512fp16" } */
> > -/* { dg-final { scan-assembler-times "vmaxsh\[ \\t\]" 1 } } */
> > +/* { dg-final { scan-assembler-times "vm\[ai\]\[nx\]sh\[ \\t\]" 1 } } */
> > /* { dg-final { scan-assembler-not "vcomish\[ \\t\]" } } */
> > /* { dg-final { scan-assembler-not "vmovsh\[ \\t\]" { target { ! ia32 } } } } */
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr54855-13.c b/gcc/testsuite/gcc.target/i386/pr54855-13.c
> > index 87b4f459a5a..a4f25066f81 100644
> > --- a/gcc/testsuite/gcc.target/i386/pr54855-13.c
> > +++ b/gcc/testsuite/gcc.target/i386/pr54855-13.c
> > @@ -1,6 +1,6 @@
> > /* { dg-do compile } */
> > /* { dg-options "-O2 -mavx512fp16" } */
> > -/* { dg-final { scan-assembler-times "vmaxsh\[ \\t\]" 1 } } */
> > +/* { dg-final { scan-assembler-times "vm\[ai\]\[nx\]sh\[ \\t\]" 1 } } */
> > /* { dg-final { scan-assembler-not "vcomish\[ \\t\]" } } */
> > /* { dg-final { scan-assembler-not "vmovsh\[ \\t\]" { target { ! ia32 } } } } */
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr88540.c b/gcc/testsuite/gcc.target/i386/pr88540.c
> > new file mode 100644
> > index 00000000000..b927d0c57d5
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr88540.c
> > @@ -0,0 +1,10 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -msse2" } */
> > +
> > +void test(double* __restrict d1, double* __restrict d2, double* __restrict d3)
> > +{
> > + for (int n = 0; n < 2; ++n)
> > + d3[n] = d1[n] < d2[n] ? d1[n] : d2[n];
> > +}
> > +
> > +/* { dg-final { scan-assembler "minpd" } } */
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index 467c9fd108a..13ee486831d 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -1580,10 +1580,6 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
> >
> > tree type = TREE_TYPE (PHI_RESULT (phi));
> >
> > - /* The optimization may be unsafe due to NaNs. */
> > - if (HONOR_NANS (type) || HONOR_SIGNED_ZEROS (type))
> > - return false;
> > -
> > gcond *cond = as_a <gcond *> (*gsi_last_bb (cond_bb));
> > enum tree_code cmp = gimple_cond_code (cond);
> > tree rhs = gimple_cond_rhs (cond);
> > @@ -1770,6 +1766,9 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
> > else
> > return false;
> > }
> > + else if (HONOR_NANS (type) || HONOR_SIGNED_ZEROS (type))
> > + /* The optimization may be unsafe due to NaNs. */
> > + return false;
> > else if (middle_bb != alt_middle_bb && threeway_p)
> > {
> > /* Recognize the following case:
> > @@ -2103,7 +2102,19 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb, basic_block alt_
> > /* Emit the statement to compute min/max. */
> > gimple_seq stmts = NULL;
> > tree phi_result = PHI_RESULT (phi);
> > - result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> > +
> > + /* When we can't use a MIN/MAX_EXPR still make sure the expression
> > + stays in a form to be recognized by ISA that map to IEEE x > y ? x : y
> > + semantics (that's not IEEE max semantics). */
> > + if (HONOR_NANS (type) || HONOR_SIGNED_ZEROS (type))
> > + {
> > + result = gimple_build (&stmts, cmp, boolean_type_node,
> > + gimple_cond_lhs (cond), rhs);
> > + result = gimple_build (&stmts, COND_EXPR, TREE_TYPE (phi_result),
> > + result, arg_true, arg_false);
> > + }
> > + else
> > + result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> >
> > gsi = gsi_last_bb (cond_bb);
> > gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
> > --
> > 2.35.3
>
--
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)
next prev parent reply other threads:[~2023-07-17 9:30 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230713095401.6C7E53858C41@sourceware.org>
2023-07-14 20:14 ` Andrew Pinski
2023-07-17 9:30 ` Richard Biener [this message]
2023-07-17 20:07 ` Andrew Pinski
2023-07-13 9:53 Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.YFH.7.77.849.2307170924310.4723@jbgna.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=pinskia@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).