On Mon, May 20, 2024 at 11:08 PM Richard Biener wrote: > > On Mon, May 20, 2024 at 11:37 PM Andrew Pinski (QUIC) > wrote: > > > > > -----Original Message----- > > > From: Richard Biener > > > Sent: Sunday, May 19, 2024 11:55 AM > > > To: Andrew Pinski (QUIC) > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: Re: [PATCH] PHIOPT: Don't transform minmax if > > > middle bb contains a phi [PR115143] > > > > > > > > > > > > > Am 19.05.2024 um 01:12 schrieb Andrew Pinski > > > : > > > > > > > > The problem here is even if last_and_only_stmt returns a > > > statement, > > > > the bb might still contain a phi node which defines a ssa > > > name which > > > > is used in that statement so we need to add a check to make > > > sure that > > > > the phi nodes are empty for the middle bbs in both the > > > > `CMP?MINMAX:MINMAX` case and the `CMP?MINMAX:B` > > > cases. > > > > > > Is that single arg PHIs or do we have an extra edge into the > > > middle BB? I think that might be unexpected, at least costing > > > wise. Maybe Also to some of the replacement code we have ? > > > > It is only a single arg PHI since we already reject multiple edges in the middle BBs for these cases. > > It was EVPR that produces the single arg PHI in the original testcase from folding of a conditional to false and evpr does not do simple name prop in this case and there is no pass inbetween evrp and phiopt that will clear up single arg PHI. > > I added the Gimple based testcases basically to avoid the needing of depending on what previous passes could produce too. > > > > > > > > > OK for trunk and backport to all open branches since r14- > > > 3827-g30e6ee074588ba was backported? > > > > Bootstrapped and tested on x86_64_linux-gnu with no > > > regressions. > > > > > > > > > > Ok > > > > Does this include the GCC 13 branch or should I wait until after the GCC 13.3.0 release? > > Please wait until after the release. Committed the modified attached patch for GCC 12. GCC 12 didn't have the diamond case which is why a modified patch was needed. Thanks, Andrew > > Thanks, > Richard. > > > Thanks, > > Andrew Pinski > > > > > > > > Richard > > > > > > > PR tree-optimization/115143 > > > > > > > > gcc/ChangeLog: > > > > > > > > * tree-ssa-phiopt.cc (minmax_replacement): Check for > > > empty > > > > phi nodes for middle bbs for the case where middle bb is > > > not empty. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * gcc.c-torture/compile/pr115143-1.c: New test. > > > > * gcc.c-torture/compile/pr115143-2.c: New test. > > > > * gcc.c-torture/compile/pr115143-3.c: New test. > > > > > > > > Signed-off-by: Andrew Pinski > > > > --- > > > > .../gcc.c-torture/compile/pr115143-1.c | 21 > > > +++++++++++++ > > > > .../gcc.c-torture/compile/pr115143-2.c | 30 > > > +++++++++++++++++++ > > > > .../gcc.c-torture/compile/pr115143-3.c | 29 > > > ++++++++++++++++++ > > > > gcc/tree-ssa-phiopt.cc | 12 ++++++++ > > > > 4 files changed, 92 insertions(+) > > > > create mode 100644 gcc/testsuite/gcc.c- > > > torture/compile/pr115143-1.c > > > > create mode 100644 gcc/testsuite/gcc.c- > > > torture/compile/pr115143-2.c > > > > create mode 100644 gcc/testsuite/gcc.c- > > > torture/compile/pr115143-3.c > > > > > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c > > > > b/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c > > > > new file mode 100644 > > > > index 00000000000..5cb119ea432 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c > > > > @@ -0,0 +1,21 @@ > > > > +/* PR tree-optimization/115143 */ > > > > +/* This used to ICE. > > > > + minmax part of phiopt would transform, > > > > + would transform `a!=0?min(a, b) : 0` into `min(a,b)` > > > > + which was correct except b was defined by a phi in the > > > inner > > > > + bb which was not handled. */ > > > > +short a, d; > > > > +char b; > > > > +long c; > > > > +unsigned long e, f; > > > > +void g(unsigned long h) { > > > > + if (c ? e : b) > > > > + if (e) > > > > + if (d) { > > > > + a = f ? ({ > > > > + unsigned long i = d ? f : 0, j = e ? h : 0; > > > > + i < j ? i : j; > > > > + }) : 0; > > > > + } > > > > +} > > > > + > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c > > > > b/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c > > > > new file mode 100644 > > > > index 00000000000..05c3bbe9738 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c > > > > @@ -0,0 +1,30 @@ > > > > +/* { dg-options "-fgimple" } */ > > > > +/* PR tree-optimization/115143 */ > > > > +/* This used to ICE. > > > > + minmax part of phiopt would transform, > > > > + would transform `a!=0?min(a, b) : 0` into `min(a,b)` > > > > + which was correct except b was defined by a phi in the > > > inner > > > > + bb which was not handled. */ > > > > +unsigned __GIMPLE (ssa,startwith("phiopt")) foo (unsigned > > > a, unsigned > > > > +b) { > > > > + unsigned j; > > > > + unsigned _23; > > > > + unsigned _12; > > > > + > > > > + __BB(2): > > > > + if (a_6(D) != 0u) > > > > + goto __BB3; > > > > + else > > > > + goto __BB4; > > > > + > > > > + __BB(3): > > > > + j_10 = __PHI (__BB2: b_11(D)); > > > > + _23 = __MIN (a_6(D), j_10); > > > > + goto __BB4; > > > > + > > > > + __BB(4): > > > > + _12 = __PHI (__BB3: _23, __BB2: 0u); return _12; > > > > + > > > > +} > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c > > > > b/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c > > > > new file mode 100644 > > > > index 00000000000..53c5fb5588e > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c > > > > @@ -0,0 +1,29 @@ > > > > +/* { dg-options "-fgimple" } */ > > > > +/* PR tree-optimization/115143 */ > > > > +/* This used to ICE. > > > > + minmax part of phiopt would transform, > > > > + would transform `a!=0?min(a, b) : 0` into `min(a,b)` > > > > + which was correct except b was defined by a phi in the > > > inner > > > > + bb which was not handled. */ > > > > +unsigned __GIMPLE (ssa,startwith("phiopt")) foo (unsigned > > > a, unsigned > > > > +b) { > > > > + unsigned j; > > > > + unsigned _23; > > > > + unsigned _12; > > > > + > > > > + __BB(2): > > > > + if (a_6(D) > 0u) > > > > + goto __BB3; > > > > + else > > > > + goto __BB4; > > > > + > > > > + __BB(3): > > > > + j_10 = __PHI (__BB2: b_7(D)); > > > > + _23 = __MIN (a_6(D), j_10); > > > > + goto __BB4; > > > > + > > > > + __BB(4): > > > > + _12 = __PHI (__BB3: _23, __BB2: 0u); > > > > + return _12; > > > > +} > > > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > > > index > > > > f166c3132cb..918cf50b589 100644 > > > > --- a/gcc/tree-ssa-phiopt.cc > > > > +++ b/gcc/tree-ssa-phiopt.cc > > > > @@ -1925,6 +1925,10 @@ minmax_replacement > > > (basic_block cond_bb, basic_block middle_bb, basic_block > > > alt_ > > > > || gimple_code (assign) != GIMPLE_ASSIGN) > > > > return false; > > > > > > > > + /* There cannot be any phi nodes in the middle bb. */ > > > > + if (!gimple_seq_empty_p (phi_nodes (middle_bb))) > > > > + return false; > > > > + > > > > lhs = gimple_assign_lhs (assign); > > > > ass_code = gimple_assign_rhs_code (assign); > > > > if (ass_code != MAX_EXPR && ass_code != MIN_EXPR) > > > @@ -1938,6 > > > > +1942,10 @@ minmax_replacement (basic_block cond_bb, > > > basic_block middle_bb, basic_block alt_ > > > > || gimple_code (assign) != GIMPLE_ASSIGN) > > > > return false; > > > > > > > > + /* There cannot be any phi nodes in the alt middle bb. > > > */ > > > > + if (!gimple_seq_empty_p (phi_nodes (alt_middle_bb))) > > > > + return false; > > > > + > > > > alt_lhs = gimple_assign_lhs (assign); > > > > if (ass_code != gimple_assign_rhs_code (assign)) > > > > return false; > > > > @@ -2049,6 +2057,10 @@ minmax_replacement > > > (basic_block cond_bb, basic_block middle_bb, basic_block > > > alt_ > > > > || gimple_code (assign) != GIMPLE_ASSIGN) > > > > return false; > > > > > > > > + /* There cannot be any phi nodes in the middle bb. */ > > > > + if (!gimple_seq_empty_p (phi_nodes (middle_bb))) > > > > + return false; > > > > + > > > > lhs = gimple_assign_lhs (assign); > > > > ass_code = gimple_assign_rhs_code (assign); > > > > if (ass_code != MAX_EXPR && ass_code != MIN_EXPR) > > > > -- > > > > 2.43.0 > > > >