On Fri, Jun 12, 2026 at 2:15 PM Andrew Pinski
<[email protected]> wrote:
>
> On Fri, Jun 12, 2026 at 12:37 PM Andrew Pinski
> <[email protected]> wrote:
> >
> > 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.
>
> Attached is the simplified patch set which I am testing now; will
> formally submit them when testing is finished.
> The first one allows for iterating of cs-elim and factoring. Which
> will allow for later on iterating of load and store factoring. Right
> now load factoring does not support a store in the middle bbs. While
> store elimination does not allow for loads after the store.
> The second patch is the load factoring with a simple heuristics.
> Enough to support the testcases that were in Kyrylo's patch.
I found one issue with it (and it will be with Kyrylo's patch too for
the same reason; just maybe not as exposed).
We have to reject all non-ssa names even if after
fold_before_rtl_expansion_p () is true.
This is comment I have here:
```
/* We can't factor out a non-ssa named based load
as it might cause a variable not taken an
address to become needing the address taken.
An example is in go.
Were we produce:
_24 = PHI <&crypto/tls.cipherSuitesPreferenceOrder(36),
&crypto/tls.cipherSuitesPreferenceOrderNoAES(37)>
And &crypto/tls.cipherSuitesPreferenceOrder address bit was not set.
FIXME: Refine to check ADDRESSABLE bit. */
```
Basically the MEM here might actually be acting as either a
VIEW_CONVERT_EXPR or a BIT_FIELD_REF so the ADDRESSABLE bit does not
need to be set. This case does not need to be factor out since it will
most likely be a register move or extraction from a register.
Thanks,
Andrea
>
> Thanks,
> Andrea
>
> >
> > > Kyrill
> > >
> > > >
> > > > jeff
> > >
> > >
