On Mon, May 20, 2024 at 11:08 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 11:37 PM Andrew Pinski (QUIC)
> <quic_apin...@quicinc.com> wrote:
> >
> > > -----Original Message-----
> > > From: Richard Biener <richard.guent...@gmail.com>
> > > Sent: Sunday, May 19, 2024 11:55 AM
> > > To: Andrew Pinski (QUIC) <quic_apin...@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_apin...@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.
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 <quic_apin...@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
> > > >
From d30afaae6764379a63c22459b40aaecfa82b0fc4 Mon Sep 17 00:00:00 2001
From: Andrew Pinski <quic_apin...@quicinc.com>
Date: Sat, 18 May 2024 11:55:58 -0700
Subject: [PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi
[PR115143]
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.
Bootstrapped and tested on x86_64_linux-gnu with no regressions.
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_apin...@quicinc.com>
(cherry picked from commit 9ff8f041331ef8b56007fb3c4d41d76f9850010d)
---
.../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 | 4 +++
4 files changed, 84 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 e2dba56383b..558d5b4b57d 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -1973,6 +1973,10 @@ minmax_replacement (basic_block cond_bb, basic_block
middle_bb,
|| 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
