Remove regNumberSmall and just use regNumber everywhere#127542
Remove regNumberSmall and just use regNumber everywhere#127542tannergooding wants to merge 3 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR removes the separate regNumberSmall type and standardizes on regNumber throughout the JIT by choosing an appropriate underlying type for regNumber per-target (e.g., uint8_t for most native targets, wider for WASM). It also updates xarch emitter address-mode storage/access to avoid relying on bitfield enum types directly.
Changes:
- Introduces
regNumberBase_t/regMaskBase_tand redefinesregNumber/_regMask_enumto use those sized underlying types. - Replaces remaining
regNumberSmallstorage/usages across LSRA, IR nodes (GenTree), ABI passing data, and GC encoding. - Refactors xarch
emitAddrMode(and related emitter reg fields) to store raw unsigned bitfields with typed getter/setter accessors, updating call sites accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/target.h | Defines per-target underlying types for regNumber/reg masks and removes regNumberSmall. |
| src/coreclr/jit/registeropswasm.cpp | Switches WASM register encoding helpers to use the new base underlying type. |
| src/coreclr/jit/lsra.h | Updates var-to-reg map typing to use regNumber*. |
| src/coreclr/jit/lsra.cpp | Updates LSRA maps/assignments to store regNumber directly instead of regNumberSmall. |
| src/coreclr/jit/gentree.h | Stores assigned regs and multi-reg arrays as regNumber. |
| src/coreclr/jit/gcencode.cpp | Adjusts a fit-check assert after removing regNumberSmall. |
| src/coreclr/jit/emitxarch.cpp | Updates address-mode field access to use new accessor methods. |
| src/coreclr/jit/emitinl.h | Updates displacement reads to use amDisp() accessor. |
| src/coreclr/jit/emit.h | Refactors xarch address mode + instrDesc reg bitfields to unsigned storage with typed accessors. |
| src/coreclr/jit/emit.cpp | Updates call-indirect displacement storage to use amDisp(...) accessor. |
| src/coreclr/jit/compiler.h | Switches local-var reg fields to regNumber and removes regNumberSmall. |
| src/coreclr/jit/clrjit.natvis | Updates natvis display to account for reg fields now being stored as unsigned. |
| src/coreclr/jit/abi.h | Stores ABI-passing register as regNumber. |
| src/coreclr/jit/abi.cpp | Updates ABI-passing segment register assignment after type unification. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/compiler.h:788
- The comment above these accessors appears outdated after switching to
regNumber: it says the register is stored in an 8-bit field and the getters/setters use a full-size unsigned type to localize casts. With_lvRegNumnow beingregNumberand the accessors also usingregNumber, this is no longer accurate and the rationale for the casts has changed. Please update or remove the comment so it reflects the current representation (including the TARGET_WASM case whereregNumberis wider).
// The register number is stored in a small format (8 bits), but the getters return and the setters take
// a full-size (unsigned) format, to localize the casts here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/jit/compiler.h:789
- The comment here still describes an 8-bit “small format” storage with casts to/from a “full-size (unsigned)” reg representation, but the fields are now
regNumberand the accessors take/returnregNumber. Consider updating/removing this comment and dropping the now-redundant casts in the getters/setters to avoid implying truncation that can no longer occur.
regNumber _lvArgInitReg; // the register into which the argument is moved at entry
public:
// The register number is stored in a small format (8 bits), but the getters return and the setters take
// a full-size (unsigned) format, to localize the casts here.
/////////////////////
|
|
||
| private: | ||
| // This stores the register assigned to the node. If a register is not assigned, _gtRegNum is set to REG_NA. | ||
| regNumberSmall _gtRegNum; | ||
| regNumber _gtRegNum; | ||
|
|
||
| // Count of operands. Used *only* by GenTreeMultiOp, exists solely due to padding constraints. | ||
| friend struct GenTreeMultiOp; |
There was a problem hiding this comment.
Now that _gtRegNum is stored as regNumber (not a separate “small” type), the nearby comment that says the reg is stored in an 8-bit format and that getters/setters use a full-size unsigned format is misleading. Consider updating/removing that comment and simplifying any redundant casts in the reg accessors so it’s clear there’s no truncation happening here anymore.
|
Provides a good improvement to bytes allocated:
But a large regression to throughput:
|
This follows up on a TODO to see if
regNumberandregNumberSmallare actually needed or if we can just type the enum to the right size and use it everywhere.