llvm / llvm/llvm-project

[TableGen] checkSubRegIndexSizes only validates DefaultMode; sub-register overflow in other HwModes is missed

Open
#210,532 6 comments 0 reactions 0 assignees View on GitHub
tablegen
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.