On Fri, 12 Jun 2026, Andrew Pinski wrote: > 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.
We should definitely avoid making auto_var_in_fn_p variables address-taken when they were not already. There might be an argument that we should not for not address-taken local binding vars either (those which can have !may_be_aliased). Testing !may_be_aliased could be one way to check, but consider that phiprop (the reverse transformation) is also done to get all address-takens go away, so a greedy local check might fight against this, esp. when done too early in the optimization pipeline. Richard. > Thanks, > Andrea > > > > > Thanks, > > Andrea > > > > > > > > > Kyrill > > > > > > > > > > > > > > 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)
