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