On Thu, Jun 11, 2026 at 5:26 PM Jeffrey Law <[email protected]> wrote: > > > > On 6/10/2026 1:10 PM, [email protected] wrote: > > From: Kyrylo Tkachov <[email protected]> > > > > In the sink pass, if-convert a diamond that selects between two loads at > > data-dependent addresses (PHI <*P, *Q>) into a single reg-offset load behind > > branchless selects, reusing phi-opt's factoring helpers > > (factor_out_conditional_load from the previous patch and > > factor_out_conditional_operation, made non-static here) to common the load > > and its address. Guarded so that outside any loop it always fires (nothing > > to vectorise), > > and in a loop only on non-vectorisable load-address recurrences, leaving > > affine vectorisable loops alone. This was needed to avoid regressing the > > marian > > benchmark in SPEC2026 that otherwise ended up using expensive gathers to > > vectorise. We can look to relax this condition in the future for cases > > where > > we think it's beneficial. > > > > The load is commoned only when both arms are guaranteed to reduce to pure > > speculatable scalar (scc_arms_speculatable_p), so the branchless finish > > always > > succeeds and no commoned-but-still-branching diamond is ever left behind. > > The > > recurrence walk is bounded by a new --param=sink-diamond-recurrence-limit > > (default 256). > > > > With this patch the Snappy hot loop gets if-converted optimally and > > performance > > improves by 25% on my aarch64 machine, making it a bit better than what LLVM > > gets today. > > > > The code goes from: > > .L8: ; ---type==0 arm > > lsr x3, x1, 2 ; tag >> 2 > > add x1, x0, x3 > > add x3, x3, 2 > > add x0, x0, x3 ; ip += (tag>>2)+2 > > ldrb w1, [x1, 1] ; LOAD #1: tag = ip[(tag>>2)+1] > > cmp x2, x0 > > bls .L2 > > .L5: ; loop head / else arm > > ands x3, x1, 3 ; type = tag & 3 > > beq .L8 ; <-- DATA-DEPENDENT BRANCH (mispredicts) > > ldrb w1, [x0, x3] ; LOAD #2: tag = ip[type] > > add w3, w3, 1 > > add x0, x0, x3 ; ip += type+1 > > cmp x2, x0 > > bhi .L5 > > > > to: > > .L3: ; --- single loop body, no diamond branch > > lsr x4, x1, 2 ; tag >> 2 > > ands x3, x1, 3 ; type = tag & 3 (Z = type==0) > > csinc x1, x3, x4, ne ; offset = (type!=0) ? type : (tag>>2)+1 > > ldrb w1, [x0, x1] ; ONE reg-offset LOAD: tag = ip[offset] > > add x4, x4, 2 > > csinc x3, x4, x3, eq ; advance = (type==0) ? (tag>>2)+2 : > > type+1 > > add x0, x0, x3 ; ip += advance > > cmp x2, x0 > > bhi .L3 > > > > crucially avoiding the badly-predicted branch. > Yea. Thanks for doing this. I've got a long standing TODO to look at > snappy performance for RISC-V. > > > > > > Bootstrapped and tested on aarch64-none-linux-gnu. > > > > Signed-off-by: Kyrylo Tkachov <[email protected]> > > > > gcc/ > > PR tree-optimization/125557 > > * params.opt (sink-diamond-recurrence-limit): New param. > > * doc/params.texi (sink-diamond-recurrence-limit): Document it. > > * tree-ssa-phiopt.h (factor_out_conditional_operation): Declare. > > * tree-ssa-phiopt.cc (factor_out_conditional_operation): Remove > > static. > > * tree-ssa-sink.cc (scc_recurrence_p, scc_arms_speculatable_p) > > (scc_try_ifconvert, sink_common_computations_to_bb): New. > > (pass_sink_code::execute): Call it at the early sink. > > > > gcc/testsuite/ > > PR tree-optimization/125557 > > * gcc.dg/tree-ssa/scc-diamond-1.c: New test. > > * gcc.dg/tree-ssa/scc-diamond-3.c: New test. > > * gcc.target/aarch64/scc-diamond-2.c: New test. > > > > > > diff --git a/gcc/tree-ssa-sink.cc b/gcc/tree-ssa-sink.cc > > index 2c6cad2687c..bc739ce6ea1 100644 > > --- a/gcc/tree-ssa-sink.cc > > +++ b/gcc/tree-ssa-sink.cc > > @@ -513,6 +518,368 @@ statement_sink_location (gimple *stmt, basic_block > > frombb, > > return true; > > } > > > > +/* If-convert a data-dependent load diamond into a single reg-offset load. > > + At a diamond merge the conditional load (and the operations feeding its > > + address) are commoned by reusing phi-opt's factoring helpers: > > + factor_out_conditional_load turns PHI <*P, *Q> into a selected-pointer > > load, > > + then factor_out_conditional_operation pulls the common operations out, > > + leaving one PHI selecting the differing offset. > > + scc_try_ifconvert then removes the branch (branchless selects + a single > > + reg-offset load). scc_recurrence_p guards it so it only fires on > > + non-vectorisable load-address recurrences (e.g. a tag-byte pointer > > + chase) and leaves affine, vectorisable loops alone. Run at the early > > sink, > > + after phi-prop so phi-prop cannot undo it. > > + > > + Having commoned a load in the clean if/else diamond HEAD -> {ARM0,ARM1} > > -> > > + JOIN (E0/E1 the arm->join edges), finish branchlessly: DCE the now-dead > > arm > > + loads, hoist the remaining pure arm arithmetic into HEAD, and turn every > > + JOIN PHI into a COND_EXPR select -- removing the data-dependent branch > > (the > > + actual win; merely commoning the load while keeping the branch > > regresses). > > + Only reached for load-commoned diamonds, cfg-cleanup later folds the > > emptied > > + diamond. */ > > +static bool > > +scc_try_ifconvert (basic_block head, basic_block arm0, basic_block arm1, > > + basic_block join, edge e0, edge e1) > > +{ > > + > > + > > + /* Both arms must now be pure scalar computation we can speculate. */ > > + for (int i = 0; i < 2; i++) > So I missed you were doing this when reviewing patch#1 of this series. > Given you've got the infrastructure to pull statements out of the arms > into the head of the diamond, you *could* handle nonzero offsets. You > just shove temp = <address arithmetic> into the appropriate arm for a > nonzero offset and let this code do its thing. Obviously not necessary > for this stage, but perhaps worth experimentation to see how often it > would fire and if it looks meaningful implement as a follow-up. > > I may have missed it, but do you ensure that nothing in the arms > references the SSA_NAME(s) in the controlling branch condition? > > Generally OK. Give others a bit of time (say until COB Monday?) time to > chime in, but I'm comfortable with this patch as well.
I am not comfortable with the heuristics used here for sink. This is why I have not been heavily reviewing that part of the patch yet. There has to be a better way of handling this. Maybe the better heuristics is just handling this for the last sink rather than early sink. There has to have some better way of handling the heuristics rather than walking backwards. I thought there is a way to tell if ssa name is based on an induction variable or not. Which is way cheaper than what is done in this patch. Thanks, Andrea > > jeff
