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.
> 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.
A minimal RTL comparison is:
Existing load fusion:
(set (reg rd1) (plus (reg rs1) (reg rs2)))
(set (reg rd1) (mem (reg rd1)))
New ADD_LD:
(set (reg rd1) (plus (reg rs1) (reg rs2)))
(set (reg rd2) (mem (reg rd1)))
where rd1 != rd2.
New ADD_ST:
(set (reg rd1) (plus (reg rs1) (reg rs2)))
(set (mem (reg rd1)) (reg rs3))
where rd1 != rs3.
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.
Enabling the existing load fusions for C950 would therefore accept a
same-destination form that C950 does not support, while missing the
distinct-destination form that it does support.
> 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.
> 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.
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.
New PREINDEX_ST:
(set (reg rd1) (plus (reg rd1) (const_int imm12)))
(set (mem (reg rd1)) (reg rs1))
where rd1 != rs1.
> RISCV_FUSE_LDST_PAIR_INC - ascending integer load/store pair
> RISCV_FUSE_LDST_PAIR_DEC - descending integer load/store pair
> And these as well.
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.
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.
> The design goal here is the enum describes a set of fairly generic
> fusion opportunities, then each core sets those it supports. Having
> multiple implementations of what is effectively the same fusion case
> seems undesirable, hence my concern about defining fusions that I
> think already exist.
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.
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.
The purpose of a new bit is not to make the accepted sets of different
checkers disjoint. It is to represent a vendor-neutral set of hardware
constraints that an existing bit cannot model accurately. A new bit is
appropriate only when that difference forms an independently definable
capability that may also be reusable by other CPUs. Some RTL overlap
between checkers is acceptable. The one-bit-per-checker relationship is
intended only to keep enablement, recognition, and the dump name in a
clear one-to-one relationship, not to partition the RTL space into
disjoint sets.
Shared RTL parsing, register-dependency checks, and address checks are a
separate issue from whether the capabilities themselves are independent.
What I am unsure about is how to choose the right capability boundary
for these subtle but real differences between microarchitectures. If
a generic fusion bit is too broad, only CPUs supporting all of its RTL
forms can safely enable it. If every subtle difference receives an
independent bit, the definitions become fragmented and less reusable.
I have not found an ideal way to balance accurate modeling with generic
reuse. Could you suggest what abstraction GCC would prefer in this
case, or whether there is a better way to represent these constraint
differences between microarchitectures?
BR,
Jin
