> 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? Thanks, Jin
