Re: [PATCH v3 4/5] RISC-V: Add macro-fusion pair recognition

On 7/22/2026 2:17 AM, Jin Ma wrote:
> Hi Jeff,
>
> Thank you for the review.
>
> I understand the concern.  These new pairs are in the same broad
> categories as existing fusions, but their register dependencies and
> accepted RTL forms are not the same.
We can probably refine them in such a way as to support both reasonably 
well.  I'm certainly not going to lose any sleep if we lose fusions on 
the Veyron V1 design (for example the store pair commit which is modeled 
as fusion) and probably not if we lose Veyron V2 fusions.  The fusions 
are meant to be fairly generic and if we over-fuse slightly or miss some 
cases I just don't think it's going to be a big deal.

>
>> RISCV_FUSE_ADD_LD - add type + integer load
>> RISCV_FUSE_ADD_ST - add type + integer store
>> Aren't these already handled by existing cases?
> RISCV_FUSE_LDINDEXED and RISCV_FUSE_EXPANDED_LD both require the add
> destination and the load destination to be the same register.  The new
> RISCV_FUSE_ADD_LD instead requires these registers to be different, so
> the key register relationships are opposite.  There is no corresponding
> existing add-and-store case, and the new recognizers also need to handle
> addw and add.uw.
That's interesting and I'd suggest you double check with your hardware team.

If the destination of the add is different than the destination of the 
load, then don't you have to generate *both* values in your register 
file?  That would be a surprise if you're doing a traditional DEC based 
pairwise fusion.  THe big advantage of requiring both destinations to be 
the same as you know the result of the add is used for the memory 
address computation and never again. Thus you fuse the two simple ops 
into a single more complex op for the execution units.

> I do not understand why the existing load fusion requires the load base
> and destination to be the same register.  From a general dependency
> perspective, this constraint seems unusual to me.  However, it may
> reflect the requirements of another vendor's microarchitecture.  Without
> the relevant hardware specification or confirmation from that vendor, I
> do not think it is appropriate to change or relax the existing
> semantics.
See above :-)  By forcing the same destination register you know the 
result of the add is only used for the memory computation and need not 
ever be computed into an architectural or physical register. You fuse 
the add+ld into a single op during decoding and pass that more complex 
op off to your schedulers/execution ports.

If the destinations are different, then the fusion engine doesn't know 
if the result of the add ever gets used.  So you can still fuse, but you 
have to send both operations into your schedulers/execution units.  
  That would reduce the dependency height of the operations allowing 
them to run in parallel, but the add still has to take up execution 
resources.

The Veyron V2 design can do both styles, the first style is done in the 
traditional way in the decoder.  Simple pairwise fusion.  The second 
style is handled completely differently and is the subject of one or 
more issued patents.  We've never tried to model any of the more complex 
fusion cases covered by those patents in GCC for multiple reasons, 
including the fact that it didn't seem likely to meaningfully improve 
performance.

If you've double checked with your design team that you can indeed fuse 
those cases, then I won't object to supporting both styles. It'd just be 
a bit of a surprise.


>
>> RISCV_FUSE_SLLI_SRLI - slli/slliw + srli/srliw
>> Similarly.  These are bitfield extractions.
> For non-word slli + srli, RISCV_FUSE_SLLI_SRLI does overlap with
> RISCV_FUSE_BFEXT.  However, RISCV_FUSE_BFEXT also recognizes
> slli + srai, and it does not fully recognize the extension-wrapped RTL
> used for slliw + srliw.  C950 supports only the logical-right-shift
> form, and requires the two instructions to be either both word
> operations or both non-word operations.
>
> For example, RISCV_FUSE_BFEXT also accepts:
>
>    (set (reg rd1) (ashift (reg rs1) (const_int shamt1)))
>    (set (reg rd1) (ashiftrt (reg rd1) (const_int shamt2)))
>
> The new pair also needs to recognize the following word RTL:
>
>    (set (reg:DI rd1)
>         (sign_extend:DI
>           (ashift:SI (reg:SI rs1) (const_int shamt1))))
>    (set (reg:DI rd1)
>         (zero_extend:DI
>           (lshiftrt:SI (reg:SI rd1) (const_int shamt2))))
>
> Enabling RISCV_FUSE_BFEXT directly would therefore report slli + srai,
> which is not in the C950 specification, while missing the word form.
> This is the most substantial overlap among these cases.  The common part
> is the slli + srli RTL matching; the differences are the word form and
> whether an arithmetic right shift is allowed.
So can we break things down and re-compose them?  So for example, sign 
and zero extended bitfields as well as sign/zero extended from the SI 
sign bit?  Maybe you're already doing this.

>
>> RISCV_FUSE_PREINDEX_LD - addi type + load
>> RISCV_FUSE_PREINDEX_ST - addi type + store
>> Again, feels like this likely mirror existing functionality.
> RISCV_FUSE_LDPREINCREMENT supports only integer loads and requires the
> updated base register and the load destination to be the same register.
> The new pre-index load requires them to be different.  The new cases
> also cover stores, floating-point loads and stores, self-moves, and
> lo_sum forms.  Thus, the principal register relationship for the
> existing and new load forms is again opposite.
lo-sum forms are no different than other immediate forms once you get 
down to binary.  So extending to cover them would naturally help anyone.

