JIT: Avoid changing def registers that may assume an implicit kill#127184
JIT: Avoid changing def registers that may assume an implicit kill#127184jakobbotsch wants to merge 1 commit intodotnet:mainfrom
Conversation
LSRA build has places where it assumes that `BuildDef(tree, SRBM_REG)` means that `SRBM_REG` is going to be spilled if something is in it, without constructing any explicit kill. This assumption is incompatible with the logic in `resolveConflictingDefAndUse` that changes the def register on fixed reg definitions -- the required spilling will happen only if there ends up being a def into that register. We could fix all the places that assume this and add explicit kills, but instead this change just avoids changing the def assignment away from the register when it detects there is an active interval assigned to the fixed def register.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Adjusts LSRA’s conflicting def/use resolution to avoid changing the assigned register of fixed-reg defs when doing so would prevent an implicit spill of a currently-occupied fixed register. This targets stress-induced miscompilations/test failures reported in #127130 and #127131.
Changes:
- Introduce a
canChangeDefgate to prevent def-reg reassignment for multireg nodes. - Further restrict def-reg reassignment for fixed-reg defs when the fixed register currently holds an active interval (ensuring the expected spill occurs).
| // Avoid changing the def reg away from its assignment if that register is | ||
| // currently busy. The reason is that we have a number of places in LSRA | ||
| // that assume that BuildDef(tree, SRBM_REG) means that SRBM_REG will be | ||
| // spilled if necessary, so they do not bother to kill SRBM_REG explicitly. | ||
| // However, that spilling happens only if we actually assign the def to | ||
| // SRBM_REG. | ||
| // A slightly better way would be to also unassign the fixed reg always, | ||
| // but this is mostly a stress-only case because the presence of the fixed | ||
| // reg on the def should make most intervals prefer not to be allocated to | ||
| // that register. | ||
| if (canChangeDef && defRefPosition->isFixedRegRef) | ||
| { | ||
| RegRecord* defRegRecord = getRegisterRecord(defRefPosition->assignedReg()); | ||
| canChangeDef = (defRegRecord->assignedInterval == nullptr) || !defRegRecord->assignedInterval->isActive; | ||
| } |
There was a problem hiding this comment.
The new comment says to avoid changing the def reg when the fixed register is "currently busy", but the actual condition only checks whether the register has an assigned active interval (it ignores regsBusyUntilKill/regsInUseThisLocation). Consider rewording the comment to match the implemented condition (e.g., “occupied by an active interval”) to avoid future confusion, or expand the check if true “busy” was intended.
LSRA build has places where it assumes that
BuildDef(tree, SRBM_REG)means thatSRBM_REGis going to be spilled if something is in it, without constructing any explicit kill. This assumption is incompatible with the logic inresolveConflictingDefAndUsethat changes the def register on fixed reg definitions -- the required spilling will happen only if there ends up being a def into that register.We could fix all the places that assume this and add explicit kills, but instead this change just avoids changing the def assignment away from the register when it detects there is an active interval assigned to the fixed def register.
Fix #127130
Fix #127131