RE: [PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143]

["Andrew Pinski (QUIC)" <quic_apinski@quicinc.com>]

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Sunday, May 19, 2024 11:55 AM
> To: Andrew Pinski (QUIC) <quic_apinski@quicinc.com>
> 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
> <quic_apinski@quicinc.com>:
> >
> > 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?

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 <quic_apinski@quicinc.com>
> > ---
> > .../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
> >