Updating LSRA to prevent eGPR use as GC tracked regs. #117991
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
DeepakRajendrakumaran
force-pushed
the
gcRegFix
branch
from
cac284c to
105550e
Compare
|
@jakobbotsch, please review it this week. |
jakobbotsch
reviewed
Jul 24, 2025
It is fine to limit this set indiscriminately. Having more registers available for this resolution is not going to help much. |
Sounds good. I am thinking something like the following |
This was referenced Jul 24, 2025
DeepakRajendrakumaran
force-pushed
the
gcRegFix
branch
from
3366e0b to
6dd1928
Compare
|
@jakobbotsch I have done the following for now
There is one pending open being discussed(if |
DeepakRajendrakumaran
force-pushed
the
gcRegFix
branch
from
6dd1928 to
6372cb6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the Linear Scan Register Allocator (LSRA) to prevent extended General Purpose Registers (eGPRs) from being used for GC-tracked registers on AMD64, ensuring proper garbage collection tracking and code generation compatibility.
- Introduces filtering logic to restrict eGPR usage for GC types and long types to low GPRs only
- Adds new method
getAvailableGPRsForTypeto encapsulate platform-specific register restrictions - Integrates the filtering into register selection paths for both normal and minimal selection
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/lsra.h | Adds declaration for new getAvailableGPRsForType method |
| src/coreclr/jit/lsra.cpp | Implements register filtering logic and integrates it into register selection process |
This was referenced Jul 25, 2025
DeepakRajendrakumaran
changed the title
jakobbotsch
approved these changes
Jul 25, 2025
|
/azp run runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs, runtime-coreclr libraries-jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
This has been our general testing strategy for APX For this particular change, testing was done with this pending PR too - #117791 SuperPMIThis step will give us the chance to check if there is any assertion failure or internal error within JIT so we can verify the encoding correctness. With the updated coredistools, superpmi also highlights incorrect encoding now The test result from the runs below show no failures related to this PR SuperPMI was run with the following commit to force the JIT to assert if we use eGPR for GC tracked registers This was also run with There are 2 asserts with this configuration which are unrelated to this PR. This issue is being worked on and will be a separate bug fix JIT SDE testThe above is mainly verifying the encoding correctness of the generated binary code. Then the last will examine the semantic correctness of the generated code. WE do this by running SDE with APX on and all changes mentioned earlier. We used the existing CoreCLR unit test set: JIT and run it in the Intel SDE emulator. The test result from the runs below show no failures related to this PR Current CI failsBuild analysis shows 3 fails(on arm and x86). Look unrelated since this change does not affect non |
|
Superpmi asmdiff perfscore improvement with all APX features Diffs are based on 2,612,032 contexts (1,013,569 MinOpts, 1,598,463 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 2 (0.00%) Diff JIT options: JitBypassApxCheck=1;EnableApxConditionalChaining=1;EnableApxNDD=1;EnableApxPPX=1;EnableApxZU=1 Overall (+16,549,934 bytes)
MinOpts (+558,416 bytes)
FullOpts (+15,991,518 bytes)
|
|
Commenter does not have sufficient privileges for PR 117991 in repo dotnet/runtime |
|
@dotnet/intel Can we trigger a run for following? - I rebased the branch to latest |
|
/azp run runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs, runtime-coreclr libraries-jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Thank you! |
Ruihan's emitter bug fixes have merged. I tested this PR again after rebasing locally to latest main Emitter unit testsIn codgenxarch.cpp, similar to genAmd64EmitterUnitTestsSse2, we used the JitLateDisasm feature to insert instructions to encode as unit tests for emitter, and LateDisasm will invoke LLVM to disasm the code stream, this gave us the chance to cross validate the disassembly from JIT and LLVM. The output of this step is to verify the emit paths are generating "correct" code that would not trigger #UD or have wrong semantics. The GC changes should not affect these in theory but ran these as part of testing SuperPMIThis step will give us the chance to check if there is any assertion failure or internal error within JIT so we can verify the encoding correctness. Superpmi also highlights incorrect encoding now SuperPMI was run with the following commit to force the JIT to assert if we use eGPR for GC tracked register. This was also run with JitStressRegs=4000 which forces eGPR usage whenever possible The test result from the runs show no failures |
|
@jakobbotsch, please take a final look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR to discuss these changes and ensure everyone is on the same page. These is not ready for review and is in very early stage(so there are comments etc. in there)
Please ignore the following commits
Relevant commits and what they do
Commit adding GC asserts
This a temporary change for me to test the proposed PR and will be reverted in the actual PR. The goal here is to ensure the JIT never allocates eGPR when the register is GC tracked. These asserts allow me to verify that this doesn't happen when I run superpmi with APX enabled
How I understand Jakob's proposed change to look
This is based on the Jakob's comment where he proposed looking for the places that call
stressLimitRegsand applying similar logic when the interval is a GC intervalThis is currently not useful since we do not pass in a specific
typetoLinearScan::getTempRegForResolution. Instead, it's eitherTYP_INTorTYP_FLOAT. I need to figure out how to handle thisI still have a whole lot of fails(due to the asserts mentioned above which I'm looking into currently
For eg Running
aspnet.run.windows.x64.checkedresults in followingLoaded 199503 Jitted 199503 FailedCompile 1589 Excluded 0 Missing 660@JulieLeeMSFT