THe FP cases are definitely new.  So again, the idea would probably be 
to break them down and compose the currently supported cases from 
simpler cases.


>
> For example:
>
>    Existing LDPREINCREMENT:
>      (set (reg rd1) (plus (reg rd1) (const_int imm12)))
>      (set (reg rd1) (mem (reg rd1)))
>
>    New PREINDEX_LD:
>      (set (reg rd1) (plus (reg rd1) (const_int imm12)))
>      (set (reg rd2) (mem (reg rd1)))
>      where rd1 != rd2.
See above.  You should really double check with your design team since 
fusing the second case here means you have to generate the value in rd1 
and rd2.  Fusion does allow the second insn to execute in parallel with 
the first though.

> There is real overlap here: pair-aligned SI/DI store pairs may be
> accepted by both an existing recognizer and a new recognizer.  However,
> their full accepted sets differ.
>
> RISCV_FUSE_CACHE_ALIGNED_STD handles only DI store pairs.  It requires
> the base to be known to have at least 16-byte alignment, requires the
> smaller offset to be 16-byte aligned, and normalizes the offset order.
> RISCV_FUSE_ALIGNED_STD also handles only store pairs, but accepts
> same-width stores in scalar integer modes.  It requires the smaller
> offset to be a multiple of twice the access size and is also insensitive
> to instruction order.
I'd suggest dropping the older case (CACHE_ALIGNED_STD).  That was a 
Veyron V1 item and I haven't heard of anyone doing anything similar.

Basically the V1 design could commit full stores to opposite sides of a 
cache line together.   It's was a neat trick to improve commit/retire 
performance without doing full store fusion in the decoder.  Given the 
V1 design is never going to silicon, I wouldn't lose any sleep if it 
were just dropped.




>
> RISCV_FUSE_LDST_PAIR_INC and RISCV_FUSE_LDST_PAIR_DEC handle both loads
> and stores, accept only 4-byte and 8-byte accesses, do not require pair
> alignment, and preserve ascending and descending instruction order as
> separate capabilities.  For loads, they also reject zero-extending forms
> and enforce destination and base-register constraints.
>
> For example, these two SI loads form a valid ascending pair, although
> the smaller offset is 4 and is not aligned to the 8-byte pair boundary:
>
>    (set (reg:SI rd1)
>         (mem:SI (plus (reg:DI rs1) (const_int 4))))
>    (set (reg:SI rd2)
>         (mem:SI (plus (reg:DI rs1) (const_int 8))))
>
> Here rd1 and rd2 are nonzero and distinct, and rd1 != rs1.
>
> Likewise, only the new ascending checker accepts this SI store pair:
>
>    (set (mem:SI (plus (reg:DI rs1) (const_int 4)))
>         (reg:SI rs2))
>    (set (mem:SI (plus (reg:DI rs1) (const_int 8)))
>         (reg:SI rs3))
>
> The descending pair contains the same addresses in offset-8, offset-4
> order.
>
> Enabling RISCV_FUSE_ALIGNED_STD directly would not only miss load pairs
> and SI/DI pairs without pair alignment, but would also overmatch byte
> and halfword store pairs that C950 does not support.  Conversely,
> broadening the existing checker would give a model that is too broad
> for CPUs that support only its original width or alignment constraints.
I suspect this is one of the cases where we probably do need different 
recognizers now that you've explained what your design does.  We found 
true fusion of store pairs to meaningfully improve performance.

> The current approach does not allocate an independent bit for every
> difference.  It reuses an existing capability when the differences are
> acceptable.  For example, the C950 definitions of LUI/AUIPC + ADDI type
> are not identical to the existing RISCV_FUSE_LUI_ADDI and
> RISCV_FUSE_AUIPC_ADDI definitions.  After evaluating the RTL accepted by
> the existing checkers and the resulting impact, those differences were
> considered acceptable, so the existing bits are reused here.
I think part of the misunderstanding here is that the existing fusions 
are fixed.  They are not in my mind.  If we want to revamp them by 
either restricting more generalizing them we can.  I don't think many 
(if any) designs on the trunk are using those fusions. So we have 
flexibility here to refine them in ways that we think are a better match 
for designs that are coming onto the market now and in the near future.  
What's on the trunk represents an initial stab at that goal, but I think 
we have room to adjust what's on the trunk.

>
> On the other hand, further microarchitectures may have additional subtle
> differences that cannot be ignored and need independent representation.
> The preceding patch widened the fusion bitmask to unsigned HOST_WIDE_INT
> so that the current width would not limit how such capabilities can be
> represented.  The wider bitmask does not itself determine how finely the
> capabilities should be divided.
True, but we also want to get something that is maintainable.  It 
doesn't have to perfectly match every design.  It just has to do a good job.

Jeff