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
