On Sun, Jun 8, 2025 at 2:03 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Sat, Jun 7, 2025 at 12:32 AM Andrew Pinski <quic_apin...@quicinc.com>
> wrote:
> >
> > So currently cselim is limited to targets which have conditional move
> > and also happens later in the pipeline. This adds the limited form of
> > cselim;
> > where there is only one store in the two sides and no loads after the store.
> >
> > This fixes phiprop-2.c for gcn target and now can match the MIN in phiopt1
> > so it moves the matching of MIN to phiopt1.
> >
> > The other testcases already disable cselim so they need to disable phiopt
> > too.
> >
> > Bootstrapped and tested on x86_64-linux-gnu.
> >
> > PR tree-optimization/120533
> > gcc/ChangeLog:
> >
> > * tree-ssa-phiopt.cc (cond_if_else_store_replacement_limited): New
> > function.
> > (pass_phiopt::execute): Call cond_if_else_store_replacement_limited
> > for diamand case.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/tree-ssa/pr35286.c: Add -fno-ssa-phiopt.
> > * gcc.dg/tree-ssa/split-path-6.c: Likewise.
> > * gcc.dg/tree-ssa/split-path-7.c: Likewise.
> > * gcc.dg/tree-ssa/phiprop-2.c: Move the check for MIN_EXPR to
> > phiopt1.
> >
> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > ---
> > gcc/testsuite/gcc.dg/tree-ssa/phiprop-2.c | 5 +-
> > gcc/testsuite/gcc.dg/tree-ssa/pr35286.c | 2 +-
> > gcc/testsuite/gcc.dg/tree-ssa/split-path-6.c | 2 +-
> > gcc/testsuite/gcc.dg/tree-ssa/split-path-7.c | 2 +-
> > gcc/tree-ssa-phiopt.cc | 53 ++++++++++++++++++++
> > 5 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phiprop-2.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/phiprop-2.c
> > index 7181787db5e..ae0d181a43a 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/phiprop-2.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/phiprop-2.c
> > @@ -1,5 +1,5 @@
> > /* { dg-do compile } */
> > -/* { dg-options "-O1 -fdump-tree-phiopt2 -fdump-tree-phiprop1-details" } */
> > +/* { dg-options "-O1 -fdump-tree-phiopt1 -fdump-tree-phiprop1-details" } */
> >
> > /* PR tree-optimization/116824 */
> >
> > @@ -24,5 +24,4 @@ int g(int i, int *tt)
> >
> > /* Check that phiprop1 can do the insert of the loads. */
> > /* { dg-final { scan-tree-dump-times "Inserting PHI for result of load" 1
> > "phiprop1"} } */
> > -/* Should be able to get MIN_EXPR in phiopt2 after cselim and phiprop. */
> > -/* { dg-final { scan-tree-dump-times "MIN_EXPR " 1 "phiopt2" } } */
> > +/* { dg-final { scan-tree-dump-times "MIN_EXPR " 1 "phiopt1" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr35286.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/pr35286.c
> > index 4429cc857bf..b4f8c7ce4fc 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr35286.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr35286.c
> > @@ -1,5 +1,5 @@
> > /* { dg-do compile } */
> > -/* { dg-options "-O2 -fno-code-hoisting -fno-tree-cselim
> > -fdump-tree-pre-stats" } */
> > +/* { dg-options "-O2 -fno-code-hoisting -fno-tree-cselim -fno-ssa-phiopt
> > -fdump-tree-pre-stats" } */
> > int g2;
> > struct A {
> > int a; int b;
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/split-path-6.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/split-path-6.c
> > index 71e6362b10c..e2b0a9571f0 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/split-path-6.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/split-path-6.c
> > @@ -1,5 +1,5 @@
> > /* { dg-do compile } */
> > -/* { dg-options "-O2 -fsplit-paths -fno-tree-cselim
> > -fdump-tree-split-paths-details -fno-finite-loops -fno-tree-dominator-opts
> > -fno-tree-vrp -w" } */
> > +/* { dg-options "-O2 -fsplit-paths -fno-tree-cselim -fno-ssa-phiopt
> > -fdump-tree-split-paths-details -fno-finite-loops -fno-tree-dominator-opts
> > -fno-tree-vrp -w" } */
> >
> > struct __sFILE
> > {
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/split-path-7.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/split-path-7.c
> > index 252fe06c666..35634ab3bd8 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/split-path-7.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/split-path-7.c
> > @@ -1,5 +1,5 @@
> > /* { dg-do compile } */
> > -/* { dg-options "-O2 -fsplit-paths -fno-tree-cselim -fno-tree-sink
> > -fdump-tree-split-paths-details -w" } */
> > +/* { dg-options "-O2 -fsplit-paths -fno-tree-cselim -fno-ssa-phiopt
> > -fno-tree-sink -fdump-tree-split-paths-details -w" } */
> >
> >
> > struct _reent
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index b9c753abc3a..71a71232049 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -3739,6 +3739,54 @@ single_trailing_store_in_bb (basic_block bb, tree
> > vdef, gphi *vphi)
> > return store;
> > }
> >
> > +/* Limited Conditional store replacement. We already know
> > + that the recognized pattern looks like so:
> > +
> > + split:
> > + if (cond) goto THEN_BB; else goto ELSE_BB (edge E1)
> > + THEN_BB:
> > + ...
> > + ONLY_STORE = Y;
> > + ...
> > + goto JOIN_BB;
> > + ELSE_BB:
> > + ...
> > + ONLY_STORE = Z;
> > + ...
> > + fallthrough (edge E0)
> > + JOIN_BB:
> > + some more
> > +
> > + Handleas only the case with single store in THEN_BB and ELSE_BB. That
> > is
>
> Handles
>
> > + cheap enough due to in phiopt and not worry about heurstics. Moving
> > the store
> > + out might provide an opportunity for a phiopt to happen. */
> > +
> > +static bool
> > +cond_if_else_store_replacement_limited (basic_block then_bb, basic_block
> > else_bb,
> > + basic_block join_bb)
> > +{
> > + gphi *vphi = NULL;
> > + for (gphi_iterator si = gsi_start_phis (join_bb); !gsi_end_p (si);
> > + gsi_next (&si))
> > + if (virtual_operand_p (gimple_phi_result (si.phi ())))
> > + {
> > + vphi = si.phi ();
> > + break;
> > + }
>
> get_virtual_phi (join_bb);
>
> (I eventually hoped to order PHIs to make the virtual PHI always the
> first/last, so
> abstraction is the first step)cond_if_else_store_replacement had the same issue where I copied it from. I also pushed a patch to fix that (https://gcc.gnu.org/pipermail/gcc-patches/2025-June/686098.html) . > > > + if (!vphi) > > + return false; > > + tree then_vdef = PHI_ARG_DEF_FROM_EDGE (vphi, single_succ_edge > > (then_bb)); > > + tree else_vdef = PHI_ARG_DEF_FROM_EDGE (vphi, single_succ_edge > > (else_bb)); > > + gimple *then_assign = single_trailing_store_in_bb (then_bb, then_vdef, > > vphi); > > + if (!then_assign) > > + return false; > > else PHI_ARG_DEF_FROM_EDGE belongs here I copied this again from cond_if_else_store_replacement where it has a similar issue. Will push a patch later on to fix that too. Attached is the patch which I pushed. Thanks, Andrew > > OK with those changes. > > Thanks, > Richard. > > > + gimple *else_assign = single_trailing_store_in_bb (else_bb, else_vdef, > > vphi); > > + if (!else_assign) > > + return false; > > + return cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb, > > + then_assign, else_assign, vphi); > > +} > > + > > /* Conditional store replacement. We already know > > that the recognized pattern looks like so: > > > > @@ -4467,6 +4515,11 @@ pass_phiopt::execute (function *) > > && !predictable_edge_p (EDGE_SUCC (bb, 0)) > > && !predictable_edge_p (EDGE_SUCC (bb, 1))) > > hoist_adjacent_loads (bb, bb1, bb2, bb3); > > + > > + /* Try to see if there are only one store in each side of the if > > + and try to remove that. */ > > + if (EDGE_COUNT (bb3->preds) == 2) > > + cond_if_else_store_replacement_limited (bb1, bb2, bb3); > > } > > > > gimple_stmt_iterator gsi; > > -- > > 2.43.0 > >
0001-phi-opt-Do-limited-form-of-cselim-from-phiopt-PR1205.patch
Description: Binary data
