JIT: Handle some def/use conflicts less conservatively in LSRA#125214
JIT: Handle some def/use conflicts less conservatively in LSRA#125214jakobbotsch merged 7 commits intodotnet:mainfrom
Conversation
A def-use conflict happens for SDSU intervals when the def and use share no common register. For these cases we run through a set of potential ways to resolve the conflict that make use of the contract between LSRA and codegen. For example, when the definition requires a single fixed register, it is still valid for LSRA to assign a different register. This means that codegen can use the original fixed register, but that it must insert a copy into the register assigned by LSRA. In the worst case we cannot use any of the approaches to solve the conflict and then we will fall back to spilling/copying as necessary. It appears that several of the cases were overly conservative. Particularly case 1 and 2, as described by the function header, were never reachable; we assign `defRegConflict` and `useRegConflict` based on whether there is a conflict in register constraints, but that is always true when we get into this function. It appears the original intention of these variables were whether there is a conflict with a _different_ use of the registers we could otherwise have resolved the original conflict with. This PR restores the meaning to that.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR fixes an over-conservative bug in LinearScan::resolveConflictingDefAndUse in the JIT's LSRA (Linear Scan Register Allocator). When a single-def, single-use interval has conflicting register requirements between its definition and use, LSRA tries several resolution strategies (Cases 1–6) before falling back to spilling/copying. The two guard variables defRegConflict and useRegConflict were incorrectly initialized to true (based on the def/use set disjointness — which is always true at the call site), making Cases 1 and 2 entirely unreachable dead code. The PR restores their intended semantics by initializing them to false, so they instead track whether a specific candidate register has an intervening conflict elsewhere — only being set to true inside the blocks when such a conflict is actually detected.
Changes:
- Initialize
defRegConflictanduseRegConflicttofalseinstead of deriving them from the always-true def/use register set disjointness. - Remove the redundant
&& !defRegConflict/&& !useRegConflictguards from theisFixedRegRefchecks (now replaced by the corrected initialization).
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
cc @dotnet/jit-contrib. @AndyAyersMS can you take a look? Diffs. Regressions will mostly be addressed by #125219. Large improvements for arm32 and this PR should supersede #125178. |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Not very familiar with this code, but this looks reasonable.
A def-use conflict happens for SDSU intervals when the def and use share no common candidate register. For these cases we run through a set of potential ways to resolve the conflict that make use of the contract between LSRA and codegen. For example, when the definition requires a single fixed register, it is still valid for LSRA to assign a different register. This means that codegen can use the original fixed register, but that it must insert a copy into the register assigned by LSRA.
In the worst case we cannot use any of the approaches to solve the conflict and then we will fall back to spilling/copying as necessary.
It appears that several of the cases were overly conservative. Particularly case 1 and 2, as described by the function header, were never reachable; we assign
defRegConflictanduseRegConflictbased on whether there is a conflict in register constraints, but that is always true when we get into this function. It appears the original intention of these variables were whether there is a conflict with a different use of the registers we could otherwise have resolved the original conflict with. This PR restores the meaning to that.