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. Thanks, Andrea > > > Kyrill > > > > > > > > jeff > > > >
0001-phiopt-reorganize-factoring-cselim-limited-for-phiop.patch
Description: Binary data
0002-phiopt-Simplified-factoring-of-loads-PR125557.patch
Description: Binary data
