| Message ID | 20240518231146.954808-1-quic_apinski@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143] | expand |
> 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 ? > 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 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 >
> -----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 > >
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 > > >
On Mon, May 20, 2024 at 11:08 PM Richard Biener <richard.guenther@gmail.com> wrote: > > 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. 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_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 > > > > From d30afaae6764379a63c22459b40aaecfa82b0fc4 Mon Sep 17 00:00:00 2001 From: Andrew Pinski <quic_apinski@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_apinski@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)
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)
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. 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. 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