JIT: Avoid resolving def-use conflicts by changing use registers#125333
JIT: Avoid resolving def-use conflicts by changing use registers#125333jakobbotsch wants to merge 10 commits intodotnet:mainfrom
Conversation
The only benefit is that it saves insertion of some explicit copies, but the trade off is an implicit contract between LSRA and codegen that is not very faithful. When LSRA defines in a different register than the constrained register, it believes that codegen can insert a copy and simultaneously use it from the new location, which is not true.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR refactors LinearScan::resolveConflictingDefAndUse in the JIT's register allocator (LSRA) to eliminate the practice of changing use-side register assignments when resolving def-use conflicts. The change ensures that only the definition RefPosition's register assignment is ever modified, while explicit copies are inserted for cases where the use constraint cannot be satisfied. This removes a fragile implicit contract between LSRA and codegen where LSRA would assign a def to a different register than constrained and expect codegen to simultaneously insert a copy and use from the new location.
Changes:
- Simplifies
resolveConflictingDefAndUseinlsrabuild.cppby removing old Cases 1 and 4 (which changed the USE register assignment), consolidating the remaining cases into four cleaner ones that only modify the DEF's register assignment. - Updates the
LsraDumpEventenum inlsra.hto replace numbered case events (CASE1–CASE6) with descriptive names. - Updates the debug dump handler in
lsra.cppto use the new event names and removes the now-deadLSRA_EVENT_DEFUSE_FIXED_DELAY_USEno-op case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/coreclr/jit/lsrabuild.cpp |
Core logic change: removes use-register-changing cases, simplifies conflict resolution to only modify def registers, updates comments |
src/coreclr/jit/lsra.h |
Replaces numbered DEFUSE case enum values with descriptive names |
src/coreclr/jit/lsra.cpp |
Updates debug dump switch/case to use new enum names, removes dead LSRA_EVENT_DEFUSE_FIXED_DELAY_USE case |
|
Another related issue here is #10196 |
|
/azp run runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
jitstressregs failures looks like the same that are present in main. Rest looks like infra failures, will rerun CI. |
The only benefit is that it saves insertion of some explicit copies, but the trade off is an implicit contract between LSRA and codegen that is not very faithful. When LSRA defines in a different register than the constrained register, it believes that codegen can insert a copy and simultaneously use it from the new location, which is not true. This difference in the model has to be handled by inserting additional kill constraints during LSRA build, for example for shifts on xarch.
This PR additionally fixes a few issues uncovered while making this change:
GT_CATCH_ARGcodegen was straight up wrong, not properly producing its register valueFix #125432
Fix #125685