On Fri, Jun 12, 2026 at 10:21 AM Kyrylo Tkachov <[email protected]> wrote:
>
>
>
> > On 12 Jun 2026, at 02:31, Jeffrey Law <[email protected]> wrote:
> >
> > On 6/10/2026 1:10 PM, [email protected] wrote:
> >> From: Kyrylo Tkachov <[email protected]>
> >>
> >> factor_out_conditional_load currently only runs from the sink pass. Also
> >> call
> >> it from phiopt itself when the join block of a diamond load PHI <*P, *Q>
> >> lies
> >> outside any loop: there is nothing to loop-vectorise there, so commoning
> >> the
> >> two arm loads into a single P' = PHI <P, Q>; result = *P' is a pure win
> >> and is
> >> cheap to attempt.
> >>
> >> This is restricted to:
> >> * the late phiopt run (!early_p), since after inlining the region may yet
> >> become part of a loop where the commoned load could inhibit
> >> vectorisation;
> >> * SSA-name arm pointers (a new REQUIRE_SSA_ARGS argument to
> >> factor_out_conditional_load), so that phiprop does not undo it.
> >>
> >> This triggers a few 100s times in SPEC2026 with no harmful effects. The
> >> llvm benchmarks improves slightly on my aarch64 machine (1.6%)
> >>
> >> Bootstrapped and tested on aarch64-none-linux-gnu.
> >> Ok for trunk after the prerequisites?
> >>
> >> Thanks,
> >> Kyrill
> >>
> >> Signed-off-by: Kyrylo Tkachov <[email protected]>
> >>
> >> gcc/
> >> PR tree-optimization/125557
> >> * tree-ssa-phiopt.h (conditional_load_factorable_p)
> >> (factor_out_conditional_load): Add a REQUIRE_SSA_ARGS parameter
> >> defaulting to false.
> >> * tree-ssa-phiopt.cc (conditional_load_factorable_p): Honour
> >> REQUIRE_SSA_ARGS.
> >> (factor_out_conditional_load): Pass it through.
> >> (pass_phiopt::execute): Call factor_out_conditional_load for
> >> out-of-loop diamonds in the late run.
> >>
> >> gcc/testsuite/
> >>
> >> PR tree-optimization/125557
> >> * gcc.dg/tree-ssa/phiopt-factor-load-1.c: New test.
> >> ---
> >>
> >> diff --git a/gcc/tree-ssa-phiopt.h b/gcc/tree-ssa-phiopt.h
> >> index 7383095f5a5..8f9b9edcdad 100644
> >> --- a/gcc/tree-ssa-phiopt.h
> >> +++ b/gcc/tree-ssa-phiopt.h
> >> @@ -25,11 +25,13 @@ extern int find_different_opnum (const gimple_match_op
> >> &arg0_op,
> >> /* True if factor_out_conditional_load would factor the load PHI at
> >> MERGE; lets
> >> the sink pass gate on commonability without mutating. */
> >> extern bool conditional_load_factorable_p (edge e0, edge e1, basic_block
> >> merge,
> >> - gphi *phi);
> >> + gphi *phi,
> >> + bool require_ssa_args = false);
> > I tend to prefer fixing caller rather than defaulting arguments. If
> > there's only a few call sites just fix them. Similarly for
> > factor_out_conditional_load.
> >
> > OK when prereqs go in. Ideally with the defaulted arguments fixed too.
>
> Thanks for the reviews from all of you. I’ve tried to incorporate all the
> comments and ended up merging the patches into one patch that runs during
> sink_code_in_bb that hopefully addresses all the comments.
> I’ve sent it separately.
I still think this is wrong. We want to do some stuff earlier in phiopt.
I am not sure we want it in sink just yet.
Note the alignment change you added can just be simplified down to:
/* The alignment of the two accesses need to be the same. */
if (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (ref0)))
!= TYPE_ALIGN (TREE_TYPE (TREE_TYPE (ref1))))
return false;
I am working on a much simpler patch which extracts your code into
phiopt and then we can go forward with figuring out how to handle the
sink pass scc stuff. I am still curious about the idea behind the scc
stuff because it seems a little over thought for almost no gain.
> Kyrill
>
> >
> > jeff
>
>
