On Fri, May 8, 2026 at 5:38 AM Richard Biener <[email protected]> wrote:
> On Sat, Apr 11, 2026 at 2:44 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. > > > > PR tree-optimization/124405 > > > > gcc/ChangeLog: > > > > * tree-ssa-phiopt.cc (trailing_store_in_bb): Allow vphi to be > NULL to > > support more general scenarios. > > (cselim_candidate): New. > > (cond_store_replacement): Add split_bb argument and call > > cselim_candidate instead of last_and_only_stmt. > > (pass_cselim::execute): Update call 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 | 151 +++++++++++++++++------ > > 2 files changed, 124 insertions(+), 39 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 0bf7e58b8f0..1db20ce53e8 100644 > > --- a/gcc/tree-ssa-phiopt.cc > > +++ b/gcc/tree-ssa-phiopt.cc > > @@ -2980,6 +2980,113 @@ get_non_trapping (void) > > return nontrap; > > } > > > > +/* Return the last store in BB with VDEF or NULL if there are loads > following > > + the store. VPHI if non-NULL is where the only use of the vdef > should be. If > > + VPHI is NULL, return NULL if there is any load or store following > the store. > > + If ONLYONESTORE is true, then the store is the only store in the > BB. */ > > + > > +static gimple * > > +trailing_store_in_bb (basic_block bb, tree vdef, gphi *vphi, bool > onlyonestore) > > +{ > > + if (SSA_NAME_IS_DEFAULT_DEF (vdef)) > > + return NULL; > > + gimple *store = SSA_NAME_DEF_STMT (vdef); > > + if (gimple_bb (store) != bb > > + || gimple_code (store) == GIMPLE_PHI) > > + return NULL; > > + > > + /* Verify there is no other store in this BB if requested. */ > > + if (onlyonestore > > + && !SSA_NAME_IS_DEFAULT_DEF (gimple_vuse (store)) > > + && gimple_bb (SSA_NAME_DEF_STMT (gimple_vuse (store))) == bb > > + && gimple_code (SSA_NAME_DEF_STMT (gimple_vuse (store))) != > GIMPLE_PHI) > > + return NULL; > > + > > + if (vphi) > > + { > > + /* Verify the vdef of the store should only be used by VPHI. */ > > + use_operand_p use_p; > > + gimple *use_stmt; > > + if (!single_imm_use (gimple_vdef (store), &use_p, &use_stmt)) > > + return NULL; > > + if (use_stmt != vphi) > > + return NULL; > > + } > > + else > > + { > > + /* Verify there is no load or store after the store. */ > > + use_operand_p use_p; > > + imm_use_iterator imm_iter; > > + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_vdef (store)) > > + if (USE_STMT (use_p) != store && gimple_bb (USE_STMT (use_p)) == > bb) > > The VDEF of a store is never used in the store itself, so the BB check > is enough. > > Note I don't see why we'd ever have no virtual PHI when there's a store in > middle-bb. > Thanks for the review, Richard. The changes here were mainly made for the case when BB is the split_bb. However, as you pointed out below, we don't need to call trailing_store_in_bb to get split_assign. Therefore, I've reverted all the changes made in trailing_store_in_bb in the updated patch. https://gcc.gnu.org/pipermail/gcc-patches/2026-May/716181.html > > > + return NULL; > > + } > > + > > + 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 split_bb, 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; > > + > > + 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; > > + > > + tree split_vdef = PHI_ARG_DEF_FROM_EDGE (vphi, e1); > > + gimple *split_assign > > + = trailing_store_in_bb (split_bb, split_vdef, NULL, true); > > + > > So what are you trying to match here? A comment is very much > warranted. Why can't you just look at the definition of the > VUSE of split_vdef? e1->src == split_bb, right? > > I agree. I have removed the call to trailing_store_in_bb and updated the patch accordingly. > > + 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; > > + > > + /* The vdef of split_assign should only be used by middle_assign or > vphi. */ > > Why's that? Isn't the earlier store solely because we need to know > the conditional > store doesn't trap? For the actual cselim we'll insert a load, no? > So we'd need > to verify there's no (aliasing) store between split_bb and the trailing > store > in middle-bb? > > This does seem to be unnecessary and I have removed the check in the updated patch. > > + use_operand_p use_p; > > + imm_use_iterator imm_iter; > > + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_vdef (split_assign)) > > + if (USE_STMT (use_p) != middle_assign > > + && USE_STMT (use_p) != vphi) > > + return NULL; > > + > > + return middle_assign; > > +} > > + > > /* Do the main work of conditional store replacement. We already know > > that the recognized pattern looks like so: > > > > @@ -2997,10 +3104,11 @@ get_non_trapping (void) > > 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 split_bb, basic_block middle_bb, > > + basic_block join_bb, edge e0, edge e1, > > + hash_set<tree> *nontrap) > > { > > - gimple *assign = last_and_only_stmt (middle_bb); > > + gimple *assign = cselim_candidate (split_bb, middle_bb, join_bb, e0, > e1); > > I think logically this now belongs as pre-check to ... (*) > > > tree lhs, rhs, name, name2; > > gphi *newphi; > > gassign *new_stmt; > > @@ -3254,41 +3362,6 @@ cond_if_else_store_replacement_1 (basic_block > then_bb, basic_block else_bb, > > return true; > > } > > > > -/* Return the last store in BB with VDEF or NULL if there are > > - loads following the store. VPHI is where the only use of the > > - vdef should be. If ONLYONESTORE is true, then the store is > > - the only store in the BB. */ > > - > > -static gimple * > > -trailing_store_in_bb (basic_block bb, tree vdef, gphi *vphi, bool > onlyonestore) > > -{ > > - if (SSA_NAME_IS_DEFAULT_DEF (vdef)) > > - return NULL; > > - gimple *store = SSA_NAME_DEF_STMT (vdef); > > - if (gimple_bb (store) != bb > > - || gimple_code (store) == GIMPLE_PHI) > > - return NULL; > > - > > - /* Verify there is no other store in this BB if requested. */ > > - if (onlyonestore > > - && !SSA_NAME_IS_DEFAULT_DEF (gimple_vuse (store)) > > - && gimple_bb (SSA_NAME_DEF_STMT (gimple_vuse (store))) == bb > > - && gimple_code (SSA_NAME_DEF_STMT (gimple_vuse (store))) != > GIMPLE_PHI) > > - return NULL; > > - > > - > > - /* Verify there is no load or store after the store, the vdef of the > store > > - should only be used by the vphi joining the 2 bbs. */ > > - use_operand_p use_p; > > - gimple *use_stmt; > > - if (!single_imm_use (gimple_vdef (store), &use_p, &use_stmt)) > > - return NULL; > > - if (use_stmt != vphi) > > - return NULL; > > - > > - return store; > > -} > > - > > /* Limited Conditional store replacement. We already know > > that the recognized pattern looks like so: > > > > @@ -4259,7 +4332,7 @@ 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)) > > + if (cond_store_replacement (bb, bb1, bb2, e1, e2, nontrap)) > > (*) here. > > I've updated the patch accordingly. Please let me know if there's any other comment. Here's the link to the updated patch: https://gcc.gnu.org/pipermail/gcc-patches/2026-May/716181.html Thanks, Pengxuan > > cfgchanged = true; > > }; > > > > -- > > 2.34.1 > > >
