Hi Jin,
> For now, sharing common matching and control logic seems > simpler and would avoid substantial duplication among vendors. I agree. A shared implementation is the way the go. I simply didn't consider this. > One simple idea is to split the decision into two levels. The first > level would be used by the priority hook. ... > > The second level would be used where both candidate instructions are > available. It would call the complete fusion matcher to check actual > pair constraints, ... Sounds good, I had something similar in mind. E.g. riscv_fuse_mem_p could gate the priority hook and the riscv_fusion_mem_info it returns could contain enough fields to implement the pair matcher in a completely generic way. > Do you have any further thoughts or suggestions? I'm believe that your suggested improvements are sufficient to cover the current designs and it is easily extensible if constraints are missing. Plus it prevents people from potentially forgetting to implement 2 separate hooks to get their loads/stores paired up properly. Thanks, Michiel "Jin Ma" <[email protected]> writes: >> On 9/1/26 10:07 AM, Jin Ma wrote: >> >> On 8/31/26 9:31 AM, Jin Ma wrote: >> >>>> On 8/17/26 12:19 AM, Jin Ma wrote: >> >>>>> The sched_fusion pass can bring independent memory accesses together >> >>>>> before sched2. Add a priority hook that groups scalar load/store-pair >> >>>>> candidates by register file, access kind, mode and base register, then >> >>>>> orders each group by offset. Prefer decreasing offsets when both >> >>>>> directions are available, matching frame save/restore memory order. >> >>>>> >> >>>>> Enable scheduling fusion by default for load/store-pair tunes while >> >>>>> preserving an explicit -f[no-]schedule-fusion option. Keep the >> >>>>> positive >> >>>>> test as XFAIL until the tuning patch enables the pairs. >> >>>>> >> >>>>> Static sched2 macro-fusion counts for SPEC CPU 2017 Integer built with >> >>>>> -O3 -flto, measured independently of ready-list reordering against a >> >>>>> baseline with xt-c9501fdvt fusion pairs enabled: >> >>>>> >> >>>>> Benchmark Baseline With patch Change >> >>>>> 500.perlbench_r 15302 17077 +11.6% >> >>>>> 502.gcc_r 72806 78135 +7.3% >> >>>>> 505.mcf_r 63 85 +34.9% >> >>>>> 520.omnetpp_r 20479 22084 +7.8% >> >>>>> 523.xalancbmk_r 42442 44651 +5.2% >> >>>>> 525.x264_r 6232 7033 +12.9% >> >>>>> 531.deepsjeng_r 299 328 +9.7% >> >>>>> 541.leela_r 794 851 +7.2% >> >>>>> 548.exchange2_r 353 426 +20.7% >> >>>>> 557.xz_r 1217 1321 +8.5% >> >>>>> SUM 159987 171991 +7.5% >> >>>>> >> >>>>> gcc/ChangeLog: >> >>>>> >> >>>>> * config/riscv/riscv-fusion.cc (riscv_sched_fusion_priority): New >> >>>>> function. >> >>>>> * config/riscv/riscv-protos.h (riscv_sched_fusion_priority): >> >>>>> Declare. >> >>>>> * config/riscv/riscv.cc (riscv_override_options_internal): Set the >> >>>>> scheduling-fusion default for the current tune. >> >>>>> (TARGET_SCHED_FUSION_PRIORITY): Define. >> >>>>> >> >>>>> gcc/testsuite/ChangeLog: >> >>>>> >> >>>>> * gcc.target/riscv/sched-fusion-priority.c: New test. >> >>>>> >> >>>>> Signed-off-by: Jin Ma <[email protected]> >> >>>>> + >> >>>>> + isload_p = mem.type != SCHED_FUSION_ST; >> >>>>> + fp_p = mem.fp_p; >> >>>>> + access_size = GET_MODE_SIZE (mem.mode).to_constant (); >> >>>>> + if (access_size != 4 && access_size != 8) >> >>>>> + return; >> >>>> In an ideal world we'd have a way to describe the access size stuff >> >>>> better since that restriction is a function of the uarch and some don't >> >>>> have the access size restrictions. Could we perhaps query which of the >> >>>> fusions are enabled to determine which access sizes should be allowed? >> >>>> >> >>>> More generally it seems like we should test that the two instructions >> >>>> are fusible before we do any priority adjustments. I don't have >> >>>> get_fusible_ops in front of me right now. If it's doing that check, >> >>>> then we're likely good here (and I think that would eliminate the need >> >>>> to test access sizes in this routine since the check that the insns >> >>>> could be fused would be false and we'd exit without any priority >> >>>> adjustments). >> >>>> >> >>>> Jeff >> >>> Hi Jeff, >> >>> >> >>> Thank you for the review. >> >>> >> >>> riscv_get_fusible_ops only returns the fusion mask enabled by the >> >>> current tune. It does not check whether a specific pair of instructions >> >>> can be fused. >> >>> >> >>> Under the current definitions, the access width is part of the semantics >> >>> of these pair operations. RISCV_FUSE_LDST_PAIR_INC/DEC describes 4- or >> >>> 8-byte integer accesses, while RISCV_FUSE_FLDFST_PAIR_INC/DEC describes >> >>> 4- or 8-byte floating-point accesses. Thus, the pair operation and the >> >>> 4/8-byte restriction are currently coupled rather than being independent >> >>> microarchitectural properties. The priority hook already uses the >> >>> enabled operation to select an integer or floating-point pair and its >> >>> direction. The width check ensures that it adjusts only candidates >> >>> supported by that operation. Removing the check would also give byte >> >>> and halfword accesses a pair priority, although the existing pair >> >>> matcher would reject them. >> >> Note that the Veyron V2 can fuse an arbitrary pair of same sized >> >> adjacent stores. What you're doing inhibits that case and fusing of >> >> stores into pairs is one of the most important fusion cases, hence my >> >> focus on getting those right. In fact, I would claim that getting >> >> stores and the address->load cases is all that *really* matters in the >> >> end for DEC based pair fusion. >> >> >> >> Jeff >> > Hi Jeff, >> > >> > Thank you for the clarification. You are right. I am very sorry that >> > I had considered only the newly added RISCV_FUSE_LDST_PAIR_INC/DEC and >> > RISCV_FUSE_FLDFST_PAIR_INC/DEC operations and overlooked the existing >> > RISCV_FUSE_ALIGNED_STD. I will include it in the priority hook and in >> > the set of operations that enable scheduling fusion by default, while >> > retaining the 4/8-byte restriction only for operations that require it. >> > >> > I noticed that Patchwork has marked this patch as Accepted and archived >> > it. Would you prefer that I post a revised version with this change for >> > review, or may I make this adjustment before committing it to trunk >> > without reposting the patch? >> Ugh. I think I may have mis-marked during a late night cleanup. I >> think this one needs further iteration. Michiel's response from a few >> minutes ago would tend to suggest refinement would be helpful as well. >> >> If we have access to both potential insns, then I think we just call >> into the fusion code to ask "will these fuse" rather than any specific >> length check. We've essentially got three meaningfully different >> designs in this space, but if we can make that "will these fuse" query, >> we can support all three relatively easily. >> >> Jeff > > Thank you for the further clarification. > > TARGET_SCHED_FUSION_PRIORITY receives only one instruction per call, so > it cannot directly ask whether two specific instructions can fuse. I > am trying to find a better approach that covers these different designs > without duplicating microarchitectural constraints. > > One simple idea is to split the decision into two levels. The first > level would be used by the priority hook. Since that interface sees > only one instruction at a time, it would determine only whether the > instruction could belong to an enabled fusion operation for the current > tune. This single-instruction eligibility check could be parameterized > by the operation and handle properties already known at that point, > such as load versus store, integer versus floating-point register file, > access mode and width, address form, and extension type. It would not > try to determine relationships between two instructions. > > The second level would be used where both candidate instructions are > available. It would call the complete fusion matcher to check actual > pair constraints, such as a common base, adjacent offsets, increasing > or decreasing order, pair alignment, and register relationships. To > avoid maintaining two sets of rules that could drift apart, the > priority classification and final matcher could share the same > operation-specific single-instruction eligibility check, with the full > matcher adding only the constraints between the two instructions. > > This would allow Veyron V2, C950, and ARC-V to retain their respective > access-width and other restrictions without applying the restrictions > of one design globally to the others. Existing fusion operations would > be reused where they can express the required behavior, and a new > definition would be added only when an important difference cannot be > represented otherwise. This idea is not final yet. I will continue > checking whether it can support these three designs cleanly and share > the common logic with Michiel's implementation. > > I am not sure whether this accurately reflects your suggestion. Do you > have any further comments or guidance? > > Thanks, > Jin
