On Fri, May 15, 2026 at 2:12 AM Richard Biener <[email protected]> wrote:
> 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? > Agreed. > > > + > > + 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. > Yes, I have removed these in the latest patch. https://gcc.gnu.org/pipermail/gcc-patches/2026-May/717749.html > > > + > > + 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. > Good point! Currently, we do the following legality check in cond_store_replacement. /* And no PHI nodes so all uses in the single stmt are also available where we insert to. */ if (!gimple_seq_empty_p (phi_nodes (middle_bb))) return false; This is enough to guarantee all uses in the single stmt are also available where we insert to. However, when we relax the restriction that middle_bb can only have one statement, we need to generalize the check. I updated the patch to do the following check instead to make sure all uses are also available we insert to. Please let me know if you have any comment on this. FOR_EACH_SSA_TREE_OPERAND (use, assign, iter, SSA_OP_USE) { if (use == rhs) continue; gimple *stmt = SSA_NAME_DEF_STMT (use); if (stmt && gimple_bb (stmt) == middle_bb) return false; } Here's the link to the latest patch: https://gcc.gnu.org/pipermail/gcc-patches/2026-May/717749.html Thanks, Pengxuan > > > > + > > + 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 > > >
