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.
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
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
AndyAyersMS
left a comment
There was a problem hiding this comment.
Not very familiar with this code, but this looks reasonable.
This revives #81242 on top of #125214. I noticed some regressions with #125214 because we were missing this logic (specifically case 5 when resolving DU conflicts). Also, this may have some TP implications that I want to look at separately from the other change. Example: ```diff G_M40772_IG01: - sub rsp, 40 - xor eax, eax - mov qword ptr [rsp+0x20], rax - ;; size=11 bbWeight=1 PerfScore 1.50 + push rbx + sub rsp, 32 + ;; size=5 bbWeight=1 PerfScore 1.25 G_M40772_IG02: call [CORINFO_HELP_READYTORUN_GCSTATIC_BASE] - mov bword ptr [rsp+0x20], rax + mov rbx, rax call [System.Reflection.Internal.PooledStringBuilder:CreatePool():System.Reflection.Internal.ObjectPool`1[System.Reflection.Internal.PooledStringBuilder]] - mov rcx, bword ptr [rsp+0x20] + mov rcx, rbx mov rdx, rax call [CORINFO_HELP_ASSIGN_REF] nop - ;; size=32 bbWeight=1 PerfScore 11.50 + ;; size=28 bbWeight=1 PerfScore 10.00 G_M40772_IG03: - add rsp, 40 + add rsp, 32 + pop rbx ret - ;; size=5 bbWeight=1 PerfScore 1.25 + ;; size=6 bbWeight=1 PerfScore 1.75 -; Total bytes of code 48, prolog size 11, PerfScore 14.25, instruction count 12, allocated bytes for code 48 (MethodHash=bb6e60bb) for method System.Reflection.Internal.PooledStringBuilder:.cctor() (FullOpts) +; Total bytes of code 39, prolog size 5, PerfScore 13.00, instruction count 12, allocated bytes for code 39 (MethodHash=bb6e60bb) for method System.Reflection.Internal.PooledStringBuilder:.cctor() (FullOpts) ; ============================================================ ```
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.