-
Couldn't load subscription status.
- Fork 5.2k
JIT: Fix unnecessary GC transitions recorded for tailcalls #120934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jakobbotsch <[email protected]>
src/coreclr/jit/emitloongarch64.cpp
Outdated
| // For tail calls, we don't trim registers since we're jumping to another function, not returning. | ||
| regMaskTP gcrefRegs; | ||
| regMaskTP byrefRegs; | ||
| if (params.isJump) | ||
| { | ||
| gcrefRegs = params.gcrefRegs; | ||
| byrefRegs = params.byrefRegs; | ||
| } | ||
| else | ||
| { | ||
| regMaskTP savedSet = emitGetGCRegsSavedOrModified(params.methHnd); | ||
| gcrefRegs = params.gcrefRegs & savedSet; | ||
| byrefRegs = params.byrefRegs & savedSet; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This fix looks good, but can we make it a bit terser by using a ternary for savedSet and making it equal RBM_ALLINT in the params.isJump case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 9c6c876. Used ternary operator with RBM_ALLINT for tail calls across all 5 architecture emitters.
Co-authored-by: jakobbotsch <[email protected]>
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes unnecessary GC transitions being recorded for tail calls in the JIT compiler. For tail calls (jumps), the function doesn't return but jumps to another function, so registers should remain live without triggering GC transitions.
Key Changes:
- Modified the
emitIns_Callfunction across all 5 architecture-specific emitters to skip register trimming for tail calls - Changed
savedSetcalculation to useRBM_ALLINT(all integer registers) whenparams.isJumpis true, preventing unnecessary GC info transitions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/emitxarch.cpp | Updated savedSet calculation for x86/x64 to skip register trimming on tail calls |
| src/coreclr/jit/emitriscv64.cpp | Updated savedSet calculation for RISC-V 64-bit to skip register trimming on tail calls |
| src/coreclr/jit/emitloongarch64.cpp | Updated savedSet calculation for LoongArch 64-bit to skip register trimming on tail calls |
| src/coreclr/jit/emitarm64.cpp | Updated savedSet calculation for ARM64 to skip register trimming on tail calls |
| src/coreclr/jit/emitarm.cpp | Updated savedSet calculation for ARM to skip register trimming on tail calls |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Fix JIT: Unnecessary GC transitions recorded for tailcalls
Plan:
Issue Summary:
The
emitIns_Callfunction in all JIT emitters incorrectly trims callee-trashed registers from the live set for tail calls (jumps). For tail calls, the function doesn't return but jumps to another function, so the registers should remain live and not trigger unnecessary GC transitions.Changes Made:
Modified all 5 architecture-specific emitter files to use a ternary operator for
savedSet, setting it toRBM_ALLINT(all integer registers) for tail calls instead of callingemitGetGCRegsSavedOrModified:The refactored implementation is more concise while maintaining the same logic:
Original prompt
Fixes #120933
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.