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

[Richard Biener <richard.guenther@gmail.com>]

On Mon, May 20, 2024 at 11:37 PM Andrew Pinski (QUIC)
<quic_apinski@quicinc.com> wrote:
>
> > -----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?

Please wait until after the release.

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 <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
> > >