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 ?
> 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
> ---
> .../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 000..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 000..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 000..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 = gimpl
RE: [PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143]
> -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?
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 000..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 000..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;
>
Re: [PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143]
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.
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 000..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 000..05c3bbe9738
> > > --- /dev/null
> > > +++ b
Re: [PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143]
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 000..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; >
