On Sat, May 9, 2026 at 3:41 AM Pengxuan Zheng <[email protected]> wrote: > > Currently, cselim requires the middle_bb to have only a single statement. This > patch relaxes this restriction if the following pattern is found and also when > it is safe to do the optimization. > > SPLIT_BB: > ... > STORE = X > if (cond) goto MIDDLE_BB; else goto JOIN_BB (edge E1) > MIDDLE_BB: > ... > STORE = Y; > ... > fallthrough (edge E0) > JOIN_BB: > some more > > Bootstrapped and tested on x86_64-linux-gnu and aarch64-linux-gnu. > > Changes since v1: > * v2: Revert changes to trailing_store_in_bb and do not call > trailing_store_in_bb to get split_assign. > Remove some unnecessary checks in cselim_candidate. > Pass the result of cselim_candidate which is the candidate store for > cselim as an argument to cond_store_replacement instead. > > PR tree-optimization/124405 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (cond_store_replacement): Make ASSIGN (the > candidate store for cselim) an argument of the function instead. > (cselim_candidate): New. > (pass_cselim::execute): Call cselim_candidate and pass the result to > cond_store_replacement. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/pr124405.c: New test. > > Signed-off-by: Pengxuan Zheng <[email protected]> > --- > gcc/testsuite/gcc.dg/tree-ssa/pr124405.c | 12 +++++ > gcc/tree-ssa-phiopt.cc | 68 +++++++++++++++++++++--- > 2 files changed, 73 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr124405.c > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr124405.c > b/gcc/testsuite/gcc.dg/tree-ssa/pr124405.c > new file mode 100644 > index 00000000000..9ba230d2b56 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr124405.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O1 -fdump-tree-cselim-details" } */ > + > +void > +f (int *a, int b) > +{ > + *a &= ~3; > + if (b) > + *a |= 1; > +} > + > +/* { dg-final { scan-tree-dump-times "Conditional store replacement > happened" 1 "cselim"} } */ > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index 324559e6a7d..e55d6efaa2b 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -2991,16 +2991,16 @@ get_non_trapping (void) > JOIN_BB: > some more > > - We check that MIDDLE_BB contains only one store, that that store > + ASSIGN is a store in MIDDLE_BB which is the candidate for cselim. We > check > + that MIDDLE_BB contains only one store (i.e., ASSIGN), that that store > doesn't trap (not via NOTRAP, but via checking if an access to the same > - memory location dominates us, or the store is to a local addressable > - object) and that the store has a "simple" RHS. */ > + memory location dominates us, or the store is to a local addressable > object) > + and that the store has a "simple" RHS. */ > > static bool > -cond_store_replacement (basic_block middle_bb, basic_block join_bb, > - edge e0, edge e1, hash_set<tree> *nontrap) > +cond_store_replacement (basic_block middle_bb, basic_block join_bb, edge e0, > + edge e1, gimple *assign, hash_set<tree> *nontrap) > { > - gimple *assign = last_and_only_stmt (middle_bb); > tree lhs, rhs, name, name2; > gphi *newphi; > gassign *new_stmt; > @@ -3289,6 +3289,58 @@ trailing_store_in_bb (basic_block bb, tree vdef, gphi > *vphi, bool onlyonestore) > return store; > } > > +/* Return the candidate store for cselim. If there is only a single > statement > + in MIDDLE_BB, return that statement. Otherwise, try to find the following > + pattern and return the only store in MIDDLE_BB. > + > + SPLIT_BB: > + ... > + STORE = X > + if (cond) goto MIDDLE_BB; else goto JOIN_BB (edge E1) > + MIDDLE_BB: > + ... > + STORE = Y; > + ... > + fallthrough (edge E0) > + JOIN_BB: > + some more > +*/ > + > +static gimple * > +cselim_candidate (basic_block middle_bb, basic_block join_bb, edge e0, edge > e1) > +{ > + gimple *middle_assign = last_and_only_stmt (middle_bb); > + if (middle_assign) > + return middle_assign;
trailing_store_in_bb would also find the above, so why bother to try both? > + > + gphi *vphi = get_virtual_phi (join_bb); > + if (!vphi) > + return NULL; > + > + tree middle_vdef = PHI_ARG_DEF_FROM_EDGE (vphi, e0); > + middle_assign = trailing_store_in_bb (middle_bb, middle_vdef, vphi, true); > + > + if (!middle_assign || !gimple_assign_single_p (middle_assign) > + || gimple_has_volatile_ops (middle_assign) > + || stmt_references_abnormal_ssa_name (middle_assign)) > + return NULL; This seems to be part of cond_store_replacement already. > + > + tree split_vdef = PHI_ARG_DEF_FROM_EDGE (vphi, e1); > + gimple *split_assign = SSA_NAME_DEF_STMT (split_vdef); > + > + if (!split_assign || !gimple_assign_single_p (split_assign) > + || gimple_has_volatile_ops (split_assign) > + || stmt_references_abnormal_ssa_name (split_assign)) > + return NULL; > + > + if (gimple_vdef (split_assign) != gimple_vuse (middle_assign) > + || !operand_equal_p (gimple_assign_lhs (middle_assign), > + gimple_assign_lhs (split_assign), 0)) > + return NULL; So I'm not sure, the original last_and_only_stmt case didn't check this, but relies on the NOTRAP based check in cond_store_replacement (contrary to the function comment ...) Either this check is not neccessary or it's also necessary when there's no other stmts besides middle_assing in the block. > + > + return middle_assign; > +} > + > /* Limited Conditional store replacement. We already know > that the recognized pattern looks like so: > > @@ -4260,7 +4312,9 @@ pass_cselim::execute (function *) > optimization if the join block has more than two predecessors. */ > if (EDGE_COUNT (bb2->preds) > 2) > return; > - if (cond_store_replacement (bb1, bb2, e1, e2, nontrap)) > + > + gimple *assign = cselim_candidate (bb1, bb2, e1, e2); > + if (cond_store_replacement (bb1, bb2, e1, e2, assign, nontrap)) > cfgchanged = true; > }; > > -- > 2.34.1 >
