From 1b42e2c500b1757b5428e79d36f16a33202e869f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Sep 2025 12:38:54 -0700 Subject: [PATCH 1/2] JIT: handle x86 tail call via helper when control expression is spilled Make sure we order the control expression after the spill. Also add a range control to selectively enable instrumentation stress. Fixes #120170. --- src/coreclr/jit/compiler.cpp | 12 ++++++++++-- src/coreclr/jit/jitconfigvalues.h | 1 + src/coreclr/jit/lower.cpp | 7 ++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 6f62c555ce60ee..137677357c5d28 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -7025,8 +7025,16 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr, #ifdef DEBUG if (JitConfig.JitInstrumentIfOptimizing() && opts.OptimizationEnabled() && !IsReadyToRun()) { - JITDUMP("\nForcibly enabling instrumentation\n"); - opts.jitFlags->Set(JitFlags::JIT_FLAG_BBINSTR); + // Optionally disable by range + // + static ConfigMethodRange JitInstrumentIfOptimizingRange; + JitInstrumentIfOptimizingRange.EnsureInit(JitConfig.JitInstrumentIfOptimizingRange()); + const unsigned hash = impInlineRoot()->info.compMethodHash(); + if (JitInstrumentIfOptimizingRange.Contains(hash)) + { + JITDUMP("\nEnabling instrumentation\n"); + opts.jitFlags->Set(JitFlags::JIT_FLAG_BBINSTR); + } } #endif diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 154054d8453041..3a9188a3e1eb0f 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -756,6 +756,7 @@ RELEASE_CONFIG_INTEGER(JitCollect64BitCounts, "JitCollect64BitCounts", 0) // Col CONFIG_INTEGER(JitInstrumentIfOptimizing, "JitInstrumentIfOptimizing", 0) // 1: Always add instrumentation if optimizing // and not prejitting +CONFIG_STRING(JitInstrumentIfOptimizingRange, "JitInstrumentIfOptimizingRange") RELEASE_CONFIG_INTEGER(JitInstrumentInlinees, "JitInstrumentInlinees", 1) // Add instrumentation to inlined methods diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 73597ea88edffa..a337bd8f849512 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3504,8 +3504,10 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar assert(argEntry != nullptr); GenTree* arg0 = argEntry->GetEarlyNode()->AsPutArgStk()->gtGetOp1(); + // Temporarily link in the call target range just before the call, since it + // may refer to a spilled temp. We will move the putarg after this via MoveCFGCallArgs below. ContainCheckRange(callTargetRange); - BlockRange().InsertAfter(arg0, std::move(callTargetRange)); + BlockRange().InsertBefore(call, std::move(callTargetRange)); bool isClosed; LIR::ReadOnlyRange secondArgRange = BlockRange().GetTreeRange(arg0, &isClosed); @@ -3540,6 +3542,9 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar assert(arg3->OperIs(GT_CNS_INT)); #endif // DEBUG + // Now reorder so all the putargs are just before the call. + MoveCFGCallArgs(call); + // Transform this call node into a call to Jit tail call helper. call->gtCallType = CT_HELPER; call->gtCallMethHnd = comp->eeFindHelper(CORINFO_HELP_TAILCALL); From 63e5d08946d37a8703492e09fa9ff3d44e5915e7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 20 Oct 2025 12:34:51 -0700 Subject: [PATCH 2/2] review feedback --- src/coreclr/jit/lower.cpp | 66 +++++++++++++++++---------------------- src/coreclr/jit/lower.h | 4 +-- 2 files changed, 30 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a337bd8f849512..6551f49af83163 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2127,7 +2127,7 @@ void Lowering::InsertSpecialCopyArg(GenTreePutArgStk* putArgStk, CORINFO_CLASS_H // Finally move all GT_PUTARG_* nodes // Re-use the existing logic for CFG call args here - MoveCFGCallArgs(call); + MovePutArgNodesUpToCall(call); BlockRange().Remove(destPlaceholder); BlockRange().Remove(srcPlaceholder); @@ -2899,13 +2899,24 @@ GenTree* Lowering::LowerCall(GenTree* node) if (call->IsTailCallViaJitHelper()) { // Either controlExpr or gtCallAddr must contain real call target. - if (controlExpr == nullptr) + if (controlExpr != nullptr) { + // Link controlExpr into the IR before the call. + // The callTarget tree needs to be sequenced. + LIR::Range callTargetRange = LIR::SeqTree(comp, controlExpr); + ContainCheckRange(callTargetRange); + BlockRange().InsertBefore(call, std::move(callTargetRange)); + } + else + { + // gtCallAddr is already sequenced and just before the call. assert(call->gtCallType == CT_INDIRECT); assert(call->gtCallAddr != nullptr); controlExpr = call->gtCallAddr; } + // LowerTailCallViaJitHelper will turn the control expr + // into an arg and produce a new control expr for the helper call. controlExpr = LowerTailCallViaJitHelper(call, controlExpr); } @@ -3472,21 +3483,6 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar InsertPInvokeMethodEpilog(comp->compCurBB DEBUGARG(call)); } - // Remove gtCallAddr from execution order if present. - if (call->gtCallType == CT_INDIRECT) - { - assert(call->gtCallAddr != nullptr); - - bool isClosed; - LIR::ReadOnlyRange callAddrRange = BlockRange().GetTreeRange(call->gtCallAddr, &isClosed); - assert(isClosed); - - BlockRange().Remove(std::move(callAddrRange)); - } - - // The callTarget tree needs to be sequenced. - LIR::Range callTargetRange = LIR::SeqTree(comp, callTarget); - // Verify the special args are what we expect, and replace the dummy args with real values. // We need to figure out the size of the outgoing stack arguments, not including the special args. // The number of 4-byte words is passed to the helper for the incoming and outgoing argument sizes. @@ -3504,11 +3500,6 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar assert(argEntry != nullptr); GenTree* arg0 = argEntry->GetEarlyNode()->AsPutArgStk()->gtGetOp1(); - // Temporarily link in the call target range just before the call, since it - // may refer to a spilled temp. We will move the putarg after this via MoveCFGCallArgs below. - ContainCheckRange(callTargetRange); - BlockRange().InsertBefore(call, std::move(callTargetRange)); - bool isClosed; LIR::ReadOnlyRange secondArgRange = BlockRange().GetTreeRange(arg0, &isClosed); assert(isClosed); @@ -3543,7 +3534,7 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar #endif // DEBUG // Now reorder so all the putargs are just before the call. - MoveCFGCallArgs(call); + MovePutArgNodesUpToCall(call); // Transform this call node into a call to Jit tail call helper. call->gtCallType = CT_HELPER; @@ -3713,7 +3704,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) LowerNode(regNode); // Finally move all GT_PUTARG_* nodes - MoveCFGCallArgs(call); + MovePutArgNodesUpToCall(call); break; } case CFGCallKind::Dispatch: @@ -3809,12 +3800,12 @@ bool Lowering::IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive } //------------------------------------------------------------------------ -// MoveCFGCallArg: Given a call that will be CFG transformed using the -// validate+call scheme, and an argument GT_PUTARG_* or GT_FIELD_LIST node, +// MovePutArgUpToCall: Given a call that will be transformed using the +// CFG validate+call or similar scheme, and an argument GT_PUTARG_* or GT_FIELD_LIST node, // move that node right before the call. // // Arguments: -// call - The call that is being CFG transformed +// call - The call that is being transformed // node - The argument node // // Remarks: @@ -3823,7 +3814,7 @@ bool Lowering::IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive // are not always safe to move further ahead; for invariant operands, we // move them ahead as well to shorten the lifetime of these values. // -void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) +void Lowering::MovePutArgUpToCall(GenTreeCall* call, GenTree* node) { assert(node->OperIsPutArg() || node->OperIsFieldList()); @@ -3833,7 +3824,7 @@ void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) for (GenTreeFieldList::Use& operand : node->AsFieldList()->Uses()) { assert(operand.GetNode()->OperIsPutArg()); - MoveCFGCallArg(call, operand.GetNode()); + MovePutArgUpToCall(call, operand.GetNode()); } } else @@ -3861,30 +3852,29 @@ void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) } //------------------------------------------------------------------------ -// MoveCFGCallArgs: Given a call that will be CFG transformed using the -// validate+call scheme, move all GT_PUTARG_* or GT_FIELD_LIST nodes right before the call. +// MovePutArgNodesUpToCall: Move all GT_PUTARG_* or GT_FIELD_LIST nodes right before the call. // // Arguments: -// call - The call that is being CFG transformed +// call - The call that is being transformed // // Remarks: -// See comments in MoveCFGCallArg for more details. +// See comments in MovePutArgUpToCall for more details. // -void Lowering::MoveCFGCallArgs(GenTreeCall* call) +void Lowering::MovePutArgNodesUpToCall(GenTreeCall* call) { // Finally move all GT_PUTARG_* nodes for (CallArg& arg : call->gtArgs.EarlyArgs()) { GenTree* node = arg.GetEarlyNode(); assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); + MovePutArgUpToCall(call, node); } for (CallArg& arg : call->gtArgs.LateArgs()) { GenTree* node = arg.GetLateNode(); assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); + MovePutArgUpToCall(call, node); } } @@ -9781,7 +9771,7 @@ bool Lowering::TryLowerBlockStoreAsGcBulkCopyCall(GenTreeBlk* blk) // Finally move all GT_PUTARG_* nodes // Re-use the existing logic for CFG call args here - MoveCFGCallArgs(call); + MovePutArgNodesUpToCall(call); BlockRange().Remove(destPlaceholder); BlockRange().Remove(sizePlaceholder); @@ -9917,7 +9907,7 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) // Finally move all GT_PUTARG_* nodes // Re-use the existing logic for CFG call args here - MoveCFGCallArgs(call); + MovePutArgNodesUpToCall(call); BlockRange().Remove(destPlaceholder); BlockRange().Remove(sizePlaceholder); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index fb8d87b982d177..e2778bbd71cc4f 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -151,8 +151,8 @@ class Lowering final : public Phase bool LowerCallMemcmp(GenTreeCall* call, GenTree** next); bool LowerCallMemset(GenTreeCall* call, GenTree** next); void LowerCFGCall(GenTreeCall* call); - void MoveCFGCallArgs(GenTreeCall* call); - void MoveCFGCallArg(GenTreeCall* call, GenTree* node); + void MovePutArgNodesUpToCall(GenTreeCall* call); + void MovePutArgUpToCall(GenTreeCall* call, GenTree* node); #ifndef TARGET_64BIT GenTree* DecomposeLongCompare(GenTree* cmp); #endif