From 14fcda7e6f1e400c69bad3db751229a52ab4fa1d Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 16 Aug 2026 18:31:11 +0200 Subject: [PATCH 1/3] Don't require GDV targets to be inlineable Guarded devirtualization used to give up whenever the target we'd devirtualize to couldn't be inlined. But a direct call is still cheaper than a virtual or interface call, so keep the candidate and just don't inline it. Added JitGuardedDevirtualizationRequireInlining to get the old behavior back. Class-based GDV only for now, method/delegate GDV is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38498b8d-c116-4637-b069-d1d275634193 --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/importercalls.cpp | 158 ++++++++++++++------ src/coreclr/jit/indirectcalltransformer.cpp | 91 ++++++----- src/coreclr/jit/inline.h | 8 + src/coreclr/jit/jitconfigvalues.h | 6 + src/coreclr/jit/jitmetadatalist.h | 1 + 6 files changed, 182 insertions(+), 84 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 03fa22ec8c5463..1f296f95fe9af2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -8287,6 +8287,8 @@ class Compiler CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_RESOLVED_TOKEN* pUnboxedResolvedToken); + bool canKeepNonInlineableGdvCandidate(GenTreeCall* call); + int getGDVMaxTypeChecks() { int typeChecks = JitConfig.JitGuardedDevirtualizationMaxTypeChecks(); diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 19b001e5026bb2..0dd97c636d7e0a 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -2157,14 +2157,15 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN assert(retRegCount >= 2); - if (!call->CanTailCall() && !call->IsInlineCandidate()) + if (!call->CanTailCall() && !call->IsInlineCandidate() && !call->IsGuardedDevirtualizationCandidate()) { // Force a call returning multi-reg struct to be always of the IR form // tmp = call // // No need to assign a multi-reg struct to a local var if: // - It is a tail call or - // - The call is marked for in-lining later + // - The call is marked for in-lining later or + // - The call is a guarded devirtualization candidate (we defer this until we know its fate) return impStoreMultiRegValueToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); } return call; @@ -9034,16 +9035,25 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, // Gather some information for later. Note we actually allocate InlineCandidateInfo // here, as the devirtualized half of this call will likely become an inline candidate. // - InlineCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo; - - pInfo->guardedMethodHandle = methodHandle; - pInfo->guardedMethodInstParamLookup = {}; - pInfo->guardedMethodResolvedToken = {}; - pInfo->guardedMethodUnboxedResolvedToken = {}; - pInfo->guardedClassHandle = classHandle; - pInfo->likelihood = likelihood; - pInfo->exactContextHandle = contextHandle; - pInfo->originalMethodHandle = originalMethodHandle; + // Value-initialize: a GDV candidate can now survive all the way through the expansion + // in fgTransformIndirectCalls without ever being seen by impCheckCanInline, so every + // field has to be in a well-defined state from the start. + // + InlineCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo{}; + + pInfo->guardedMethodHandle = methodHandle; + pInfo->guardedClassHandle = classHandle; + pInfo->likelihood = likelihood; + pInfo->exactContextHandle = contextHandle; + pInfo->originalMethodHandle = originalMethodHandle; + pInfo->clsAttr = classAttr; + pInfo->methAttr = methodAttr; + pInfo->preexistingSpillTemp = BAD_VAR_NUM; + + // Note: the call node only carries its IL offset in debug builds, and not this early, + // so we can't record a meaningful offset here. It is only used for reporting. + // + pInfo->ilOffset = BAD_IL_OFFSET; if (instParamLookup != nullptr) { @@ -9106,6 +9116,67 @@ void Compiler::impConvertToUserCallAndMarkForInlining(GenTreeCall* call) } } +//------------------------------------------------------------------------ +// canKeepNonInlineableGdvCandidate: check if we can keep a guarded devirtualization +// candidate whose target we're not going to inline. +// +// Arguments: +// call -- guarded devirtualization candidate +// +// Return Value: +// true if the candidate can be kept just for the sake of devirtualization. +// +// Notes: +// A direct call is normally cheaper than a virtual/interface call and it unlocks +// further optimizations (exact type of 'this', better inlining of the callee's +// callees, etc.), so by default we keep such candidates around. There are a few +// call sites, however, where the expansion itself is either illegal or would +// interfere with a more valuable optimization. +// +// The bail-outs below mirror the call-site legality checks in +// impMarkInlineCandidateHelper; keep the two in sync. +// +bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) +{ + assert(call->IsGuardedDevirtualizationCandidate()); + + if (JitConfig.JitGuardedDevirtualizationRequireInlining() != 0) + { + return false; + } + + // For now this is limited to class-based GDV. Method-based (e.g. delegate) GDV keeps + // the old behavior of requiring the target to be inlineable. + // + if (call->GetGDVCandidateInfo(0)->guardedClassHandle == NO_CLASS_HANDLE) + { + return false; + } + + // The expansion moves the call out of the tail position, so leave tail calls alone. + // + if (call->IsTailPrefixedCall()) + { + return false; + } + + // Tail recursion elimination (turning the call into a loop) is more valuable. + // + if (call->IsImplicitTailCall() && gtIsRecursiveCall(call)) + { + return false; + } + + // The NextCallReturnAddress intrinsic needs the call to stay exactly where it is. + // + if (info.compHasNextCallRetAddr) + { + return false; + } + + return true; +} + //------------------------------------------------------------------------ // impMarkInlineCandidate: determine if this call can be subsequently inlined // @@ -9117,8 +9188,8 @@ void Compiler::impConvertToUserCallAndMarkForInlining(GenTreeCall* call) // // Notes: // Mostly a wrapper for impMarkInlineCandidateHelper that also undoes -// guarded devirtualization for virtual calls where the method we'd -// devirtualize to cannot be inlined. +// guarded devirtualization for virtual calls where the guarded devirtualization +// itself is not worth doing (or not legal) once we know we can't inline the target. void Compiler::impMarkInlineCandidate(GenTree* callNode, CORINFO_CONTEXT_HANDLE exactContextHnd, @@ -9140,19 +9211,35 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsGuardedDevirtualizationCandidate()) { assert(call->GetInlineCandidatesCount() > 0); + + // If the target can't be inlined we normally still want to devirtualize it, + // see canKeepNonInlineableGdvCandidate for the exceptions. + // + const bool keepNonInlineable = canKeepNonInlineableGdvCandidate(call); + for (uint8_t candidateId = 0; candidateId < call->GetInlineCandidatesCount(); candidateId++) { InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate for GDV"); // Do the actual evaluation impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, callInfo, inlinersContext, &inlineResult); - // Ignore non-inlineable candidates - // TODO: Consider keeping them to just devirtualize without inlining, at least for interface - // calls on NativeAOT, but that requires more changes elsewhere too. + if (!inlineResult.IsCandidate()) { - call->RemoveGDVCandidateInfo(this, candidateId); - candidateId--; + if (!keepNonInlineable) + { + call->RemoveGDVCandidateInfo(this, candidateId); + candidateId--; + continue; + } + + JITDUMP("Keeping GDV candidate %u of call [%06u] for devirtualization only: target can't be inlined\n", + candidateId, dspTreeID(call)); + + // We only keep class-based candidates, and only ones we won't inline. + // + assert(!call->GetGDVCandidateInfo(candidateId)->isInlineable); + assert(call->GetGDVCandidateInfo(candidateId)->guardedClassHandle != NO_CLASS_HANDLE); } } @@ -9170,25 +9257,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); impMarkInlineCandidateHelper(call, 0, exactContextHnd, callInfo, inlinersContext, &inlineResult); } - - // If this call is an inline candidate or is not a guarded devirtualization - // candidate, we're done. - if (call->IsInlineCandidate() || !call->IsGuardedDevirtualizationCandidate()) - { - return; - } - - // If we can't inline the call we'd guardedly devirtualize to, - // we undo the guarded devirtualization, as the benefit from - // just guarded devirtualization alone is likely not worth the - // extra jit time and code size. - // - // TODO: it is possibly interesting to allow this, but requires - // fixes elsewhere too... - JITDUMP("Revoking guarded devirtualization candidacy for call [%06u]: target method can't be inlined\n", - dspTreeID(call)); - - call->ClearInlineInfo(); } //------------------------------------------------------------------------ @@ -9477,6 +9545,8 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, call->SetSingleInlineCandidateInfo(inlineCandidateInfo); } + inlineCandidateInfo->isInlineable = true; + // Let the strategy know there's another candidate. impInlineRoot()->m_inlineStrategy->NoteCandidate(); @@ -11045,17 +11115,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, } else { - pInfo = new (pParam->pThis, CMK_Inlining) InlineCandidateInfo; - - // Null out bits we don't use when we're just inlining - // - pInfo->guardedClassHandle = nullptr; - pInfo->guardedMethodHandle = nullptr; - pInfo->guardedMethodInstParamLookup = {}; - pInfo->guardedMethodResolvedToken = {}; - pInfo->guardedMethodUnboxedResolvedToken = {}; - pInfo->originalMethodHandle = nullptr; - pInfo->likelihood = 0; + pInfo = new (pParam->pThis, CMK_Inlining) InlineCandidateInfo{}; } pInfo->methInfo = methInfo; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index b7974c64cdfc22..1fb1d17e6baee7 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -572,16 +572,6 @@ class IndirectCallTransformer JITDUMP("\n----------------\n\n*** %s contemplating [%06u] in " FMT_BB " \n", Name(), m_compiler->dspTreeID(m_origCall), m_currBlock->bbNum); - // We currently need inline candidate info to guarded devirt. - // - if (!m_origCall->IsInlineCandidate()) - { - JITDUMP("*** %s Bailing on [%06u] -- not an inline candidate\n", Name(), - m_compiler->dspTreeID(m_origCall)); - ClearFlag(); - return; - } - m_likelihood = m_origCall->GetGDVCandidateInfo(0)->likelihood; assert((m_likelihood >= 0) && (m_likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", m_likelihood); @@ -848,10 +838,24 @@ class IndirectCallTransformer } else { - // If there's a spill temp already associated with this inline candidate, + // If there's a spill temp already associated with any of the candidates, // use that instead of allocating a new temp. // - m_returnTemp = inlineInfo->preexistingSpillTemp; + // Only candidates that made it through impMarkInlineCandidateHelper get a spill + // temp assigned, so we can't just look at candidate 0: it may be a candidate we + // kept for devirtualization only while a later one carries the inliner's temp. + // + m_returnTemp = BAD_VAR_NUM; + for (uint8_t i = 0; i < m_origCall->GetInlineCandidatesCount(); i++) + { + const unsigned spillTemp = m_origCall->GetGDVCandidateInfo(i)->preexistingSpillTemp; + if (spillTemp != BAD_VAR_NUM) + { + // All candidates share the same call site, so they must agree. + assert((m_returnTemp == BAD_VAR_NUM) || (m_returnTemp == spillTemp)); + m_returnTemp = spillTemp; + } + } if (m_returnTemp != BAD_VAR_NUM) { @@ -958,22 +962,6 @@ class IndirectCallTransformer GenTreeCall* call = m_compiler->gtCloneCandidateCall(m_origCall); call->gtArgs.GetThisArg()->SetEarlyNode(m_compiler->gtNewLclvNode(thisTemp, TYP_REF)); - // If the original call was flagged as one that might inspire enumerator de-abstraction - // cloning, move the flag to the devirtualized call. - // - if (m_compiler->hasImpEnumeratorGdvLocalMap()) - { - Compiler::NodeToUnsignedMap* const map = m_compiler->getImpEnumeratorGdvLocalMap(); - unsigned enumeratorLcl = BAD_VAR_NUM; - if (map->Lookup(m_origCall, &enumeratorLcl)) - { - JITDUMP("Flagging [%06u] for enumerator cloning via V%02u\n", m_compiler->dspTreeID(call), - enumeratorLcl); - map->Remove(m_origCall); - map->Set(call, enumeratorLcl); - } - } - INDEBUG(call->SetIsGuarded()); JITDUMP("Direct call [%06u] in block " FMT_BB "\n", m_compiler->dspTreeID(call), block->bbNum); @@ -1022,16 +1010,29 @@ class IndirectCallTransformer // assert(!call->IsVirtual() && !call->IsDelegateInvoke()); - // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info - // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. + // We won't inline this call if either: + // 1. the candidate was kept for devirtualization only (the target isn't inlineable), or + // 2. the devirtualizer was unable to transform the call to invoke the unboxed entry, + // in which case the inline info we set up may be invalid. + // + // In both cases we keep the direct call, we just don't (re-)mark it as a candidate. // CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedResolvedToken.hMethod; - if ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd)) + const bool unboxedEntryMismatch = (unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd); + + if (!inlineInfo->isInlineable || unboxedEntryMismatch) { - // Demote this call to a non-inline candidate - // - JITDUMP("Devirtualization was unable to use the unboxed entry; so marking call (to boxed entry) as not " - "inlineable\n"); + if (unboxedEntryMismatch) + { + JITDUMP("Devirtualization was unable to use the unboxed entry; so marking call (to boxed entry) as " + "not inlineable\n"); + } + else + { + JITDUMP("Target of this GDV candidate is not inlineable; leaving the devirtualized call as a plain " + "direct call\n"); + m_compiler->Metrics.NoInlineGDV++; + } call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; call->ClearInlineInfo(); @@ -1048,6 +1049,26 @@ class IndirectCallTransformer } else { + // If the original call was flagged as one that might inspire enumerator de-abstraction + // cloning, move the flag to the devirtualized call. + // + // Note this only pays off if we go on to inline the call, so we deliberately do it here + // rather than right after the clone: that way a candidate we're not going to inline + // doesn't consume the mapping and hide it from a subsequent candidate we will inline. + // + if (m_compiler->hasImpEnumeratorGdvLocalMap()) + { + Compiler::NodeToUnsignedMap* const map = m_compiler->getImpEnumeratorGdvLocalMap(); + unsigned enumeratorLcl = BAD_VAR_NUM; + if (map->Lookup(m_origCall, &enumeratorLcl)) + { + JITDUMP("Flagging [%06u] for enumerator cloning via V%02u\n", m_compiler->dspTreeID(call), + enumeratorLcl); + map->Remove(m_origCall); + map->Set(call, enumeratorLcl); + } + } + // Add the call. // m_compiler->fgNewStmtAtEnd(block, call, m_stmt->GetDebugInfo()); diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index 7e307f49d50d8c..816607e0e152a8 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -621,6 +621,14 @@ struct InlineCandidateInfo : public HandleHistogramProfileCandidateInfo unsigned clsAttr; unsigned methAttr; + // True if the target of this candidate can be inlined. + // + // Guarded devirtualization candidates are kept around even when the target can't be + // inlined (a direct call is still better than a virtual one), so this is what tells + // the two apart once the candidate is expanded. + // + bool isInlineable; + CorInfoInitClassResult initClassResult; InlineContext* inlinersContext; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 1e95c26b1e31cf..0a10cb25780c24 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -745,6 +745,12 @@ RELEASE_CONFIG_INTEGER(JitEnableGuardedDevirtualization, "JitEnableGuardedDevirt // Max number is MAX_GDV_TYPE_CHECKS defined above ^. -1 means it's up to JIT to decide RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, "JitGuardedDevirtualizationMaxTypeChecks", -1) +// Whether a guarded devirtualization candidate is required to be inlineable. +// 0 - keep the candidate even if we're not going to inline it: a direct call is still +// cheaper than a virtual/interface call and unlocks further optimizations. +// 1 - drop the candidate if the devirtualized target can't be inlined (legacy behavior). +RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationRequireInlining, "JitGuardedDevirtualizationRequireInlining", 0) + // Various policies for GuardedDevirtualization (0x4B == 75) RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, "JitGuardedDevirtualizationChainLikelihood", 0x4B) RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationChainStatements, "JitGuardedDevirtualizationChainStatements", 1) diff --git a/src/coreclr/jit/jitmetadatalist.h b/src/coreclr/jit/jitmetadatalist.h index db90f3a2932f41..3ac13fc14b261d 100644 --- a/src/coreclr/jit/jitmetadatalist.h +++ b/src/coreclr/jit/jitmetadatalist.h @@ -63,6 +63,7 @@ JITMETADATAMETRIC(GDV, int, 0) JITMETADATAMETRIC(ClassGDV, int, 0) JITMETADATAMETRIC(MethodGDV, int, 0) JITMETADATAMETRIC(MultiGuessGDV, int, 0) +JITMETADATAMETRIC(NoInlineGDV, int, 0) JITMETADATAMETRIC(ChainedGDV, int, 0) JITMETADATAMETRIC(EnumeratorGDV, int, 0) JITMETADATAMETRIC(InlinerBranchFold, int, 0) From 1480b6fb1c65309608930dd20788955717a54fb4 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 16 Aug 2026 18:53:12 +0200 Subject: [PATCH 2/3] Clarify the tail call bail-outs in canKeepNonInlineableGdvCandidate Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38498b8d-c116-4637-b069-d1d275634193 --- src/coreclr/jit/importercalls.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 0dd97c636d7e0a..ff31490fbce512 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -9153,14 +9153,20 @@ bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) return false; } - // The expansion moves the call out of the tail position, so leave tail calls alone. + // An explicit tail call has to stay a tail call, so don't perturb its shape. + // (mirrors CALLSITE_EXPLICIT_TAIL_PREFIX in impMarkInlineCandidateHelper) + // + // Note implicit tail calls are fine: fgMorphPotentialTailCall knows how to tail call + // out of the BBJ_ALWAYS blocks the expansion produces, so both the devirtualized call + // and the fallback still end up as tail calls. // if (call->IsTailPrefixedCall()) { return false; } - // Tail recursion elimination (turning the call into a loop) is more valuable. + // Except for recursive ones, where turning the call into a loop is more valuable. + // (mirrors CALLSITE_IMPLICIT_REC_TAIL_CALL in impMarkInlineCandidateHelper) // if (call->IsImplicitTailCall() && gtIsRecursiveCall(call)) { From 8737c902d3f5d6dc833eec9d1eac690c2176a759 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 16 Aug 2026 19:09:53 +0200 Subject: [PATCH 3/3] Trim comments Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38498b8d-c116-4637-b069-d1d275634193 --- src/coreclr/jit/importercalls.cpp | 42 +++++++-------------- src/coreclr/jit/indirectcalltransformer.cpp | 28 +++++--------- src/coreclr/jit/inline.h | 7 +--- src/coreclr/jit/jitconfigvalues.h | 7 ++-- 4 files changed, 29 insertions(+), 55 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ff31490fbce512..891310083d0319 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -2165,7 +2165,7 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN // No need to assign a multi-reg struct to a local var if: // - It is a tail call or // - The call is marked for in-lining later or - // - The call is a guarded devirtualization candidate (we defer this until we know its fate) + // - The call is a guarded devirtualization candidate (fate not yet known) return impStoreMultiRegValueToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); } return call; @@ -9035,9 +9035,8 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, // Gather some information for later. Note we actually allocate InlineCandidateInfo // here, as the devirtualized half of this call will likely become an inline candidate. // - // Value-initialize: a GDV candidate can now survive all the way through the expansion - // in fgTransformIndirectCalls without ever being seen by impCheckCanInline, so every - // field has to be in a well-defined state from the start. + // Value-initialize: the candidate may be expanded without ever going through + // impCheckCanInline, so every field has to be in a well-defined state. // InlineCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo{}; @@ -9050,8 +9049,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, pInfo->methAttr = methodAttr; pInfo->preexistingSpillTemp = BAD_VAR_NUM; - // Note: the call node only carries its IL offset in debug builds, and not this early, - // so we can't record a meaningful offset here. It is only used for reporting. + // The call only carries its IL offset in debug builds, and not this early. // pInfo->ilOffset = BAD_IL_OFFSET; @@ -9127,13 +9125,8 @@ void Compiler::impConvertToUserCallAndMarkForInlining(GenTreeCall* call) // true if the candidate can be kept just for the sake of devirtualization. // // Notes: -// A direct call is normally cheaper than a virtual/interface call and it unlocks -// further optimizations (exact type of 'this', better inlining of the callee's -// callees, etc.), so by default we keep such candidates around. There are a few -// call sites, however, where the expansion itself is either illegal or would -// interfere with a more valuable optimization. -// -// The bail-outs below mirror the call-site legality checks in +// A direct call is cheaper than a virtual/interface call, so we keep such candidates +// by default. The bail-outs below mirror the call-site checks in // impMarkInlineCandidateHelper; keep the two in sync. // bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) @@ -9145,8 +9138,7 @@ bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) return false; } - // For now this is limited to class-based GDV. Method-based (e.g. delegate) GDV keeps - // the old behavior of requiring the target to be inlineable. + // Class-based GDV only for now; method-based (e.g. delegate) GDV still requires inlining. // if (call->GetGDVCandidateInfo(0)->guardedClassHandle == NO_CLASS_HANDLE) { @@ -9154,26 +9146,22 @@ bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) } // An explicit tail call has to stay a tail call, so don't perturb its shape. - // (mirrors CALLSITE_EXPLICIT_TAIL_PREFIX in impMarkInlineCandidateHelper) - // - // Note implicit tail calls are fine: fgMorphPotentialTailCall knows how to tail call - // out of the BBJ_ALWAYS blocks the expansion produces, so both the devirtualized call - // and the fallback still end up as tail calls. + // Implicit ones are fine: fgMorphPotentialTailCall can tail call out of the + // BBJ_ALWAYS blocks the expansion produces. // if (call->IsTailPrefixedCall()) { return false; } - // Except for recursive ones, where turning the call into a loop is more valuable. - // (mirrors CALLSITE_IMPLICIT_REC_TAIL_CALL in impMarkInlineCandidateHelper) + // Except recursive ones, where turning the call into a loop is more valuable. // if (call->IsImplicitTailCall() && gtIsRecursiveCall(call)) { return false; } - // The NextCallReturnAddress intrinsic needs the call to stay exactly where it is. + // NextCallReturnAddress needs the call to stay exactly where it is. // if (info.compHasNextCallRetAddr) { @@ -9194,8 +9182,8 @@ bool Compiler::canKeepNonInlineableGdvCandidate(GenTreeCall* call) // // Notes: // Mostly a wrapper for impMarkInlineCandidateHelper that also undoes -// guarded devirtualization for virtual calls where the guarded devirtualization -// itself is not worth doing (or not legal) once we know we can't inline the target. +// guarded devirtualization when it's not worth doing (or not legal) once +// we know we can't inline the target. void Compiler::impMarkInlineCandidate(GenTree* callNode, CORINFO_CONTEXT_HANDLE exactContextHnd, @@ -9218,7 +9206,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { assert(call->GetInlineCandidatesCount() > 0); - // If the target can't be inlined we normally still want to devirtualize it, + // We usually still want to devirtualize a target we can't inline, // see canKeepNonInlineableGdvCandidate for the exceptions. // const bool keepNonInlineable = canKeepNonInlineableGdvCandidate(call); @@ -9242,8 +9230,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, JITDUMP("Keeping GDV candidate %u of call [%06u] for devirtualization only: target can't be inlined\n", candidateId, dspTreeID(call)); - // We only keep class-based candidates, and only ones we won't inline. - // assert(!call->GetGDVCandidateInfo(candidateId)->isInlineable); assert(call->GetGDVCandidateInfo(candidateId)->guardedClassHandle != NO_CLASS_HANDLE); } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 1fb1d17e6baee7..b45d10f96e23c3 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -838,12 +838,8 @@ class IndirectCallTransformer } else { - // If there's a spill temp already associated with any of the candidates, - // use that instead of allocating a new temp. - // - // Only candidates that made it through impMarkInlineCandidateHelper get a spill - // temp assigned, so we can't just look at candidate 0: it may be a candidate we - // kept for devirtualization only while a later one carries the inliner's temp. + // Only candidates that made it through impMarkInlineCandidateHelper get a + // spill temp, so candidate 0 may not be the one carrying it. // m_returnTemp = BAD_VAR_NUM; for (uint8_t i = 0; i < m_origCall->GetInlineCandidatesCount(); i++) @@ -851,7 +847,7 @@ class IndirectCallTransformer const unsigned spillTemp = m_origCall->GetGDVCandidateInfo(i)->preexistingSpillTemp; if (spillTemp != BAD_VAR_NUM) { - // All candidates share the same call site, so they must agree. + // Same call site, so all candidates must agree. assert((m_returnTemp == BAD_VAR_NUM) || (m_returnTemp == spillTemp)); m_returnTemp = spillTemp; } @@ -1010,12 +1006,9 @@ class IndirectCallTransformer // assert(!call->IsVirtual() && !call->IsDelegateInvoke()); - // We won't inline this call if either: - // 1. the candidate was kept for devirtualization only (the target isn't inlineable), or - // 2. the devirtualizer was unable to transform the call to invoke the unboxed entry, - // in which case the inline info we set up may be invalid. - // - // In both cases we keep the direct call, we just don't (re-)mark it as a candidate. + // Don't inline if the candidate was kept for devirtualization only, or if the + // devirtualizer couldn't use the unboxed entry (which invalidates the inline info). + // Either way we keep the direct call, we just don't re-mark it as a candidate. // CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedResolvedToken.hMethod; const bool unboxedEntryMismatch = (unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd); @@ -1049,12 +1042,11 @@ class IndirectCallTransformer } else { - // If the original call was flagged as one that might inspire enumerator de-abstraction - // cloning, move the flag to the devirtualized call. + // If the original call was flagged as one that might inspire enumerator + // de-abstraction cloning, move the flag to the devirtualized call. // - // Note this only pays off if we go on to inline the call, so we deliberately do it here - // rather than right after the clone: that way a candidate we're not going to inline - // doesn't consume the mapping and hide it from a subsequent candidate we will inline. + // Done here rather than right after the clone so a candidate we won't inline + // doesn't consume the mapping and hide it from one we will. // if (m_compiler->hasImpEnumeratorGdvLocalMap()) { diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index 816607e0e152a8..28452a567fb1de 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -621,11 +621,8 @@ struct InlineCandidateInfo : public HandleHistogramProfileCandidateInfo unsigned clsAttr; unsigned methAttr; - // True if the target of this candidate can be inlined. - // - // Guarded devirtualization candidates are kept around even when the target can't be - // inlined (a direct call is still better than a virtual one), so this is what tells - // the two apart once the candidate is expanded. + // True if the target of this candidate can be inlined. GDV candidates are kept + // around even when it can't be, so this is what tells the two apart. // bool isInlineable; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 0a10cb25780c24..07f581bd59e61a 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -745,10 +745,9 @@ RELEASE_CONFIG_INTEGER(JitEnableGuardedDevirtualization, "JitEnableGuardedDevirt // Max number is MAX_GDV_TYPE_CHECKS defined above ^. -1 means it's up to JIT to decide RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, "JitGuardedDevirtualizationMaxTypeChecks", -1) -// Whether a guarded devirtualization candidate is required to be inlineable. -// 0 - keep the candidate even if we're not going to inline it: a direct call is still -// cheaper than a virtual/interface call and unlocks further optimizations. -// 1 - drop the candidate if the devirtualized target can't be inlined (legacy behavior). +// Whether a guarded devirtualization candidate has to be inlineable. +// 0 - keep it even if we won't inline it, a direct call is still cheaper. +// 1 - drop it if the target can't be inlined (legacy behavior). RELEASE_CONFIG_INTEGER(JitGuardedDevirtualizationRequireInlining, "JitGuardedDevirtualizationRequireInlining", 0) // Various policies for GuardedDevirtualization (0x4B == 75)