[TableGen] checkSubRegIndexSizes only validates DefaultMode; sub-register overflow in other HwModes is missed
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
Follow-up to #206346, which added `CodeGenRegister::checkSubRegIndexSizes` — a build-time check that a `CoveredBySubRegs` register's explicit `SubRegIndex`es do not tile past the register's own size.
The check reads the register size, the class value type, and the sub-register ranges at `DefaultMode` only, and ignores every other HwMode. Two gaps follow:
1. A register whose class has a **HwMode-varying value type** is skipped entirely (via the `!VT.isSimple()` guard), even though its per-mode size is well defined.
2. An overflow that exists **only in a non-default HwMode** is not diagnosed, because only the `DefaultMode` geometry is validated.
### Repro (gap 2)
`EX0` tiles exactly in `DefaultMode` (`[0,32) + [32,64) = 64`) but overflows in `TestMode` (`[0,32) + [32,96) = 96 > 64`). `llvm-tblgen -gen-register-info` accepts it silently:
```
include "llvm/Target/Target.td"
def HasFeat : Predicate<"Subtarget->hasFeat()">;
def TestMode : HwMode<[HasFeat]>;
class MyReg subs = []> : Register {
let Namespace = "Test"; let SubRegs = subs; let CoveredBySubRegs = 1;
}
class MyClass types, dag regs>
: RegisterClass<"Test", types, sz, regs> { let Size = sz; }
def sub_lo : SubRegIndex<32, 0> {
let SubRegRanges = SubRegRangeByHwMode<[DefaultMode, TestMode],
[SubRegRange<32, 0>, SubRegRange<32, 0>]>;
}
// DefaultMode: 32 @ 32 -> [32,64), exact. TestMode: 64 @ 32 -> [32,96), overflow.
def sub_hi : SubRegIndex<32, 32> {
let SubRegRanges = SubRegRangeByHwMode<[DefaultMode, TestMode],
[SubRegRange<32, 32>, SubRegRange<64, 32>]>;
}
def LO0 : MyReg<"lo0">;
def HI0 : MyReg<"hi0">;
def LORegs : MyClass<32, [i32], (add LO0)>;
def HIRegs : MyClass<32, [i32], (add HI0)>;
let SubRegIndices = [sub_lo, sub_hi] in
def EX0 : MyReg<"ex0", [LO0, HI0]>;
let RegInfos = RegInfoByHwMode<[DefaultMode, TestMode],
[RegInfo<64,64,64>, RegInfo<64,64,64>]> in
def EXRC : MyClass<64, [i64], (add EX0)>;
def TestTarget : Target;
```
### Candidate fix
Iterate over every HwMode instead of just `DefaultMode`: resolve the size, value type, and ranges with `get(Mode)`, and name the offending mode in the diagnostic. I have this implemented locally. It keeps the `DefaultMode` diagnostic byte-identical, catches the repro above (`register 'EX0' has size 64 but its explicit sub-registers cover 96 bits in hardware mode 'TestMode'`), and produces no new diagnostics across every in-tree target (`-gen-register-info` over all backends, including the multi-mode AMDGPU / X86 / RISC-V / ARM).
### The actual question
No in-tree target hits this today: the obvious candidate, RISC-V's `GPRPair`, is `untyped`, so it is skipped for an unrelated reason. So this is forward-looking hardening, not a live bug.
Given that, is the all-HwMode generalization something you would want, or is the `DefaultMode`-only scope intentional? Happy to send a PR if it is wanted.
cc @jurahul (merged the check), @topperc (added HwMode support for sub-register index size/offset in #86368)
Contributor guide
Research direction
Start at CodeGenRegister::checkSubRegIndexSizes and inspect how it currently resolves the register size, value type, and sub-register ranges for DefaultMode. Run the provided TableGen reproducer with llvm-tblgen -gen-register-info; done means validating every HwMode, including the TestMode overflow, while preserving the existing DefaultMode diagnostic and naming the offending mode.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100