-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Handle FIELD_LIST for tailcall arg/parameter interference checks
#122147
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
Conversation
Since tailcalls place the arguments in the incoming parameter area it corrupts the value of existing parameters to the function. To handle this there is logic in place to create defensive copies of the parameters if we detect that we will need those later in the tailcalling sequence. However, this logic was missing handling when the argument was a `FIELD_LIST`: - When an argument was a `FIELD_LIST` we did not take into account that we needed to look for parameter uses starting from its operands. Fix this by introducing a `FirstUncontainedOperand` function and use it here. - When the value of a `PUTARG_STK` was a contained `FIELD_LIST` we did not take into account that its codegen may invalidate the values of its own operands. Detect this case and introduce temps for it.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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 a bug in the JIT compiler's tailcall handling where FIELD_LIST nodes were not properly accounted for during argument/parameter interference checks. When a tailcall places arguments in the incoming parameter area, it can corrupt existing parameters, so the JIT must create defensive copies when needed. The fix introduces a FirstOperand helper function to find the earliest operand of a node (handling contained nodes recursively), and updates the interference detection logic to properly handle FIELD_LIST cases.
Key Changes
- Added
FirstOperandhelper function to recursively find the earliest evaluated operand of a node, including contained operands - Updated
LowerFastTailCallto useFirstOperandinstead ofgtGetOp1()to properly handleFIELD_LISTnodes - Enhanced interference detection to check for self-interfering non-atomic copies when the source is contained
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/lower.h | Declares the new FirstOperand helper function |
| src/coreclr/jit/lower.cpp | Implements FirstOperand and updates tailcall interference logic to use it; adds debugging output and improves comments |
| src/tests/JIT/Regression/Regression_ro_1.csproj | Adds the new regression test to the project file |
| src/tests/JIT/Regression/JitBlue/Runtime_122138/Runtime_122138.cs | Regression test that verifies correct handling of nullable parameters in tailcall scenarios |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS (Edit: oh, already tagged above) |
|
/backport to release/10.0 |
|
Started backporting to |
|
@jakobbotsch backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: JIT: Handle `FIELD_LIST` for tailcall arg/parameter interference checks
Using index info to reconstruct a base tree...
M src/coreclr/jit/lower.cpp
M src/coreclr/jit/lower.h
A src/tests/JIT/Regression/Regression_ro_1.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/lower.cpp
Auto-merging src/coreclr/jit/lower.h
CONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_1.csproj deleted in HEAD and modified in JIT: Handle `FIELD_LIST` for tailcall arg/parameter interference checks. Version JIT: Handle `FIELD_LIST` for tailcall arg/parameter interference checks of src/tests/JIT/Regression/Regression_ro_1.csproj left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 JIT: Handle `FIELD_LIST` for tailcall arg/parameter interference checks
Error: The process '/usr/bin/git' failed with exit code 128 |
Since tailcalls place the arguments in the incoming parameter area it corrupts the value of existing parameters to the function. To handle this there is logic in place to create defensive copies of the parameters if we detect that we will need those later in the tailcalling sequence. However, this logic was missing handling when the argument was a
FIELD_LIST:When an argument was a
FIELD_LISTwe did not take into account that we needed to look for parameter uses starting from its operands, and not just from theFIELD_LISTitself. Fix this by introducing aFirstOperandfunction and use it here.When the value of a
PUTARG_STKwas a containedFIELD_LISTwe did not take into account that its codegen may invalidate the values of its own operands. Detect this case and introduce temps for it.Fix #122138