From 055b050648634fd80aa58cbcec1840791c72537c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 3 Dec 2025 16:53:28 +0100 Subject: [PATCH 1/3] JIT: Handle `FIELD_LIST` for tailcall arg/parameter interference checks 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. --- src/coreclr/jit/lower.cpp | 50 +++++++++++++++++-- src/coreclr/jit/lower.h | 1 + .../JitBlue/Runtime_122138/Runtime_122138.cs | 28 +++++++++++ .../JIT/Regression/Regression_ro_1.csproj | 1 + 4 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_122138/Runtime_122138.cs diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index e66abbbf9b9ca2..d25399ceebdb6e 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3239,11 +3239,11 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) if (!putargs.Empty()) { GenTree* firstPutargStk = putargs.Bottom(0); - GenTree* firstPutargStkOp = firstPutargStk->gtGetOp1(); + GenTree* firstPutargStkOp = FirstUncontainedOperand(firstPutargStk); for (int i = 1; i < putargs.Height(); i++) { firstPutargStk = LIR::FirstNode(firstPutargStk, putargs.Bottom(i)); - firstPutargStkOp = LIR::FirstNode(firstPutargStkOp, putargs.Bottom(i)->gtGetOp1()); + firstPutargStkOp = LIR::FirstNode(firstPutargStkOp, FirstUncontainedOperand(putargs.Bottom(i))); } // Since this is a fast tailcall each PUTARG_STK will place the argument in the // _incoming_ arg space area. This will effectively overwrite our already existing @@ -3279,6 +3279,10 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) continue; } + JITDUMP( + "PUTARG_STK [%06u] overwrites [%06u..%06u); parameter V%03u lives in [%06u..%06u); may need defensive copies\n", + Compiler::dspTreeID(put), overwrittenStart, overwrittenEnd, callerArgLclNum, argStart, argEnd); + // Codegen cannot handle a partially overlapping copy. For // example, if we have // bar(S16 stack, S32 stack2) @@ -3287,10 +3291,13 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) // ahead. It is possible that this PUTARG_STK is the only use, // in which case we will need to introduce a temp, so look for // uses starting from it. Note that we assume that in-place - // copies are OK. + // copies are ok provided the source is a scalar value (below + // checked via isContained(), but which primarily targets + // GT_FIELD_LIST). GenTree* lookForUsesFrom = put->gtNext; - if (overwrittenStart != argStart) + if ((overwrittenStart != argStart) || put->gtGetOp1()->isContained()) { + JITDUMP("Non-atomic copy may be self-interfering. Expanding search...\n"); lookForUsesFrom = firstPutargStkOp; } @@ -3354,7 +3361,42 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) unreached(); #endif } + +//------------------------------------------------------------------------ +// FirstUncontainedOperand: +// Find the earliest evaluated operand of a node. +// +// Arguments: +// node - The node // +GenTree* Lowering::FirstUncontainedOperand(GenTree* node) +{ + struct Helper + { + GenTree* Result = nullptr; + + void Visit(GenTree* node) + { + node->VisitOperands([=](GenTree* op) { + if (op->isContained()) + { + Visit(op); + } + else + { + Result = Result == nullptr ? op : LIR::FirstNode(Result, op); + } + + return GenTree::VisitResult::Continue; + }); + } + }; + + Helper helper; + helper.Visit(node); + return helper.Result; +} + //------------------------------------------------------------------------ // RehomeArgForFastTailCall: Introduce temps for args that may be overwritten // during fast tailcall sequence. diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 8f5d18e16f9eff..a40dd04656774b 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -202,6 +202,7 @@ class Lowering final : public Phase GenTree* LowerNonvirtPinvokeCall(GenTreeCall* call); GenTree* LowerTailCallViaJitHelper(GenTreeCall* callNode, GenTree* callTarget); void LowerFastTailCall(GenTreeCall* callNode); + GenTree* FirstUncontainedOperand(GenTree* node); void RehomeArgForFastTailCall(unsigned int lclNum, GenTree* insertTempBefore, GenTree* lookForUsesStart, diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_122138/Runtime_122138.cs b/src/tests/JIT/Regression/JitBlue/Runtime_122138/Runtime_122138.cs new file mode 100644 index 00000000000000..ae896d12e8e64b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_122138/Runtime_122138.cs @@ -0,0 +1,28 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_122138 +{ + [MethodImpl(MethodImplOptions.NoOptimization)] + [Fact] + public static void TestEntryPoint() + { + var test = new Runtime_122138(); + test.Method1(0, 0, 0, 999, 999); + } + + private void Method1(int a, int b, int c, int value1, int? value2) + { + Method2(1, 2, 3, value1, value2); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private void Method2(long a, int b, int c, int? value1, int? value2) + { + Assert.Equal(value1, value2); + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/Regression_ro_1.csproj b/src/tests/JIT/Regression/Regression_ro_1.csproj index 7181197dd10a42..123bfaa9c14033 100644 --- a/src/tests/JIT/Regression/Regression_ro_1.csproj +++ b/src/tests/JIT/Regression/Regression_ro_1.csproj @@ -75,6 +75,7 @@ + From 8cf03693cba068b2c942ebd05bf83c92500381e4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 3 Dec 2025 18:01:24 +0100 Subject: [PATCH 2/3] PUTARG_STK may not have any uncontained operands --- src/coreclr/jit/lower.cpp | 19 ++++++++++--------- src/coreclr/jit/lower.h | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d25399ceebdb6e..21222579aaf689 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3239,11 +3239,11 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) if (!putargs.Empty()) { GenTree* firstPutargStk = putargs.Bottom(0); - GenTree* firstPutargStkOp = FirstUncontainedOperand(firstPutargStk); + GenTree* firstPutargStkOp = FirstOperand(firstPutargStk); for (int i = 1; i < putargs.Height(); i++) { firstPutargStk = LIR::FirstNode(firstPutargStk, putargs.Bottom(i)); - firstPutargStkOp = LIR::FirstNode(firstPutargStkOp, FirstUncontainedOperand(putargs.Bottom(i))); + firstPutargStkOp = LIR::FirstNode(firstPutargStkOp, FirstOperand(putargs.Bottom(i))); } // Since this is a fast tailcall each PUTARG_STK will place the argument in the // _incoming_ arg space area. This will effectively overwrite our already existing @@ -3363,13 +3363,16 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) } //------------------------------------------------------------------------ -// FirstUncontainedOperand: -// Find the earliest evaluated operand of a node. +// FirstEvaluatedOperand: +// Find the earliest operand of a node. // // Arguments: // node - The node // -GenTree* Lowering::FirstUncontainedOperand(GenTree* node) +// Returns: +// The earliest evaluated operand. +// +GenTree* Lowering::FirstOperand(GenTree* node) { struct Helper { @@ -3378,14 +3381,12 @@ GenTree* Lowering::FirstUncontainedOperand(GenTree* node) void Visit(GenTree* node) { node->VisitOperands([=](GenTree* op) { + Result = Result == nullptr ? op : LIR::FirstNode(Result, op); + if (op->isContained()) { Visit(op); } - else - { - Result = Result == nullptr ? op : LIR::FirstNode(Result, op); - } return GenTree::VisitResult::Continue; }); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index a40dd04656774b..a09f858a2a5fd0 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -202,7 +202,7 @@ class Lowering final : public Phase GenTree* LowerNonvirtPinvokeCall(GenTreeCall* call); GenTree* LowerTailCallViaJitHelper(GenTreeCall* callNode, GenTree* callTarget); void LowerFastTailCall(GenTreeCall* callNode); - GenTree* FirstUncontainedOperand(GenTree* node); + GenTree* FirstOperand(GenTree* node); void RehomeArgForFastTailCall(unsigned int lclNum, GenTree* insertTempBefore, GenTree* lookForUsesStart, From 5a5d563830ea498b7d3df39aa9f97ddc8bfa335c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 4 Dec 2025 13:18:47 +0100 Subject: [PATCH 3/3] Fix regressions --- src/coreclr/jit/lower.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 21222579aaf689..73221a60143bf6 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3291,11 +3291,9 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) // ahead. It is possible that this PUTARG_STK is the only use, // in which case we will need to introduce a temp, so look for // uses starting from it. Note that we assume that in-place - // copies are ok provided the source is a scalar value (below - // checked via isContained(), but which primarily targets - // GT_FIELD_LIST). + // copies are ok provided the source is a scalar value. GenTree* lookForUsesFrom = put->gtNext; - if ((overwrittenStart != argStart) || put->gtGetOp1()->isContained()) + if ((overwrittenStart != argStart) || put->gtGetOp1()->OperIsFieldList()) { JITDUMP("Non-atomic copy may be self-interfering. Expanding search...\n"); lookForUsesFrom = firstPutargStkOp; @@ -3363,7 +3361,7 @@ void Lowering::LowerFastTailCall(GenTreeCall* call) } //------------------------------------------------------------------------ -// FirstEvaluatedOperand: +// FirstOperand: // Find the earliest operand of a node. // // Arguments: