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..891310083d0319 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 (fate not yet known) return impStoreMultiRegValueToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); } return call; @@ -9034,16 +9035,23 @@ 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: 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{}; + + 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; + + // The call only carries its IL offset in debug builds, and not this early. + // + pInfo->ilOffset = BAD_IL_OFFSET; if (instParamLookup != nullptr) { @@ -9106,6 +9114,63 @@ 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 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) +{ + assert(call->IsGuardedDevirtualizationCandidate()); + + if (JitConfig.JitGuardedDevirtualizationRequireInlining() != 0) + { + return false; + } + + // Class-based GDV only for now; method-based (e.g. delegate) GDV still requires inlining. + // + if (call->GetGDVCandidateInfo(0)->guardedClassHandle == NO_CLASS_HANDLE) + { + return false; + } + + // An explicit tail call has to stay a tail call, so don't perturb its shape. + // Implicit ones are fine: fgMorphPotentialTailCall can tail call out of the + // BBJ_ALWAYS blocks the expansion produces. + // + if (call->IsTailPrefixedCall()) + { + return false; + } + + // Except recursive ones, where turning the call into a loop is more valuable. + // + if (call->IsImplicitTailCall() && gtIsRecursiveCall(call)) + { + return false; + } + + // NextCallReturnAddress 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 +9182,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 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, @@ -9140,19 +9205,33 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsGuardedDevirtualizationCandidate()) { assert(call->GetInlineCandidatesCount() > 0); + + // We usually still want to devirtualize a target we can't inline, + // 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)); + + assert(!call->GetGDVCandidateInfo(candidateId)->isInlineable); + assert(call->GetGDVCandidateInfo(candidateId)->guardedClassHandle != NO_CLASS_HANDLE); } } @@ -9170,25 +9249,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 +9537,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 +11107,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..b45d10f96e23c3 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,20 @@ class IndirectCallTransformer } else { - // If there's a spill temp already associated with this inline candidate, - // use that instead of allocating a new temp. + // Only candidates that made it through impMarkInlineCandidateHelper get a + // spill temp, so candidate 0 may not be the one carrying it. // - m_returnTemp = inlineInfo->preexistingSpillTemp; + 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) + { + // Same call site, so all candidates must agree. + assert((m_returnTemp == BAD_VAR_NUM) || (m_returnTemp == spillTemp)); + m_returnTemp = spillTemp; + } + } if (m_returnTemp != BAD_VAR_NUM) { @@ -958,22 +958,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 +1006,26 @@ 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. + // 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; - 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 +1042,25 @@ 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. + // + // 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()) + { + 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..28452a567fb1de 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -621,6 +621,11 @@ struct InlineCandidateInfo : public HandleHistogramProfileCandidateInfo unsigned clsAttr; unsigned methAttr; + // 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; + CorInfoInitClassResult initClassResult; InlineContext* inlinersContext; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 1e95c26b1e31cf..07f581bd59e61a 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -745,6 +745,11 @@ 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 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) 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)