On Thu, 11 Jun 2026, Jeffrey Law wrote: > On 6/10/2026 1:10 PM, [email protected] wrote: > > From: Kyrylo Tkachov <[email protected]> > > > > Add factor_out_conditional_load: it turns a diamond PHI <*P, *Q> whose two > > arguments are single-use, non-volatile scalar MEM_REF loads reading the same > > memory state into P' = PHI <P, Q> and a single load *P', reusing the PHI > > result so its range info is preserved. No speculative load is introduced > > (the > > load uses whichever pointer the taken edge selected). > > > > When the two arms' TBAA alias-ptr types differ, the merged load > > conservatively > > falls back to ptr_type_node and drops the dependence clique/base, the way > > get_alias_type_for_stmts does, rather than rejecting the transform. > > > > Its eligibility test is split out into conditional_load_factorable_p so the > > sink pass (next patch) can gate on commonability without mutating; both are > > exported for it to reuse. > > > > Bootstrapped and tested on aarch64-none-linux-gnu. > > > > Signed-off-by: Kyrylo Tkachov <[email protected]> > > > > PR tree-optimization/125557 > > * tree-ssa-phiopt.h (conditional_load_factorable_p): Declare. > > (factor_out_conditional_load): Declare. > > * tree-ssa-phiopt.cc: Include "alias.h". > > (remove_factored_arm_defs): New. > > (conditional_load_factorable_p): New. > > (factor_out_conditional_load): New. > > --- > > gcc/tree-ssa-phiopt.cc | 132 +++++++++++++++++++++++++++++++++++++++++ > > gcc/tree-ssa-phiopt.h | 9 +++ > > 2 files changed, 141 insertions(+) > > > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > > index dc6c33a930d..b018590c378 100644 > > --- a/gcc/tree-ssa-phiopt.cc > > +++ b/gcc/tree-ssa-phiopt.cc > > @@ -49,6 +49,7 @@ along with GCC; see the file COPYING3. If not see > > > > + > > +/* Return true if factor_out_conditional_load (below) would factor the > > load PHI > > + at MERGE. E0/E1 are the arm->MERGE edges. This is > > factor_out_conditional_load's > > + eligibility test, split out so callers (the sink pass) can gate on > > commonability > > + without mutating: the two MUST stay in lockstep, otherwise a caller > > could common > > + a load the sink then cannot finish if-converting. */ > > + > > +bool > > +conditional_load_factorable_p (edge e0, edge e1, basic_block merge, gphi > > *phi) > > +{ > > + if (virtual_operand_p (gimple_phi_result (phi)) > > + || gimple_phi_num_args (phi) != 2 > > + || get_virtual_phi (merge)) > > + return false; > > + > > + tree arg0 = gimple_phi_arg_def (phi, e0->dest_idx); > > + tree arg1 = gimple_phi_arg_def (phi, e1->dest_idx); > > + if (TREE_CODE (arg0) != SSA_NAME || TREE_CODE (arg1) != SSA_NAME > > + || !has_single_use (arg0) || !has_single_use (arg1)) > > + return false; > > + > > + gimple *d0 = SSA_NAME_DEF_STMT (arg0); > > + gimple *d1 = SSA_NAME_DEF_STMT (arg1); > > + if (!is_gimple_assign (d0) || !gimple_assign_load_p (d0) > > + || !is_gimple_assign (d1) || !gimple_assign_load_p (d1) > > + || gimple_has_volatile_ops (d0) || gimple_has_volatile_ops (d1) > > + || gimple_vuse (d0) != gimple_vuse (d1)) > > + return false; > > + > > + tree ref0 = gimple_assign_rhs1 (d0); > > + tree ref1 = gimple_assign_rhs1 (d1); > > + /* Both must be plain *P loads (zero offset) of a compatible value type. > > The > > + TBAA alias-ptr type carried by MEM_REF operand 1 need not match; it is > > + merged the way get_alias_type_for_stmts does when the load is built. > > */ > > + if (TREE_CODE (ref0) != MEM_REF || TREE_CODE (ref1) != MEM_REF > > + || !integer_zerop (TREE_OPERAND (ref0, 1)) > > + || !integer_zerop (TREE_OPERAND (ref1, 1)) > > + || !types_compatible_p (TREE_TYPE (ref0), TREE_TYPE (ref1))) > > + return false; > So presumably the idea being zero-offset is we can trivially select with > the new PHI rather than having to emit the address calculation into a > new SSA_NAME in the appropriate predecessor, right? > > > > > + > > +bool > > +factor_out_conditional_load (edge e0, edge e1, basic_block merge, gphi > > *phi) > > +{ > > + if (!conditional_load_factorable_p (e0, e1, merge, phi)) > > + return false; > So it seems like this prevents default defs (since the target function > requires the defining statement to be an assignment that is a load with > associated vuses, so it's never going to be a default def. WHich in > turn I think means we can never have a NULL in remove_factored_arm_defs, > right? > > > > + > > + /* Re-derive the loads and pointers validated by the predicate above. */ > > + gimple *d0 = SSA_NAME_DEF_STMT (gimple_phi_arg_def (phi, e0->dest_idx)); > > + gimple *d1 = SSA_NAME_DEF_STMT (gimple_phi_arg_def (phi, e1->dest_idx)); > > + tree ref0 = gimple_assign_rhs1 (d0); > > + tree ref1 = gimple_assign_rhs1 (d1); > > + tree p0 = TREE_OPERAND (ref0, 0); > > + tree p1 = TREE_OPERAND (ref1, 0); > > + > > + /* Build P' = PHI <P, Q> and the single load result = *P'. */ > > + tree res = gimple_phi_result (phi); > > + tree ptr = make_ssa_name (TREE_TYPE (p0)); > > + gphi *pphi = create_phi_node (ptr, merge); > > + add_phi_arg (pphi, p0, e0, gimple_phi_arg_location (phi, e0->dest_idx)); > > + add_phi_arg (pphi, p1, e1, gimple_phi_arg_location (phi, e1->dest_idx)); > > + > > + /* Merge the two arms' TBAA info as get_alias_type_for_stmts does: keep > > the > > + common alias-ptr type and dependence clique/base when the arms agree, > > + otherwise fall back to ptr_type_node (alias-everything) and drop the > > + clique/base, so the combined load conservatively conflicts with any > > store > > + either original arm could. */ > > + unsigned short clique = MR_DEPENDENCE_CLIQUE (ref0); > > + unsigned short base = MR_DEPENDENCE_BASE (ref0); > > + if (clique != MR_DEPENDENCE_CLIQUE (ref1) || base != MR_DEPENDENCE_BASE > > (ref1)) > > + clique = base = 0; > > + tree atype = TREE_TYPE (TREE_OPERAND (ref0, 1)); > > + if (!alias_ptr_types_compatible_p (atype, TREE_TYPE (TREE_OPERAND (ref1, > > 1)))) > > + { > > + atype = ptr_type_node; > > + clique = base = 0; > > + } > Do we want to carry over any other information? I guess a locus > wouldn't make sense since the original likely had two loci and selecting > across those doesn't make much sense. > > Do we need to check that the alignments of the two memory references are > the same and copy that data over to the new reference? > > > > Generally looks pretty good. I'd say we can go forward once we know if > we can drop the NULL statement test in remove_factored_arm_defs and > whether or not there are any other attributes of the memory references > we should check and potentially propagate to the new reference.
This looks much like what tree-ssa-sink.cc:sink_common_stores_to_bb does and I wonder whether sinking common operations other than stores should be done from the same pass, as sinking common stores would eventually introduce PHIs for the stored value? If we do this in phiopt we'd create a pass ordering dependence between sinking of stores and this? > jeff -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
