Skip to content

JIT: Fix tailcall rehoming logic for some interfering operands #116468

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

Merged
merged 3 commits into from
Jun 10, 2025

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Jun 10, 2025

When the JIT notices that an argument being passed to a tailcall will override a later used parameter it tries to create defensive copies of the parameter. However, this logic did not properly handle the case where the first argument the JIT has decided to pass overrides itself partially.

In the test case added we have IR like

N039 (  9,  6) [000025] -c---+-----                   t25 =    LCL_VAR   struct<S0, 48> V00 arg0         u:1 NA (last use) REG NA $80
N041 (???,???) [000043] -----------                            NO_OP     void   REG NA
N043 (???,???) [000042] -----------                            START_NONGC void   REG NA
                                                            ┌──▌  t25    struct
N045 (???,???) [000039] -----------                         ▌  PUTARG_STK [+0x20] void   (48 stackByteSize), (32 byteOffset) (Unroll) REG NA
N047 (  1,  1) [000029] -----+-----                   t29 =    LCL_VAR   int    V03 tmp1         u:1 rdi (last use) REG rdi <l:$380, c:$381>
                                                            ┌──▌  t29    int
N049 (???,???) [000040] -----------                   t40 = ▌  PUTARG_REG int    REG rdi
N051 (  1,  2) [000018] -----+-----                   t18 =    CNS_VEC   simd32<0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000> REG mm0 $3c0
                                                            ┌──▌  t18    simd32
N053 (???,???) [000041] -----------                         ▌  PUTARG_STK [+0x00] void   (32 stackByteSize), (0 byteOffset) REG NA
                                                            ┌──▌  t40    int    arg1 rdi
N055 ( 51, 35) [000026] -ACXG+-----                         ▌  CALL      void   Program:M12(System.Numerics.Vector`1[ulong],sbyte,S0):short REG NA $VN.Void

where [000039] will partially override its operand [000025]. The previous logic did not detect that a temp was needed for [000025].

Fix #116466

When the JIT notices that an argument being passed to a tailcall will
override a parameter it tries to create defensive copies of the
parameter. However, this logic did not properly handle the case where
the first argument the JIT has decided to pass overrides itself
partially.

In the test case added we have IR like
```
N039 (  9,  6) [000025] -c---+-----                   t25 =    LCL_VAR   struct<S0, 48> V00 arg0         u:1 NA (last use) REG NA $80
N041 (???,???) [000043] -----------                            NO_OP     void   REG NA
N043 (???,???) [000042] -----------                            START_NONGC void   REG NA
                                                            ┌──▌  t25    struct
N045 (???,???) [000039] -----------                         ▌  PUTARG_STK [+0x20] void   (48 stackByteSize), (32 byteOffset) (Unroll) REG NA
N047 (  1,  1) [000029] -----+-----                   t29 =    LCL_VAR   int    V03 tmp1         u:1 rdi (last use) REG rdi <l:$380, c:$381>
                                                            ┌──▌  t29    int
N049 (???,???) [000040] -----------                   t40 = ▌  PUTARG_REG int    REG rdi
N051 (  1,  2) [000018] -----+-----                   t18 =    CNS_VEC   simd32<0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000> REG mm0 $3c0
                                                            ┌──▌  t18    simd32
N053 (???,???) [000041] -----------                         ▌  PUTARG_STK [+0x00] void   (32 stackByteSize), (0 byteOffset) REG NA
                                                            ┌──▌  t40    int    arg1 rdi
N055 ( 51, 35) [000026] -ACXG+-----                         ▌  CALL      void   Program:M12(System.Numerics.Vector`1[ulong],sbyte,S0):short REG NA $VN.Void
```

where `[000039]` will partially override its operand `[000025]`. The
previous logic did not detect that a temp was needed for `[000025]`.

Fix dotnet#116466
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 10, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@jakobbotsch jakobbotsch marked this pull request as ready for review June 10, 2025 13:13
@Copilot Copilot AI review requested due to automatic review settings June 10, 2025 13:13
Copy link
Contributor

@Copilot Copilot AI left a 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 tailcall rehoming logic by ensuring that a temporary is created when an argument partially overrides itself. It also adds a new regression test to capture the scenario and updates the lowering logic to use the operand version of the first PUTARG_STK.

  • Fix tailcall rehoming by tracking the operand node
  • Add regression test for the identified scenario
  • Adjust calls in the lowering phase to use the correct operand

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/tests/JIT/Regression/JitBlue/Runtime_116466/Runtime_116466.csproj New project configuration for the regression test
src/tests/JIT/Regression/JitBlue/Runtime_116466/Runtime_116466.cs New regression test case demonstrating the tailcall issue
src/coreclr/jit/lower.cpp Updates to use the operand node (firstPutargStkOp) when invoking RehomeArgForFastTailCall
Comments suppressed due to low confidence (4)

src/coreclr/jit/lower.cpp:3342

  • [nitpick] Consider adding a brief comment explaining that 'firstPutargStkOp' is used to track the operand node for tailcall rehoming, clarifying its role in the defensive copy logic.
GenTree* firstPutargStkOp = firstPutargStk->gtGetOp1();

src/coreclr/jit/lower.cpp:3394

  • [nitpick] Add a comment to clarify why 'firstPutargStkOp' is used to set 'lookForUsesFrom', which aids in understanding the operand rehoming for fast tailcalls.
lookForUsesFrom = firstPutargStkOp;

src/coreclr/jit/lower.cpp:3397

  • [nitpick] It would be helpful to include a note describing why passing the operand node ('firstPutargStkOp') is necessary here compared to the original node.
RehomeArgForFastTailCall(callerArgLclNum, firstPutargStkOp, lookForUsesFrom, call);

src/coreclr/jit/lower.cpp:3411

  • [nitpick] Consider adding a comment explaining the use of 'firstPutargStkOp' for struct field rehoming, which helps clarify the adjustments in the tailcall logic.
RehomeArgForFastTailCall(j, firstPutargStkOp, lookForUsesFrom, call);

@jakobbotsch
Copy link
Member Author

PTAL @dotnet/jit-contrib

Minor diffs.

@jakobbotsch jakobbotsch requested a review from a team June 10, 2025 13:29
@jakobbotsch jakobbotsch merged commit 04514db into dotnet:main Jun 10, 2025
116 of 118 checks passed
@jakobbotsch jakobbotsch deleted the fix-116466 branch June 10, 2025 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: Wrong result forwarding argument for tailcall on linux-x64
2 participants