From d418631fa53e0e173620425ecdda612e8dc8ee7b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 May 2023 15:08:42 +0200 Subject: [PATCH 01/17] Multiple GDV (NAOT only for now) --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/gentree.cpp | 18 ++- src/coreclr/jit/gentree.h | 19 ++- src/coreclr/jit/importercalls.cpp | 158 ++++++++++++++++---- src/coreclr/jit/indirectcalltransformer.cpp | 74 +++++---- 5 files changed, 208 insertions(+), 65 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 3edbf2b1475da..c56277e6b00c6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4356,6 +4356,7 @@ class Compiler InlineResult* inlineResult); void impCheckCanInline(GenTreeCall* call, + uint8_t candidateIndex, CORINFO_METHOD_HANDLE fncHandle, unsigned methAttr, CORINFO_CONTEXT_HANDLE exactContextHnd, @@ -4383,7 +4384,8 @@ class Compiler CORINFO_CALL_INFO* callInfo, IL_OFFSET ilOffset); - void impMarkInlineCandidateHelper(GenTreeCall* call, + bool impMarkInlineCandidateHelper(GenTreeCall* call, + uint8_t candidateIndex, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo, diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3529a344ea0fe..bf817fee34c49 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2189,16 +2189,22 @@ void GenTreeCall::SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo { if (candidateInfo != nullptr) { - gtFlags |= GTF_CALL_INLINE_CANDIDATE; gtInlineInfoCount = 1; + gtFlags |= GTF_CALL_INLINE_CANDIDATE; } else { gtInlineInfoCount = 0; gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; } gtInlineCandidateInfo = candidateInfo; + ClearGuardedDevirtualizationCandidate(); +} + +void GenTreeCall::UpdateGDVCandateInfo(uint8_t index, InlineCandidateInfo* newInfo) +{ + assert(index < gtInlineInfoCount); + gtInlineCandidateInfo[index] = *newInfo; } //------------------------------------------------------------------------- @@ -2218,9 +2224,10 @@ InlineCandidateInfo* GenTreeCall::GetGDVCandidateInfo(uint8_t index) // for this call. For now, we only support one GDV candidate per call. // // Arguments: +// comp - Compiler instance // candidateInfo - GDV candidate info // -void GenTreeCall::AddGDVCandidateInfo(InlineCandidateInfo* candidateInfo) +void GenTreeCall::AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candidateInfo) { assert(candidateInfo != nullptr); if (gtInlineInfoCount == 0) @@ -2229,8 +2236,9 @@ void GenTreeCall::AddGDVCandidateInfo(InlineCandidateInfo* candidateInfo) } else { - // Allocate a fixed list of InlineCandidateInfo structs - assert(!"multiple GDV candidates are not implemented yet"); + // We only support two candidates for now ( + assert(gtInlineInfoCount == 1); + gtInlineCandidateInfo = new (comp, CMK_Inlining) InlineCandidateInfo[2]{*gtInlineCandidateInfo, *candidateInfo}; } gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 3462654705207..2280504aa04d6 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4049,6 +4049,7 @@ enum GenTreeCallFlags : unsigned int GTF_CALL_M_DEVIRTUALIZED = 0x00040000, // this call was devirtualized GTF_CALL_M_UNBOXED = 0x00080000, // this call was optimized to use the unboxed entry point GTF_CALL_M_GUARDED_DEVIRT = 0x00100000, // this call is a candidate for guarded devirtualization + GTF_CALL_M_GUARDED_DEVIRT_EXACT = 0x80000000, // this call is a candidate for guarded devirtualization where no fallback is needed GTF_CALL_M_GUARDED_DEVIRT_CHAIN = 0x00200000, // this call is a candidate for chained guarded devirtualization GTF_CALL_M_GUARDED = 0x00400000, // this call was transformed by guarded devirtualization GTF_CALL_M_ALLOC_SIDE_EFFECTS = 0x00800000, // this is a call to an allocator with side effects @@ -5371,7 +5372,8 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { - gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; + gtCallMoreFlags &= + ~(GTF_CALL_M_GUARDED_DEVIRT | GTF_CALL_M_GUARDED_DEVIRT_CHAIN | GTF_CALL_M_GUARDED_DEVIRT_EXACT); } void SetIsGuarded() @@ -5431,20 +5433,31 @@ struct GenTreeCall final : public GenTree InlineCandidateInfo* GetInlineCandidateInfo() { + if (gtInlineInfoCount > 1) + { + assert(!"Call has multiple inline candidates - use GetGDVCandidateInfo instead"); + } return gtInlineCandidateInfo; } void SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo); - InlineCandidateInfo* GetGDVCandidateInfo(uint8_t index = 0); + void UpdateGDVCandateInfo(uint8_t index, InlineCandidateInfo* newInfo); + + InlineCandidateInfo* GetGDVCandidateInfo(uint8_t index); - void AddGDVCandidateInfo(InlineCandidateInfo* candidateInfo); + void AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candidateInfo); void ClearInlineInfo() { SetSingleInlineCadidateInfo(nullptr); } + uint8_t GetInlineCandidatesCount() + { + return gtInlineInfoCount; + } + //----------------------------------------------------------------------------------------- // GetIndirectionCellArgKind: Get the kind of indirection cell used by this call. // diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 61af557d583c2..ae0cc3f473e08 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1350,8 +1350,11 @@ var_types Compiler::impImportCall(OPCODE opcode, // TODO: Still using the widened type. GenTreeRetExpr* retExpr = gtNewInlineCandidateReturnExpr(call->AsCall(), genActualType(callRetTyp)); - // Link the retExpr to the call so if necessary we can manipulate it later. - origCall->GetInlineCandidateInfo()->retExpr = retExpr; + for (UINT8 i = 0; i < origCall->GetInlineCandidatesCount(); i++) + { + // Save link to retExpr to all gdv candidates just in case. + origCall->GetGDVCandidateInfo(i)->retExpr = retExpr; + } // Propagate retExpr as the placeholder for the call. call = retExpr; @@ -5858,6 +5861,66 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, { JITDUMP("Considering guarded devirtualization at IL offset %u (0x%x)\n", ilOffset, ilOffset); + // NativeAOT is the only target that currently supports getExactClasses-based GDV + // where we know the exact number of classes implementing the given base in compile-time + if (IsTargetAbi(CORINFO_NATIVEAOT_ABI) && (baseClass != NO_CLASS_HANDLE)) + { + const int maxExactClasses = 2; + CORINFO_CLASS_HANDLE exactClasses[maxExactClasses]; + if (info.compCompHnd->getExactClasses(baseClass, maxExactClasses, exactClasses) == maxExactClasses) + { + JITDUMP("We have exactly %d classes implementing %s:\n", maxExactClasses, eeGetClassName(baseClass)); + + // NOTE: The case where we have only a single implementation is handled naturally + // through gtGetClassHandle without help of GDV. + // + int skipped = 0; + for (int exactClsIdx = 0; exactClsIdx < maxExactClasses; exactClsIdx++) + { + CORINFO_CLASS_HANDLE exactCls = exactClasses[exactClsIdx]; + assert(exactCls != NO_CLASS_HANDLE); + + uint32_t clsAttrs = info.compCompHnd->getClassAttribs(exactCls); + + // The getExactClasses method is expected to return precise data, thus eliminating the need + // to check if it is stale. + // + assert((clsAttrs & CORINFO_FLG_ABSTRACT) == 0); + + JITDUMP(" %d) %s\n", exactClsIdx, eeGetClassName(exactCls)); + + // Figure out which method will be called. + // + CORINFO_DEVIRTUALIZATION_INFO dvInfo; + dvInfo.virtualMethod = baseMethod; + dvInfo.objClass = exactCls; + dvInfo.context = *pContextHandle; + dvInfo.exactContext = *pContextHandle; + dvInfo.pResolvedTokenVirtualMethod = nullptr; + + if (!info.compCompHnd->resolveVirtualMethod(&dvInfo)) + { + JITDUMP("Can't figure out which method would be invoked, sorry\n"); + return; + } + + CORINFO_METHOD_HANDLE exactMethod = dvInfo.devirtualizedMethod; + uint32_t exactMethodAttrs = info.compCompHnd->getMethodAttribs(exactMethod); + + // NOTE: This is currently used only with NativeAOT. In theory, we could also check if we + // have static PGO data to decide which class to guess first. Presumably, this is a rare case. + // + const int likelyHood = 100 / maxExactClasses; + + addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, + likelyHood); + } + + call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; + return; + } + } + // We currently only get likely class guesses when there is PGO data // with class profiles. // @@ -6093,7 +6156,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, } } - call->AddGDVCandidateInfo(pInfo); + call->AddGDVCandidateInfo(this, pInfo); } //------------------------------------------------------------------------ @@ -6119,8 +6182,22 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { GenTreeCall* call = callNode->AsCall(); - // Do the actual evaluation - impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset); + const uint8_t candidatesCount = call->GetInlineCandidatesCount(); + + for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) + { + // Do the actual evaluation + bool success = impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, + callInfo, ilOffset); + if (!success) + { + // TODO: we should not give up if one of the candidates fails to inline while others succeed. + // but that requires a bit more logic here (to strip out the failing candidates from the list) + // + call->ClearGuardedDevirtualizationCandidate(); + break; + } + } // If this call is an inline candidate or is not a guarded devirtualization // candidate, we're done. @@ -6148,6 +6225,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // // Arguments: // callNode -- call under scrutiny +// candidateIndex -- index of the inline candidate to evaluate // exactContextHnd -- context handle for inlining // exactContextNeedsRuntimeLookup -- true if context required runtime lookup // callInfo -- call info from VM @@ -6159,11 +6237,14 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // filled in the associated InlineCandidateInfo. // // If callNode is not an inline candidate, and the reason is -// something that is inherent to the method being called, the // method may be marked as "noinline" to short-circuit any // future assessments of calls to this method. +// +// Return value: +// true if we can inline the given inline candidate, false otherwise. -void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, +bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, + uint8_t candidateIndex, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo, @@ -6181,7 +6262,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, * figure out why we did not set MAXOPT for this compile. */ assert(!compIsForInlining()); - return; + return false; } InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); @@ -6190,21 +6271,21 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (opts.compDbgCode) { inlineResult.NoteFatal(InlineObservation::CALLER_DEBUG_CODEGEN); - return; + return false; } // Don't inline if inlining into this method is disabled. if (impInlineRoot()->m_inlineStrategy->IsInliningDisabled()) { inlineResult.NoteFatal(InlineObservation::CALLER_IS_JIT_NOINLINE); - return; + return false; } // Don't inline into callers that use the NextCallReturnAddress intrinsic. if (info.compHasNextCallRetAddr) { inlineResult.NoteFatal(InlineObservation::CALLER_USES_NEXT_CALL_RET_ADDR); - return; + return false; } // Inlining candidate determination needs to honor only IL tail prefix. @@ -6212,7 +6293,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsTailPrefixedCall()) { inlineResult.NoteFatal(InlineObservation::CALLSITE_EXPLICIT_TAIL_PREFIX); - return; + return false; } // Delegate Invoke method doesn't have a body and gets special cased instead. @@ -6220,7 +6301,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsDelegateInvoke() && !call->IsGuardedDevirtualizationCandidate()) { inlineResult.NoteFatal(InlineObservation::CALLEE_HAS_NO_BODY); - return; + return false; } // Tail recursion elimination takes precedence over inlining. @@ -6230,7 +6311,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (gtIsRecursiveCall(call) && call->IsImplicitTailCall()) { inlineResult.NoteFatal(InlineObservation::CALLSITE_IMPLICIT_REC_TAIL_CALL); - return; + return false; } if (call->IsVirtual()) @@ -6240,7 +6321,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (!call->IsGuardedDevirtualizationCandidate()) { inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT); - return; + return false; } } @@ -6250,14 +6331,14 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, { assert(!call->IsGuardedDevirtualizationCandidate()); inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_CALL_TO_HELPER); - return; + return false; } /* Ignore indirect calls */ if (call->gtCallType == CT_INDIRECT) { inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT_MANAGED); - return; + return false; } /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less @@ -6269,13 +6350,13 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - if (call->GetGDVCandidateInfo()->guardedMethodUnboxedEntryHandle != nullptr) + if (call->GetGDVCandidateInfo(candidateIndex)->guardedMethodUnboxedEntryHandle != nullptr) { - fncHandle = call->GetGDVCandidateInfo()->guardedMethodUnboxedEntryHandle; + fncHandle = call->GetGDVCandidateInfo(candidateIndex)->guardedMethodUnboxedEntryHandle; } else { - fncHandle = call->GetGDVCandidateInfo()->guardedMethodHandle; + fncHandle = call->GetGDVCandidateInfo(candidateIndex)->guardedMethodHandle; } methAttr = info.compCompHnd->getMethodAttribs(fncHandle); } @@ -6321,7 +6402,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, #endif inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_CATCH); - return; + return false; } if (bbInFilterILRange(compCurBB)) @@ -6334,7 +6415,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, #endif inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_FILTER); - return; + return false; } } @@ -6343,7 +6424,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (methAttr & CORINFO_FLG_DONT_INLINE) { inlineResult.NoteFatal(InlineObservation::CALLEE_IS_NOINLINE); - return; + return false; } /* Cannot inline synchronized methods */ @@ -6351,7 +6432,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (methAttr & CORINFO_FLG_SYNCH) { inlineResult.NoteFatal(InlineObservation::CALLEE_IS_SYNCHRONIZED); - return; + return false; } /* Check legality of PInvoke callsite (for inlining of marshalling code) */ @@ -6363,16 +6444,16 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (!impCanPInvokeInlineCallSite(block)) { inlineResult.NoteFatal(InlineObservation::CALLSITE_PINVOKE_EH); - return; + return false; } } InlineCandidateInfo* inlineCandidateInfo = nullptr; - impCheckCanInline(call, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, &inlineResult); + impCheckCanInline(call, candidateIndex, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, &inlineResult); if (inlineResult.IsFailure()) { - return; + return false; } // The old value should be null OR this call should be a guarded devirtualization candidate. @@ -6382,7 +6463,18 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, assert(inlineCandidateInfo != nullptr); inlineCandidateInfo->exactContextNeedsRuntimeLookup = exactContextNeedsRuntimeLookup; inlineCandidateInfo->ilOffset = ilOffset; - call->SetSingleInlineCadidateInfo(inlineCandidateInfo); + + if (call->IsGuardedDevirtualizationCandidate()) + { + assert(call->GetGDVCandidateInfo(candidateIndex) != nullptr); + call->UpdateGDVCandateInfo(candidateIndex, inlineCandidateInfo); + call->gtFlags |= GTF_CALL_INLINE_CANDIDATE; + } + else + { + assert(candidateIndex == 0); + call->SetSingleInlineCadidateInfo(inlineCandidateInfo); + } // If we're in an inlinee compiler, and have a return spill temp, and this inline candidate // is also a tail call candidate, it can use the same return spill temp. @@ -6401,6 +6493,8 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, // Since we're not actually inlining yet, and this call site is // still just an inline candidate, there's nothing to report. inlineResult.SetSuccessResult(INLINE_CHECK_CAN_INLINE_SUCCESS); + + return true; } /******************************************************************************/ @@ -7525,6 +7619,7 @@ bool Compiler::impTailCallRetTypeCompatible(bool allowWideni // // Arguments: // call - inline candidate +// candidateIndex - index of inline candidate in the call's inline candidate list // fncHandle - method that will be called // methAttr - attributes for the method // exactContextHnd - exact context for the method @@ -7536,12 +7631,15 @@ bool Compiler::impTailCallRetTypeCompatible(bool allowWideni // status (if method cannot be inlined) // void Compiler::impCheckCanInline(GenTreeCall* call, + uint8_t candidateIndex, CORINFO_METHOD_HANDLE fncHandle, unsigned methAttr, CORINFO_CONTEXT_HANDLE exactContextHnd, InlineCandidateInfo** ppInlineCandidateInfo, InlineResult* inlineResult) { + assert(candidateIndex < call->GetInlineCandidatesCount()); + // Either EE or JIT might throw exceptions below. // If that happens, just don't inline the method. // @@ -7549,6 +7647,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, { Compiler* pThis; GenTreeCall* call; + uint8_t candidateIndex; CORINFO_METHOD_HANDLE fncHandle; unsigned methAttr; CORINFO_CONTEXT_HANDLE exactContextHnd; @@ -7559,6 +7658,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, param.pThis = this; param.call = call; + param.candidateIndex = candidateIndex; param.fncHandle = fncHandle; param.methAttr = methAttr; param.exactContextHnd = (exactContextHnd != nullptr) ? exactContextHnd : MAKE_METHODCONTEXT(fncHandle); @@ -7671,7 +7771,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, if (pParam->call->IsGuardedDevirtualizationCandidate()) { - pInfo = pParam->call->GetInlineCandidateInfo(); + pInfo = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex); } else { diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index fae20cb92c7cc..fa8ff2902bb35 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -465,7 +465,7 @@ class IndirectCallTransformer return; } - likelihood = origCall->GetGDVCandidateInfo()->likelihood; + likelihood = origCall->GetGDVCandidateInfo(0)->likelihood; assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); @@ -574,7 +574,7 @@ class IndirectCallTransformer // lastStmt = checkBlock->lastStmt(); - InlineCandidateInfo* guardedInfo = origCall->GetGDVCandidateInfo(); + InlineCandidateInfo* guardedInfo = origCall->GetGDVCandidateInfo(0); // Create comparison. On success we will jump to do the indirect call. GenTree* compare; @@ -655,7 +655,7 @@ class IndirectCallTransformer // // Note implicit by-ref returns should have already been converted // so any struct copy we induce here should be cheap. - InlineCandidateInfo* const inlineInfo = origCall->GetInlineCandidateInfo(); + InlineCandidateInfo* const inlineInfo = origCall->GetGDVCandidateInfo(0); if (!origCall->TypeIs(TYP_VOID)) { @@ -730,13 +730,11 @@ class IndirectCallTransformer } //------------------------------------------------------------------------ - // CreateThen: create then block with direct call to method + // Devirtualize origCall using the given inline candidate // - virtual void CreateThen() + void DevirtualizeCall(BasicBlock* block, uint8_t candidateId) { - thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); - thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; - InlineCandidateInfo* inlineInfo = origCall->GetInlineCandidateInfo(); + InlineCandidateInfo* inlineInfo = origCall->GetGDVCandidateInfo(candidateId); CORINFO_CLASS_HANDLE clsHnd = inlineInfo->guardedClassHandle; // @@ -771,7 +769,7 @@ class IndirectCallTransformer compiler->info.compCompHnd->getMethodClass(inlineInfo->guardedMethodHandle)); } - compiler->fgNewStmtAtEnd(thenBlock, assign); + compiler->fgNewStmtAtEnd(block, assign); // Clone call for the devirtualized case. Note we must use the // special candidate helper and we need to use the new 'this'. @@ -779,7 +777,7 @@ class IndirectCallTransformer call->gtArgs.GetThisArg()->SetEarlyNode(compiler->gtNewLclvNode(thisTemp, TYP_REF)); call->SetIsGuarded(); - JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), thenBlock->bbNum); + JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), block->bbNum); CORINFO_METHOD_HANDLE methodHnd = call->gtCallMethHnd; CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; @@ -845,18 +843,18 @@ class IndirectCallTransformer if (returnTemp != BAD_VAR_NUM) { GenTree* const assign = compiler->gtNewTempAssign(returnTemp, call); - compiler->fgNewStmtAtEnd(thenBlock, assign); + compiler->fgNewStmtAtEnd(block, assign); } else { - compiler->fgNewStmtAtEnd(thenBlock, call, stmt->GetDebugInfo()); + compiler->fgNewStmtAtEnd(block, call, stmt->GetDebugInfo()); } } else { // Add the call. // - compiler->fgNewStmtAtEnd(thenBlock, call, stmt->GetDebugInfo()); + compiler->fgNewStmtAtEnd(block, call, stmt->GetDebugInfo()); // Re-establish this call as an inline candidate. // @@ -886,11 +884,22 @@ class IndirectCallTransformer // We should always have a return temp if we return results by value assert(origCall->TypeGet() == TYP_VOID); } - compiler->fgNewStmtAtEnd(thenBlock, newRetExpr); + compiler->fgNewStmtAtEnd(block, newRetExpr); } } } + //------------------------------------------------------------------------ + // CreateThen: create then block with direct call to method + // + virtual void CreateThen() + { + thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); + thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + + DevirtualizeCall(thenBlock, 0); + } + //------------------------------------------------------------------------ // CreateElse: create else block. This executes the original indirect call. // @@ -898,21 +907,32 @@ class IndirectCallTransformer { elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; - GenTreeCall* call = origCall; - Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); - call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - call->SetIsGuarded(); - - JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), elseBlock->bbNum); - - if (returnTemp != BAD_VAR_NUM) + if (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT) { - GenTree* assign = compiler->gtNewTempAssign(returnTemp, call); - newStmt->SetRootNode(assign); + // Use the 2nd inline candidate to devirtualize/inline the fallback call. + assert(origCall->GetInlineCandidatesCount() == 2); + DevirtualizeCall(elseBlock, 1); } + else + { + GenTreeCall* call = origCall; + Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); + + call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; + call->SetIsGuarded(); - compiler->fgInsertStmtAtEnd(elseBlock, newStmt); + JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), + elseBlock->bbNum); + + if (returnTemp != BAD_VAR_NUM) + { + GenTree* assign = compiler->gtNewTempAssign(returnTemp, call); + newStmt->SetRootNode(assign); + } + + compiler->fgInsertStmtAtEnd(elseBlock, newStmt); + } // Set the original statement to a nop. // @@ -1092,10 +1112,10 @@ class IndirectCallTransformer GenTreeCall* const call = root->AsCall(); if (call->IsGuardedDevirtualizationCandidate() && - (call->GetGDVCandidateInfo()->likelihood >= gdvChainLikelihood)) + (call->GetGDVCandidateInfo(0)->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", - compiler->dspTreeID(call), call->GetGDVCandidateInfo()->likelihood, gdvChainLikelihood, + compiler->dspTreeID(call), call->GetGDVCandidateInfo(0)->likelihood, gdvChainLikelihood, chainStatementDup, chainNodeDup); call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_CHAIN; From c0dd21fd4888b17c389cf06dab4d700a3c62a130 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 21 May 2023 15:23:32 +0200 Subject: [PATCH 02/17] Clean up --- src/coreclr/jit/gentree.cpp | 8 +------- src/coreclr/jit/gentree.h | 2 -- src/coreclr/jit/importercalls.cpp | 6 +++--- src/coreclr/jit/indirectcalltransformer.cpp | 9 +++++++-- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bf817fee34c49..3e66babfedf5a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2189,8 +2189,8 @@ void GenTreeCall::SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo { if (candidateInfo != nullptr) { - gtInlineInfoCount = 1; gtFlags |= GTF_CALL_INLINE_CANDIDATE; + gtInlineInfoCount = 1; } else { @@ -2201,12 +2201,6 @@ void GenTreeCall::SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo ClearGuardedDevirtualizationCandidate(); } -void GenTreeCall::UpdateGDVCandateInfo(uint8_t index, InlineCandidateInfo* newInfo) -{ - assert(index < gtInlineInfoCount); - gtInlineCandidateInfo[index] = *newInfo; -} - //------------------------------------------------------------------------- // GetGDVCandidateInfo: Get GDV candidate info in the current call by index. // diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 2280504aa04d6..a7d48df3cd9db 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5442,8 +5442,6 @@ struct GenTreeCall final : public GenTree void SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo); - void UpdateGDVCandateInfo(uint8_t index, InlineCandidateInfo* newInfo); - InlineCandidateInfo* GetGDVCandidateInfo(uint8_t index); void AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candidateInfo); diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ae0cc3f473e08..62e6807bf1ee0 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1352,7 +1352,7 @@ var_types Compiler::impImportCall(OPCODE opcode, for (UINT8 i = 0; i < origCall->GetInlineCandidatesCount(); i++) { - // Save link to retExpr to all gdv candidates just in case. + // Link the retExpr to the call so if necessary we can manipulate it later. origCall->GetGDVCandidateInfo(i)->retExpr = retExpr; } @@ -6457,7 +6457,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } // The old value should be null OR this call should be a guarded devirtualization candidate. - assert((call->GetInlineCandidateInfo() == nullptr) || call->IsGuardedDevirtualizationCandidate()); + assert(call->IsGuardedDevirtualizationCandidate() || (call->GetInlineCandidateInfo() == nullptr)); // The new value should not be null. assert(inlineCandidateInfo != nullptr); @@ -6467,7 +6467,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { assert(call->GetGDVCandidateInfo(candidateIndex) != nullptr); - call->UpdateGDVCandateInfo(candidateIndex, inlineCandidateInfo); + *call->GetGDVCandidateInfo(candidateIndex) = *inlineCandidateInfo; call->gtFlags |= GTF_CALL_INLINE_CANDIDATE; } else diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index fa8ff2902bb35..53d03be78eeb4 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -442,7 +442,7 @@ class IndirectCallTransformer { public: GuardedDevirtualizationTransformer(Compiler* compiler, BasicBlock* block, Statement* stmt) - : Transformer(compiler, block, stmt), returnTemp(BAD_VAR_NUM) + : Transformer(compiler, block, stmt), devirualizeFallback(false), returnTemp(BAD_VAR_NUM) { } @@ -476,6 +476,8 @@ class IndirectCallTransformer JITDUMP("Expansion will chain to the previous GDV\n"); } + devirualizeFallback = origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT; + Transform(); if (isChainedGdv) @@ -908,7 +910,7 @@ class IndirectCallTransformer elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; - if (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT) + if (devirualizeFallback) { // Use the 2nd inline candidate to devirtualize/inline the fallback call. assert(origCall->GetInlineCandidatesCount() == 2); @@ -916,6 +918,8 @@ class IndirectCallTransformer } else { + assert(origCall->GetInlineCandidatesCount() == 1); + GenTreeCall* call = origCall; Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); @@ -1152,6 +1156,7 @@ class IndirectCallTransformer } private: + bool devirualizeFallback; unsigned returnTemp; Statement* lastStmt; From c02e68b9fd57544394c646576e7827f4e4d0c702 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 22 May 2023 00:08:56 +0200 Subject: [PATCH 03/17] fix diffs --- src/coreclr/jit/importercalls.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 62e6807bf1ee0..1977a65fdc24a 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6182,8 +6182,10 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { GenTreeCall* call = callNode->AsCall(); - const uint8_t candidatesCount = call->GetInlineCandidatesCount(); - + // Call might not have an inline candidate info yet (will be set by impMarkInlineCandidateHelper) + // so we assume there is always a least one candidate: + // + const uint8_t candidatesCount = max(1, call->GetInlineCandidatesCount()); for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) { // Do the actual evaluation @@ -6191,9 +6193,16 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, callInfo, ilOffset); if (!success) { - // TODO: we should not give up if one of the candidates fails to inline while others succeed. - // but that requires a bit more logic here (to strip out the failing candidates from the list) - // + if (candidatesCount > 1) + { + // TODO: we should not give up if one of the candidates fails to inline while others succeed. + // but that requires a bit more logic here (to strip out the failing candidates from the list) + // + // Also, in that case we no longer can use the GTF_CALL_M_GUARDED_DEVIRT_EXACT trick + // + JITDUMP("We had multiple inline candidates but have to give up on them since one of them didn't pass" + "inline checks") + } call->ClearGuardedDevirtualizationCandidate(); break; } @@ -7638,8 +7647,6 @@ void Compiler::impCheckCanInline(GenTreeCall* call, InlineCandidateInfo** ppInlineCandidateInfo, InlineResult* inlineResult) { - assert(candidateIndex < call->GetInlineCandidatesCount()); - // Either EE or JIT might throw exceptions below. // If that happens, just don't inline the method. // From 782133b3cca284be96fa92fcec0c4cf420bcfd32 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 22 May 2023 03:08:36 +0200 Subject: [PATCH 04/17] clean up --- src/coreclr/jit/gentree.cpp | 11 ++-- src/coreclr/jit/gentree.h | 4 +- src/coreclr/jit/importercalls.cpp | 64 ++++++++++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 39 ++++--------- src/coreclr/jit/inline.cpp | 2 +- 5 files changed, 56 insertions(+), 64 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3e66babfedf5a..86921140242e5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7807,10 +7807,11 @@ GenTreeCall* Compiler::gtNewCallNode(gtCallTypes callType, node->gtCallType = callType; node->gtCallMethHnd = callHnd; INDEBUG(node->callSig = nullptr;) - node->tailCallInfo = nullptr; - node->gtRetClsHnd = nullptr; - node->gtControlExpr = nullptr; - node->gtCallMoreFlags = GTF_CALL_M_EMPTY; + node->tailCallInfo = nullptr; + node->gtRetClsHnd = nullptr; + node->gtControlExpr = nullptr; + node->gtCallMoreFlags = GTF_CALL_M_EMPTY; + node->gtInlineInfoCount = 0; if (callType == CT_INDIRECT) { @@ -9469,8 +9470,8 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree, { copy->gtCallMethHnd = tree->gtCallMethHnd; copy->gtInlineCandidateInfo = tree->gtInlineCandidateInfo; - copy->gtInlineInfoCount = tree->gtInlineInfoCount; } + copy->gtInlineInfoCount = tree->gtInlineInfoCount; copy->gtCallType = tree->gtCallType; copy->gtReturnType = tree->gtReturnType; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index a7d48df3cd9db..c070822706798 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4049,7 +4049,6 @@ enum GenTreeCallFlags : unsigned int GTF_CALL_M_DEVIRTUALIZED = 0x00040000, // this call was devirtualized GTF_CALL_M_UNBOXED = 0x00080000, // this call was optimized to use the unboxed entry point GTF_CALL_M_GUARDED_DEVIRT = 0x00100000, // this call is a candidate for guarded devirtualization - GTF_CALL_M_GUARDED_DEVIRT_EXACT = 0x80000000, // this call is a candidate for guarded devirtualization where no fallback is needed GTF_CALL_M_GUARDED_DEVIRT_CHAIN = 0x00200000, // this call is a candidate for chained guarded devirtualization GTF_CALL_M_GUARDED = 0x00400000, // this call was transformed by guarded devirtualization GTF_CALL_M_ALLOC_SIDE_EFFECTS = 0x00800000, // this is a call to an allocator with side effects @@ -5372,8 +5371,7 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { - gtCallMoreFlags &= - ~(GTF_CALL_M_GUARDED_DEVIRT | GTF_CALL_M_GUARDED_DEVIRT_CHAIN | GTF_CALL_M_GUARDED_DEVIRT_EXACT); + gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; } void SetIsGuarded() diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 1977a65fdc24a..e87bea7b46c90 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5915,8 +5915,6 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, likelyHood); } - - call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; return; } } @@ -6185,28 +6183,38 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // Call might not have an inline candidate info yet (will be set by impMarkInlineCandidateHelper) // so we assume there is always a least one candidate: // - const uint8_t candidatesCount = max(1, call->GetInlineCandidatesCount()); - for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) + + if (call->IsGuardedDevirtualizationCandidate()) { - // Do the actual evaluation - bool success = impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, - callInfo, ilOffset); - if (!success) + const uint8_t candidatesCount = call->GetInlineCandidatesCount(); + assert(candidatesCount > 0); + for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) { - if (candidatesCount > 1) + // Do the actual evaluation + bool success = impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, + exactContextNeedsRuntimeLookup, callInfo, ilOffset); + if (!success) { - // TODO: we should not give up if one of the candidates fails to inline while others succeed. - // but that requires a bit more logic here (to strip out the failing candidates from the list) - // - // Also, in that case we no longer can use the GTF_CALL_M_GUARDED_DEVIRT_EXACT trick - // - JITDUMP("We had multiple inline candidates but have to give up on them since one of them didn't pass" + if (candidatesCount > 1) + { + // TODO: we should not give up if one of the candidates fails to inline while others succeed. + // + JITDUMP( + "We had multiple inline candidates but have to give up on them since one of them didn't pass" "inline checks") + call->ClearInlineInfo(); + call->ClearGuardedDevirtualizationCandidate(); + } + break; } - call->ClearGuardedDevirtualizationCandidate(); - break; } } + else + { + const uint8_t candidatesCount = call->GetInlineCandidatesCount(); + assert(candidatesCount <= 1); + impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset); + } // If this call is an inline candidate or is not a guarded devirtualization // candidate, we're done. @@ -6473,6 +6481,17 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, inlineCandidateInfo->exactContextNeedsRuntimeLookup = exactContextNeedsRuntimeLookup; inlineCandidateInfo->ilOffset = ilOffset; + // If we're in an inlinee compiler, and have a return spill temp, and this inline candidate + // is also a tail call candidate, it can use the same return spill temp. + // + if (compIsForInlining() && call->CanTailCall() && + (impInlineInfo->inlineCandidateInfo->preexistingSpillTemp != BAD_VAR_NUM)) + { + inlineCandidateInfo->preexistingSpillTemp = impInlineInfo->inlineCandidateInfo->preexistingSpillTemp; + JITDUMP("Inline candidate [%06u] can share spill temp V%02u\n", dspTreeID(call), + inlineCandidateInfo->preexistingSpillTemp); + } + if (call->IsGuardedDevirtualizationCandidate()) { assert(call->GetGDVCandidateInfo(candidateIndex) != nullptr); @@ -6485,17 +6504,6 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, call->SetSingleInlineCadidateInfo(inlineCandidateInfo); } - // If we're in an inlinee compiler, and have a return spill temp, and this inline candidate - // is also a tail call candidate, it can use the same return spill temp. - // - if (compIsForInlining() && call->CanTailCall() && - (impInlineInfo->inlineCandidateInfo->preexistingSpillTemp != BAD_VAR_NUM)) - { - inlineCandidateInfo->preexistingSpillTemp = impInlineInfo->inlineCandidateInfo->preexistingSpillTemp; - JITDUMP("Inline candidate [%06u] can share spill temp V%02u\n", dspTreeID(call), - inlineCandidateInfo->preexistingSpillTemp); - } - // Let the strategy know there's another candidate. impInlineRoot()->m_inlineStrategy->NoteCandidate(); diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 53d03be78eeb4..95fb741dae84e 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -442,7 +442,7 @@ class IndirectCallTransformer { public: GuardedDevirtualizationTransformer(Compiler* compiler, BasicBlock* block, Statement* stmt) - : Transformer(compiler, block, stmt), devirualizeFallback(false), returnTemp(BAD_VAR_NUM) + : Transformer(compiler, block, stmt), returnTemp(BAD_VAR_NUM) { } @@ -476,8 +476,6 @@ class IndirectCallTransformer JITDUMP("Expansion will chain to the previous GDV\n"); } - devirualizeFallback = origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT; - Transform(); if (isChainedGdv) @@ -910,34 +908,22 @@ class IndirectCallTransformer elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; - if (devirualizeFallback) - { - // Use the 2nd inline candidate to devirtualize/inline the fallback call. - assert(origCall->GetInlineCandidatesCount() == 2); - DevirtualizeCall(elseBlock, 1); - } - else - { - assert(origCall->GetInlineCandidatesCount() == 1); - - GenTreeCall* call = origCall; - Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); - - call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - call->SetIsGuarded(); + GenTreeCall* call = origCall; + Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); - JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), - elseBlock->bbNum); + call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; + call->SetIsGuarded(); - if (returnTemp != BAD_VAR_NUM) - { - GenTree* assign = compiler->gtNewTempAssign(returnTemp, call); - newStmt->SetRootNode(assign); - } + JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), elseBlock->bbNum); - compiler->fgInsertStmtAtEnd(elseBlock, newStmt); + if (returnTemp != BAD_VAR_NUM) + { + GenTree* assign = compiler->gtNewTempAssign(returnTemp, call); + newStmt->SetRootNode(assign); } + compiler->fgInsertStmtAtEnd(elseBlock, newStmt); + // Set the original statement to a nop. // stmt->SetRootNode(compiler->gtNewNothingNode()); @@ -1156,7 +1142,6 @@ class IndirectCallTransformer } private: - bool devirualizeFallback; unsigned returnTemp; Statement* lastStmt; diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index a9fffc513fe2a..1160062f3123d 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -749,7 +749,7 @@ void InlineResult::Report() if (IsFailure() && (m_Call != nullptr)) { // compiler should have revoked candidacy on the call by now - assert((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0); + assert(((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0) || m_Call->IsGuardedDevirtualizationCandidate()); if (m_Call->gtInlineObservation == InlineObservation::CALLEE_UNUSED_INITIAL) { From f94e2dc0301db6b1c310d4b0ea9cbd6d532b357e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 22 May 2023 22:03:29 +0200 Subject: [PATCH 05/17] Fix assert, clean up --- src/coreclr/jit/fginline.cpp | 8 ++++---- src/coreclr/jit/gentree.cpp | 9 +++++---- src/coreclr/jit/gentree.h | 2 +- src/coreclr/jit/importer.cpp | 15 ++++++++++++--- src/coreclr/jit/importercalls.cpp | 15 +++++++++++---- src/coreclr/jit/inline.cpp | 2 +- 6 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 7ad20ff1c228b..bd8e05f572d5a 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -752,7 +752,7 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) { bool inliningFailed = false; - InlineCandidateInfo* inlCandInfo = call->GetInlineCandidateInfo(); + InlineCandidateInfo* inlCandInfo = call->GetSingleInlineCandidateInfo(); // Is this call an inline candidate? if (call->IsInlineCandidate()) @@ -777,8 +777,8 @@ void Compiler::fgMorphCallInline(GenTreeCall* call, InlineResult* inlineResult) { #ifdef DEBUG // In debug we always put all inline attempts into the inline tree. - InlineContext* ctx = - m_inlineStrategy->NewContext(call->GetInlineCandidateInfo()->inlinersContext, fgMorphStmt, call); + InlineContext* ctx = m_inlineStrategy->NewContext(call->GetSingleInlineCandidateInfo()->inlinersContext, + fgMorphStmt, call); ctx->SetFailed(inlineResult); #endif } @@ -1045,7 +1045,7 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe inlineInfo.hasSIMDTypeArgLocalOrReturn = false; #endif // FEATURE_SIMD - InlineCandidateInfo* inlineCandidateInfo = call->GetInlineCandidateInfo(); + InlineCandidateInfo* inlineCandidateInfo = call->GetSingleInlineCandidateInfo(); noway_assert(inlineCandidateInfo); // Store the link to inlineCandidateInfo into inlineInfo inlineInfo.inlineCandidateInfo = inlineCandidateInfo; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 86921140242e5..bdd44f2c03711 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -12481,10 +12481,11 @@ void Compiler::gtDispTree(GenTree* tree, printf(" (FramesRoot last use)"); } - if (((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) && (call->GetInlineCandidateInfo() != nullptr) && - (call->GetInlineCandidateInfo()->exactContextHnd != nullptr)) + if (((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) && + (call->GetSingleInlineCandidateInfo() != nullptr) && + (call->GetSingleInlineCandidateInfo()->exactContextHnd != nullptr)) { - printf(" (exactContextHnd=0x%p)", dspPtr(call->GetInlineCandidateInfo()->exactContextHnd)); + printf(" (exactContextHnd=0x%p)", dspPtr(call->GetSingleInlineCandidateInfo()->exactContextHnd)); } gtDispCommonEndLine(tree); @@ -18053,7 +18054,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b // type class handle in the inline info (for GDV candidates, // this data is valid only for a correct guess, so we cannot // use it). - InlineCandidateInfo* inlInfo = call->GetInlineCandidateInfo(); + InlineCandidateInfo* inlInfo = call->GetSingleInlineCandidateInfo(); assert(inlInfo != nullptr); // Grab it as our first cut at a return type. diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index c070822706798..4b9d5e6c3c0e7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5429,7 +5429,7 @@ struct GenTreeCall final : public GenTree return (gtCallMoreFlags & GTF_CALL_M_RETBUFFARG_LCLOPT) != 0; } - InlineCandidateInfo* GetInlineCandidateInfo() + InlineCandidateInfo* GetSingleInlineCandidateInfo() { if (gtInlineInfoCount > 1) { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5d966c6122dbd..2cf31b6b877e6 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1701,9 +1701,18 @@ bool Compiler::impSpillStackEntry(unsigned level, if (tree->OperGet() == GT_RET_EXPR) { JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); - GenTree* call = tree->AsRetExpr()->gtInlineCandidate; - InlineCandidateInfo* ici = call->AsCall()->GetInlineCandidateInfo(); - ici->preexistingSpillTemp = tnum; + GenTreeCall* call = tree->AsRetExpr()->gtInlineCandidate->AsCall(); + if (call->IsGuardedDevirtualizationCandidate()) + { + for (uint8_t i = 0; i < call->GetInlineCandidatesCount(); i++) + { + call->GetGDVCandidateInfo(i)->preexistingSpillTemp = tnum; + } + } + else + { + call->AsCall()->GetSingleInlineCandidateInfo()->preexistingSpillTemp = tnum; + } } } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index e87bea7b46c90..25ea47b413182 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1350,10 +1350,17 @@ var_types Compiler::impImportCall(OPCODE opcode, // TODO: Still using the widened type. GenTreeRetExpr* retExpr = gtNewInlineCandidateReturnExpr(call->AsCall(), genActualType(callRetTyp)); - for (UINT8 i = 0; i < origCall->GetInlineCandidatesCount(); i++) + // Link the retExpr to the call so if necessary we can manipulate it later. + if (origCall->IsGuardedDevirtualizationCandidate()) { - // Link the retExpr to the call so if necessary we can manipulate it later. - origCall->GetGDVCandidateInfo(i)->retExpr = retExpr; + for (uint8_t i = 0; i < origCall->GetInlineCandidatesCount(); i++) + { + origCall->GetGDVCandidateInfo(i)->retExpr = retExpr; + } + } + else + { + origCall->GetSingleInlineCandidateInfo()->retExpr = retExpr; } // Propagate retExpr as the placeholder for the call. @@ -6474,7 +6481,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } // The old value should be null OR this call should be a guarded devirtualization candidate. - assert(call->IsGuardedDevirtualizationCandidate() || (call->GetInlineCandidateInfo() == nullptr)); + assert(call->IsGuardedDevirtualizationCandidate() || (call->GetSingleInlineCandidateInfo() == nullptr)); // The new value should not be null. assert(inlineCandidateInfo != nullptr); diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 1160062f3123d..9de025bfc9042 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -1281,7 +1281,7 @@ InlineContext* InlineStrategy::NewContext(InlineContext* parentContext, Statemen if (call->IsInlineCandidate()) { - InlineCandidateInfo* info = call->GetInlineCandidateInfo(); + InlineCandidateInfo* info = call->GetSingleInlineCandidateInfo(); context->m_Code = info->methInfo.ILCode; context->m_ILSize = info->methInfo.ILCodeSize; context->m_ActualCallOffset = info->ilOffset; From 1d8316ca5f81fec4d89fd01b7500c58e1d133c6b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 May 2023 16:02:12 +0200 Subject: [PATCH 06/17] Better approach --- src/coreclr/jit/gentree.cpp | 11 +- src/coreclr/jit/importercalls.cpp | 69 +++++++---- src/coreclr/jit/indirectcalltransformer.cpp | 130 ++++++++++++++++---- src/coreclr/jit/inline.cpp | 1 + src/coreclr/jit/jitconfigvalues.h | 5 + 5 files changed, 163 insertions(+), 53 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bdd44f2c03711..bbff478917af8 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2223,16 +2223,21 @@ InlineCandidateInfo* GenTreeCall::GetGDVCandidateInfo(uint8_t index) // void GenTreeCall::AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candidateInfo) { + assert(gtInlineInfoCount < MAX_GDV_TYPE_CHECKS); assert(candidateInfo != nullptr); + if (gtInlineInfoCount == 0) { gtInlineCandidateInfo = candidateInfo; } + else if (gtInlineInfoCount == 1) + { + gtInlineCandidateInfo = + new (comp, CMK_Inlining) InlineCandidateInfo[MAX_GDV_TYPE_CHECKS]{*gtInlineCandidateInfo, *candidateInfo}; + } else { - // We only support two candidates for now ( - assert(gtInlineInfoCount == 1); - gtInlineCandidateInfo = new (comp, CMK_Inlining) InlineCandidateInfo[2]{*gtInlineCandidateInfo, *candidateInfo}; + gtInlineCandidateInfo[gtInlineInfoCount] = *candidateInfo; } gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 25ea47b413182..398138daffda6 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5868,21 +5868,57 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, { JITDUMP("Considering guarded devirtualization at IL offset %u (0x%x)\n", ilOffset, ilOffset); + bool hasPgoData = true; + + CORINFO_CLASS_HANDLE likelyClass = NO_CLASS_HANDLE; + CORINFO_METHOD_HANDLE likelyMethod = NO_METHOD_HANDLE; + unsigned likelihood = 0; + + // We currently only get likely class guesses when there is PGO data + // with class profiles. + // + if ((fgPgoClassProfiles == 0) && (fgPgoMethodProfiles == 0)) + { + hasPgoData = false; + } + else + { + pickGDV(call, ilOffset, isInterface, &likelyClass, &likelyMethod, &likelihood); + if ((likelyClass == NO_CLASS_HANDLE) && (likelyMethod == NO_METHOD_HANDLE)) + { + hasPgoData = false; + } + } + // NativeAOT is the only target that currently supports getExactClasses-based GDV - // where we know the exact number of classes implementing the given base in compile-time - if (IsTargetAbi(CORINFO_NATIVEAOT_ABI) && (baseClass != NO_CLASS_HANDLE)) + // where we know the exact number of classes implementing the given base in compile-time. + // For now, let's only do this when we don't have any PGO data. In future, we should be able to benefit + // from both. + if (!hasPgoData && (baseClass != NO_CLASS_HANDLE)) { - const int maxExactClasses = 2; - CORINFO_CLASS_HANDLE exactClasses[maxExactClasses]; - if (info.compCompHnd->getExactClasses(baseClass, maxExactClasses, exactClasses) == maxExactClasses) + int maxTypeChecks = min(JitConfig.JitGuardedDevirtualizationMaxTypeChecks(), MAX_GDV_TYPE_CHECKS); + + CORINFO_CLASS_HANDLE exactClasses[MAX_GDV_TYPE_CHECKS]; + int numExactClasses = info.compCompHnd->getExactClasses(baseClass, MAX_GDV_TYPE_CHECKS, exactClasses); + if (numExactClasses == 0) { - JITDUMP("We have exactly %d classes implementing %s:\n", maxExactClasses, eeGetClassName(baseClass)); + JITDUMP("No exact classes implementing %s\n", eeGetClassName(baseClass)) + } + else if (numExactClasses > maxTypeChecks) + { + JITDUMP("Too many exact classes implementing %s (%d > %d)\n", eeGetClassName(baseClass), numExactClasses, + maxTypeChecks) + } + else + { + assert(numExactClasses <= maxTypeChecks); + JITDUMP("We have exactly %d classes implementing %s:\n", numExactClasses, eeGetClassName(baseClass)); // NOTE: The case where we have only a single implementation is handled naturally // through gtGetClassHandle without help of GDV. // int skipped = 0; - for (int exactClsIdx = 0; exactClsIdx < maxExactClasses; exactClsIdx++) + for (int exactClsIdx = 0; exactClsIdx < numExactClasses; exactClsIdx++) { CORINFO_CLASS_HANDLE exactCls = exactClasses[exactClsIdx]; assert(exactCls != NO_CLASS_HANDLE); @@ -5917,7 +5953,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // NOTE: This is currently used only with NativeAOT. In theory, we could also check if we // have static PGO data to decide which class to guess first. Presumably, this is a rare case. // - const int likelyHood = 100 / maxExactClasses; + const int likelyHood = 100 / numExactClasses; addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, likelyHood); @@ -5926,22 +5962,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, } } - // We currently only get likely class guesses when there is PGO data - // with class profiles. - // - if ((fgPgoClassProfiles == 0) && (fgPgoMethodProfiles == 0)) - { - JITDUMP("Not guessing for class or method: no GDV profile pgo data, or pgo disabled\n"); - return; - } - - CORINFO_CLASS_HANDLE likelyClass; - CORINFO_METHOD_HANDLE likelyMethod; - unsigned likelihood; - pickGDV(call, ilOffset, isInterface, &likelyClass, &likelyMethod, &likelihood); - - if ((likelyClass == NO_CLASS_HANDLE) && (likelyMethod == NO_METHOD_HANDLE)) + if (!hasPgoData) { + JITDUMP("Not guessing; no PGO and no exact classes\n"); return; } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 95fb741dae84e..26592be37bdb2 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -181,8 +181,12 @@ class IndirectCallTransformer FixupRetExpr(); ClearFlag(); CreateRemainder(); - CreateCheck(); - CreateThen(); + assert(GetChecksCount() > 0); + for (uint8_t i = 0; i < GetChecksCount(); i++) + { + CreateCheck(i); + CreateThen(i); + } CreateElse(); RemoveOldStatement(); SetWeights(); @@ -205,7 +209,7 @@ class IndirectCallTransformer remainderBlock->bbFlags |= BBF_INTERNAL; } - virtual void CreateCheck() = 0; + virtual void CreateCheck(uint8_t checkIdx) = 0; //------------------------------------------------------------------------ // CreateAndInsertBasicBlock: ask compiler to create new basic block. @@ -224,8 +228,16 @@ class IndirectCallTransformer return block; } - virtual void CreateThen() = 0; - virtual void CreateElse() = 0; + virtual void CreateThen(uint8_t checkIdx) = 0; + virtual void CreateElse() = 0; + + //------------------------------------------------------------------------ + // GetChecksCount: Get number of Check-Then pairs + // + virtual UINT8 GetChecksCount() + { + return 1; + } //------------------------------------------------------------------------ // RemoveOldStatement: remove original stmt from current block. @@ -345,8 +357,10 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateCheck: create check block, that checks fat pointer bit set. // - virtual void CreateCheck() + virtual void CreateCheck(uint8_t checkIdx) { + assert(checkIdx == 0); + checkBlock = CreateAndInsertBasicBlock(BBJ_COND, currBlock); GenTree* fatPointerMask = new (compiler, GT_CNS_INT) GenTreeIntCon(TYP_I_IMPL, FAT_POINTER_MASK); GenTree* fptrAddressCopy = compiler->gtCloneExpr(fptrAddress); @@ -362,7 +376,7 @@ class IndirectCallTransformer // CreateThen: create then block, that is executed if the check succeeds. // This simply executes the original call. // - virtual void CreateThen() + virtual void CreateThen(uint8_t checkIdx) { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); Statement* copyOfOriginalStmt = compiler->gtCloneStmt(stmt); @@ -469,23 +483,33 @@ class IndirectCallTransformer assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); - const bool isChainedGdv = (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0; - - if (isChainedGdv) + // TODO: implement chaining for multiple GDV candidates + const bool canChainGdv = GetChecksCount() == 1; + if (canChainGdv) { - JITDUMP("Expansion will chain to the previous GDV\n"); - } + const bool isChainedGdv = (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0; - Transform(); + if (isChainedGdv) + { + JITDUMP("Expansion will chain to the previous GDV\n"); + } + + Transform(); - if (isChainedGdv) + if (isChainedGdv) + { + TransformForChainedGdv(); + } + + // Look ahead and see if there's another Gdv we might chain to this one. + // + ScoutForChainedGdv(); + } + else { - TransformForChainedGdv(); + JITDUMP("Expansion will not chain to the previous GDV due to multiple type checks\n"); + Transform(); } - - // Look ahead and see if there's another Gdv we might chain to this one. - // - ScoutForChainedGdv(); } protected: @@ -518,15 +542,54 @@ class IndirectCallTransformer origCall->ClearGuardedDevirtualizationCandidate(); } + virtual UINT8 GetChecksCount() + { + return origCall->GetInlineCandidatesCount(); + } + + virtual void ChainFlow() + { + assert(compiler->fgPredsComputed); + + // currBlock + compiler->fgRemoveRefPred(remainderBlock, currBlock); + + // The rest of chaining is done in-place. + } + + virtual void SetWeights() + { + // remainderBlock has the same weight as the original block. + remainderBlock->inheritWeight(currBlock); + + // The rest of the weights are assigned in-place. + } + //------------------------------------------------------------------------ // CreateCheck: create check block and check method table // - virtual void CreateCheck() + virtual void CreateCheck(uint8_t checkIdx) { - // There's no need for a new block here. We can just append to currBlock. - // - checkBlock = currBlock; - checkBlock->bbJumpKind = BBJ_COND; + if (checkIdx == 0) + { + // There's no need for a new block here. We can just append to currBlock. + // + checkBlock = currBlock; + checkBlock->bbJumpKind = BBJ_COND; + } + else + { + // In case of multiple checks, append to the previous thenBlock block + BasicBlock* prevCheckBlock = checkBlock; + checkBlock = CreateAndInsertBasicBlock(BBJ_COND, thenBlock); + + // prevCheckBlock is expected to jump to this new check (if its type check doesn't succeed) + prevCheckBlock->bbJumpDest = checkBlock; + compiler->fgAddRefPred(checkBlock, prevCheckBlock); + + // Weight for the new secondary check is the difference between the previous check and the thenBlock. + checkBlock->setBBProfileWeight(prevCheckBlock->bbWeight - thenBlock->bbWeight); + } // Find last arg with a side effect. All args with any effect // before that will need to be spilled. @@ -574,7 +637,7 @@ class IndirectCallTransformer // lastStmt = checkBlock->lastStmt(); - InlineCandidateInfo* guardedInfo = origCall->GetGDVCandidateInfo(0); + InlineCandidateInfo* guardedInfo = origCall->GetGDVCandidateInfo(checkIdx); // Create comparison. On success we will jump to do the indirect call. GenTree* compare; @@ -892,12 +955,19 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateThen: create then block with direct call to method // - virtual void CreateThen() + virtual void CreateThen(uint8_t checkIdx) { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + thenBlock->bbJumpDest = remainderBlock; + thenBlock->inheritWeightPercentage(currBlock, origCall->GetGDVCandidateInfo(checkIdx)->likelihood); + + // thenBlock always jumps to remainderBlock. Also, it has a single pred - last checkBlock + thenBlock->bbJumpDest = remainderBlock; + compiler->fgAddRefPred(thenBlock, checkBlock); + compiler->fgAddRefPred(remainderBlock, thenBlock); - DevirtualizeCall(thenBlock, 0); + DevirtualizeCall(thenBlock, checkIdx); } //------------------------------------------------------------------------ @@ -908,6 +978,12 @@ class IndirectCallTransformer elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + // elseBlock always flows into remainderBlock + checkBlock->bbJumpDest = elseBlock; + compiler->fgAddRefPred(remainderBlock, elseBlock); + compiler->fgAddRefPred(elseBlock, checkBlock); + elseBlock->setBBProfileWeight(checkBlock->bbWeight - thenBlock->bbWeight); + GenTreeCall* call = origCall; Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 9de025bfc9042..57f8fe1a7d1ce 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -749,6 +749,7 @@ void InlineResult::Report() if (IsFailure() && (m_Call != nullptr)) { // compiler should have revoked candidacy on the call by now + // Unless it's a call has both failed and successful candidates (GDV candidates) assert(((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0) || m_Call->IsGuardedDevirtualizationCandidate()); if (m_Call->gtInlineObservation == InlineObservation::CALLEE_UNUSED_INITIAL) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index fe12686ab6396..1ca8959540d45 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -525,6 +525,11 @@ CONFIG_INTEGER(JitEnableRemoveEmptyTry, W("JitEnableRemoveEmptyTry"), 1) // Overall master enable for Guarded Devirtualization. CONFIG_INTEGER(JitEnableGuardedDevirtualization, W("JitEnableGuardedDevirtualization"), 1) +#define MAX_GDV_TYPE_CHECKS 5 +// Number of types to probe for polymorphic virtual call-sites to devirtualize them, +// Max number is MAX_GDV_TYPE_CHECKS defined above ^ +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 5) + // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75 CONFIG_INTEGER(JitGuardedDevirtualizationChainStatements, W("JitGuardedDevirtualizationChainStatements"), 4) From e404f4fde3158b10bc4c0cea16edd33b1290d207 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 May 2023 17:22:00 +0200 Subject: [PATCH 07/17] Fix weights --- src/coreclr/jit/indirectcalltransformer.cpp | 22 +++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 26592be37bdb2..3dc25d74886a0 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -588,7 +588,15 @@ class IndirectCallTransformer compiler->fgAddRefPred(checkBlock, prevCheckBlock); // Weight for the new secondary check is the difference between the previous check and the thenBlock. - checkBlock->setBBProfileWeight(prevCheckBlock->bbWeight - thenBlock->bbWeight); + weight_t newWeight = prevCheckBlock->bbWeight - thenBlock->bbWeight; + if (newWeight < 0) + { + // There could be a small error leading to e.g. -0.00001 instead of 0.0 here + // Mostly because of inheritWeightPercentage for thenBlock; + assert(abs(newWeight) < 0.01); + newWeight = 0; + } + checkBlock->setBBProfileWeight(newWeight); } // Find last arg with a side effect. All args with any effect @@ -982,7 +990,17 @@ class IndirectCallTransformer checkBlock->bbJumpDest = elseBlock; compiler->fgAddRefPred(remainderBlock, elseBlock); compiler->fgAddRefPred(elseBlock, checkBlock); - elseBlock->setBBProfileWeight(checkBlock->bbWeight - thenBlock->bbWeight); + + weight_t newWeight = checkBlock->bbWeight - thenBlock->bbWeight; + if (newWeight < 0) + { + // There could be a small error leading to e.g. -0.00001 instead of 0.0 here + // Mostly because of inheritWeightPercentage for thenBlock; + assert(abs(newWeight) < 0.01); + newWeight = 0; + } + + elseBlock->setBBProfileWeight(newWeight); GenTreeCall* call = origCall; Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); From 0ebf63d1eb2659333188476b0d2fde77ab1128e9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 May 2023 17:36:31 +0200 Subject: [PATCH 08/17] fix compilation --- src/coreclr/jit/indirectcalltransformer.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 3dc25d74886a0..f53c017ac66cc 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -593,7 +593,6 @@ class IndirectCallTransformer { // There could be a small error leading to e.g. -0.00001 instead of 0.0 here // Mostly because of inheritWeightPercentage for thenBlock; - assert(abs(newWeight) < 0.01); newWeight = 0; } checkBlock->setBBProfileWeight(newWeight); @@ -996,7 +995,6 @@ class IndirectCallTransformer { // There could be a small error leading to e.g. -0.00001 instead of 0.0 here // Mostly because of inheritWeightPercentage for thenBlock; - assert(abs(newWeight) < 0.01); newWeight = 0; } From f2e77fd951b7a48e045eff9740c70262646dba26 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 25 May 2023 14:06:48 +0200 Subject: [PATCH 09/17] Update src/coreclr/jit/indirectcalltransformer.cpp Co-authored-by: Andy Ayers --- src/coreclr/jit/indirectcalltransformer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index c5898abb81c83..06faedaa4880d 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -588,7 +588,7 @@ class IndirectCallTransformer compiler->fgAddRefPred(checkBlock, prevCheckBlock); // Weight for the new secondary check is the difference between the previous check and the thenBlock. - weight_t newWeight = prevCheckBlock->bbWeight - thenBlock->bbWeight; + weight_t newWeight = inheritWeightPercentage(prevCheckBlock, 100 - origCall->GetGDVCandidateInfo(checkIdx)->likelihood); if (newWeight < 0) { // There could be a small error leading to e.g. -0.00001 instead of 0.0 here From 4297165df403f2ba40f46406c4fd43040f6bb83d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 25 May 2023 14:49:25 +0200 Subject: [PATCH 10/17] Address feedback --- src/coreclr/jit/gentree.cpp | 6 +++--- src/coreclr/jit/gentree.h | 4 ++-- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/indirectcalltransformer.cpp | 12 +++--------- 4 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b2650440eb2ae..0eff332a1e710 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2164,12 +2164,12 @@ GenTree* Compiler::getArrayLengthFromAllocation(GenTree* tree DEBUGARG(BasicBloc } //------------------------------------------------------------------------- -// SetSingleInlineCadidateInfo: set a single inline candidate info in the current call. +// SetSingleInlineCandidateInfo: set a single inline candidate info in the current call. // // Arguments: // candidateInfo - inline candidate info // -void GenTreeCall::SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo) +void GenTreeCall::SetSingleInlineCandidateInfo(InlineCandidateInfo* candidateInfo) { if (candidateInfo != nullptr) { @@ -9352,8 +9352,8 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree, { copy->gtCallMethHnd = tree->gtCallMethHnd; copy->gtInlineCandidateInfo = tree->gtInlineCandidateInfo; + copy->gtInlineInfoCount = tree->gtInlineInfoCount; } - copy->gtInlineInfoCount = tree->gtInlineInfoCount; copy->gtCallType = tree->gtCallType; copy->gtReturnType = tree->gtReturnType; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b665ccd0a4087..1cdc9309a3a06 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5430,7 +5430,7 @@ struct GenTreeCall final : public GenTree return gtInlineCandidateInfo; } - void SetSingleInlineCadidateInfo(InlineCandidateInfo* candidateInfo); + void SetSingleInlineCandidateInfo(InlineCandidateInfo* candidateInfo); InlineCandidateInfo* GetGDVCandidateInfo(uint8_t index); @@ -5438,7 +5438,7 @@ struct GenTreeCall final : public GenTree void ClearInlineInfo() { - SetSingleInlineCadidateInfo(nullptr); + SetSingleInlineCandidateInfo(nullptr); } uint8_t GetInlineCandidatesCount() diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index dcfc236f1e861..cbf8db5edfb58 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6523,7 +6523,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, else { assert(candidateIndex == 0); - call->SetSingleInlineCadidateInfo(inlineCandidateInfo); + call->SetSingleInlineCandidateInfo(inlineCandidateInfo); } // Let the strategy know there's another candidate. diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 06faedaa4880d..f7a0bfaf99453 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -588,14 +588,8 @@ class IndirectCallTransformer compiler->fgAddRefPred(checkBlock, prevCheckBlock); // Weight for the new secondary check is the difference between the previous check and the thenBlock. - weight_t newWeight = inheritWeightPercentage(prevCheckBlock, 100 - origCall->GetGDVCandidateInfo(checkIdx)->likelihood); - if (newWeight < 0) - { - // There could be a small error leading to e.g. -0.00001 instead of 0.0 here - // Mostly because of inheritWeightPercentage for thenBlock; - newWeight = 0; - } - checkBlock->setBBProfileWeight(newWeight); + checkBlock->inheritWeightPercentage(prevCheckBlock, + 100 - origCall->GetGDVCandidateInfo(checkIdx)->likelihood); } // Find last arg with a side effect. All args with any effect @@ -932,7 +926,7 @@ class IndirectCallTransformer inlineInfo->clsHandle = compiler->info.compCompHnd->getMethodClass(methodHnd); inlineInfo->exactContextHnd = context; inlineInfo->preexistingSpillTemp = returnTemp; - call->SetSingleInlineCadidateInfo(inlineInfo); + call->SetSingleInlineCandidateInfo(inlineInfo); // If there was a ret expr for this call, we need to create a new one // and append it just after the call. From c4685e1289d316ac23f4583916de5a219facbffb Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 25 May 2023 14:50:54 +0200 Subject: [PATCH 11/17] Update src/coreclr/jit/gentree.h Co-authored-by: Andy Ayers --- src/coreclr/jit/gentree.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 1cdc9309a3a06..0e6bf79e7fc4c 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5423,9 +5423,9 @@ struct GenTreeCall final : public GenTree InlineCandidateInfo* GetSingleInlineCandidateInfo() { - if (gtInlineInfoCount > 1) + if (gtInlineInfoCount != 1) { - assert(!"Call has multiple inline candidates - use GetGDVCandidateInfo instead"); + assert(!"Call has no or multiple inline candidates"); } return gtInlineCandidateInfo; } From f46ced36e5662bc499cdcfb9746ce73550270f40 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 25 May 2023 14:54:06 +0200 Subject: [PATCH 12/17] Fix comment --- src/coreclr/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0eff332a1e710..907c26f1e7ae4 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2199,7 +2199,7 @@ InlineCandidateInfo* GenTreeCall::GetGDVCandidateInfo(uint8_t index) //------------------------------------------------------------------------- // AddGDVCandidateInfo: Record a guarded devirtualization (GDV) candidate info -// for this call. For now, we only support one GDV candidate per call. +// for this call. A call can't have more than MAX_GDV_TYPE_CHECKS number of candidates // // Arguments: // comp - Compiler instance From 780f60813c1521411220c2f0d38ccc3ac8c2256c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 25 May 2023 14:59:29 +0200 Subject: [PATCH 13/17] Add assert --- src/coreclr/jit/importercalls.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index cbf8db5edfb58..1bad0254e2133 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5906,9 +5906,10 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, assert(numExactClasses <= maxTypeChecks); JITDUMP("We have exactly %d classes implementing %s:\n", numExactClasses, eeGetClassName(baseClass)); - // NOTE: The case where we have only a single implementation is handled naturally - // through gtGetClassHandle without help of GDV. - // + // While it's legal to have only one exact class here, we expect that such cases are handled + // naturally without help of GDV. + assert(numExactClasses > 1); + int skipped = 0; for (int exactClsIdx = 0; exactClsIdx < numExactClasses; exactClsIdx++) { From b2b2d483d812d3529237ab18ff86995dd9ebae36 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 25 May 2023 15:35:14 +0200 Subject: [PATCH 14/17] Fix GetSingleInlineCandidateInfo() --- src/coreclr/jit/gentree.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0e6bf79e7fc4c..e6d592fc37af3 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -5423,9 +5423,16 @@ struct GenTreeCall final : public GenTree InlineCandidateInfo* GetSingleInlineCandidateInfo() { - if (gtInlineInfoCount != 1) + // gtInlineInfoCount can be 0 (not an inline candidate) or 1 + if (gtInlineInfoCount == 0) { - assert(!"Call has no or multiple inline candidates"); + assert(!IsInlineCandidate()); + assert(gtInlineCandidateInfo == nullptr); + return nullptr; + } + else if (gtInlineInfoCount > 1) + { + assert(!"Call has multiple inline candidates"); } return gtInlineCandidateInfo; } From 0939cd97b7902423ad880d8ff1ba936e0b2710e2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 25 May 2023 16:31:22 +0200 Subject: [PATCH 15/17] Fix asserts --- src/coreclr/jit/importercalls.cpp | 6 +----- src/coreclr/jit/indirectcalltransformer.cpp | 14 ++++++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 1bad0254e2133..c064095a520f3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5903,13 +5903,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, } else { - assert(numExactClasses <= maxTypeChecks); + assert((numExactClasses > 0) && (numExactClasses <= maxTypeChecks)); JITDUMP("We have exactly %d classes implementing %s:\n", numExactClasses, eeGetClassName(baseClass)); - // While it's legal to have only one exact class here, we expect that such cases are handled - // naturally without help of GDV. - assert(numExactClasses > 1); - int skipped = 0; for (int exactClsIdx = 0; exactClsIdx < numExactClasses; exactClsIdx++) { diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index f7a0bfaf99453..15042ce6d744e 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -984,15 +984,17 @@ class IndirectCallTransformer compiler->fgAddRefPred(remainderBlock, elseBlock); compiler->fgAddRefPred(elseBlock, checkBlock); - weight_t newWeight = checkBlock->bbWeight - thenBlock->bbWeight; - if (newWeight < 0) + // Calculate the likelihood of the else block as a remainder of the sum + // of all the other likelihoods. + unsigned elseLikelihood = 100; + for (uint8_t i = 0; i < origCall->GetInlineCandidatesCount(); i++) { - // There could be a small error leading to e.g. -0.00001 instead of 0.0 here - // Mostly because of inheritWeightPercentage for thenBlock; - newWeight = 0; + elseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; } + // Make sure it didn't overflow + assert(elseLikelihood <= 100); - elseBlock->setBBProfileWeight(newWeight); + elseBlock->inheritWeightPercentage(currBlock, elseLikelihood); GenTreeCall* call = origCall; Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); From dd796758f5d8fed1d537a25d2532006d1966a9eb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 26 May 2023 00:38:28 +0200 Subject: [PATCH 16/17] Address feedback --- src/coreclr/jit/compiler.h | 5 +- src/coreclr/jit/importercalls.cpp | 139 ++++++++++++++++-------------- 2 files changed, 77 insertions(+), 67 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d5679398742a8..c363ad7d9c7ee 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4392,12 +4392,13 @@ class Compiler CORINFO_CALL_INFO* callInfo, IL_OFFSET ilOffset); - bool impMarkInlineCandidateHelper(GenTreeCall* call, + void impMarkInlineCandidateHelper(GenTreeCall* call, uint8_t candidateIndex, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo, - IL_OFFSET ilOffset); + IL_OFFSET ilOffset, + InlineResult* inlineResult); bool impTailCallRetTypeCompatible(bool allowWidening, var_types callerRetType, diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index c064095a520f3..b3705276ece92 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5942,7 +5942,14 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // NOTE: This is currently used only with NativeAOT. In theory, we could also check if we // have static PGO data to decide which class to guess first. Presumably, this is a rare case. // - const int likelyHood = 100 / numExactClasses; + int likelyHood = 100 / numExactClasses; + + // If numExactClasses is 3, then likelyHood is 33 and 33*3=99. + // Apply the error to the first guess, so we'll have [34,33,33] + if (exactClsIdx == 0) + { + likelyHood += 100 - likelyHood * numExactClasses; + } addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, likelyHood); @@ -6205,34 +6212,41 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsGuardedDevirtualizationCandidate()) { - const uint8_t candidatesCount = call->GetInlineCandidatesCount(); - assert(candidatesCount > 0); - for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) - { - // Do the actual evaluation - bool success = impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, - exactContextNeedsRuntimeLookup, callInfo, ilOffset); - if (!success) - { - if (candidatesCount > 1) - { - // TODO: we should not give up if one of the candidates fails to inline while others succeed. - // - JITDUMP( - "We had multiple inline candidates but have to give up on them since one of them didn't pass" - "inline checks") - call->ClearInlineInfo(); - call->ClearGuardedDevirtualizationCandidate(); - } - break; - } - } + //const uint8_t candidatesCount = call->GetInlineCandidatesCount(); + //assert(candidatesCount > 0); + //for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) + //{ + // InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); + + // // Do the actual evaluation + // impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, + // exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); + + // if (!inlineResult.IsSuccess()) + // { + // if (candidatesCount > 1) + // { + // // TODO: we should not give up if one of the candidates fails to inline while others succeed. + // // + // JITDUMP( + // "We had multiple inline candidates but have to give up on them since one of them didn't pass" + // "inline checks") + // call->ClearInlineInfo(); + // call->ClearGuardedDevirtualizationCandidate(); + // } + // break; + // } + //} + call->ClearInlineInfo(); + call->ClearGuardedDevirtualizationCandidate(); + return; } else { const uint8_t candidatesCount = call->GetInlineCandidatesCount(); assert(candidatesCount <= 1); - impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset); + InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); + impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); } // If this call is an inline candidate or is not a guarded devirtualization @@ -6276,15 +6290,14 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // method may be marked as "noinline" to short-circuit any // future assessments of calls to this method. // -// Return value: -// true if we can inline the given inline candidate, false otherwise. -bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, +void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, uint8_t candidateIndex, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo, - IL_OFFSET ilOffset) + IL_OFFSET ilOffset, + InlineResult* inlineResult) { // Let the strategy know there's another call impInlineRoot()->m_inlineStrategy->NoteCall(); @@ -6298,46 +6311,44 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, * figure out why we did not set MAXOPT for this compile. */ assert(!compIsForInlining()); - return false; + return; } - InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); - // Don't inline if not optimizing root method if (opts.compDbgCode) { - inlineResult.NoteFatal(InlineObservation::CALLER_DEBUG_CODEGEN); - return false; + inlineResult->NoteFatal(InlineObservation::CALLER_DEBUG_CODEGEN); + return; } // Don't inline if inlining into this method is disabled. if (impInlineRoot()->m_inlineStrategy->IsInliningDisabled()) { - inlineResult.NoteFatal(InlineObservation::CALLER_IS_JIT_NOINLINE); - return false; + inlineResult->NoteFatal(InlineObservation::CALLER_IS_JIT_NOINLINE); + return; } // Don't inline into callers that use the NextCallReturnAddress intrinsic. if (info.compHasNextCallRetAddr) { - inlineResult.NoteFatal(InlineObservation::CALLER_USES_NEXT_CALL_RET_ADDR); - return false; + inlineResult->NoteFatal(InlineObservation::CALLER_USES_NEXT_CALL_RET_ADDR); + return; } // Inlining candidate determination needs to honor only IL tail prefix. // Inlining takes precedence over implicit tail call optimization (if the call is not directly recursive). if (call->IsTailPrefixedCall()) { - inlineResult.NoteFatal(InlineObservation::CALLSITE_EXPLICIT_TAIL_PREFIX); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_EXPLICIT_TAIL_PREFIX); + return; } // Delegate Invoke method doesn't have a body and gets special cased instead. // Don't even bother trying to inline it. if (call->IsDelegateInvoke() && !call->IsGuardedDevirtualizationCandidate()) { - inlineResult.NoteFatal(InlineObservation::CALLEE_HAS_NO_BODY); - return false; + inlineResult->NoteFatal(InlineObservation::CALLEE_HAS_NO_BODY); + return; } // Tail recursion elimination takes precedence over inlining. @@ -6346,8 +6357,8 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, // as a fast tail call or turned into a loop. if (gtIsRecursiveCall(call) && call->IsImplicitTailCall()) { - inlineResult.NoteFatal(InlineObservation::CALLSITE_IMPLICIT_REC_TAIL_CALL); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IMPLICIT_REC_TAIL_CALL); + return; } if (call->IsVirtual()) @@ -6356,8 +6367,8 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, // but reject all other virtual calls. if (!call->IsGuardedDevirtualizationCandidate()) { - inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT); + return; } } @@ -6366,15 +6377,15 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->gtCallType == CT_HELPER) { assert(!call->IsGuardedDevirtualizationCandidate()); - inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_CALL_TO_HELPER); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_CALL_TO_HELPER); + return; } /* Ignore indirect calls */ if (call->gtCallType == CT_INDIRECT) { - inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT_MANAGED); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT_MANAGED); + return; } /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less @@ -6437,8 +6448,8 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, #endif - inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_CATCH); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_CATCH); + return; } if (bbInFilterILRange(compCurBB)) @@ -6450,8 +6461,8 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } #endif - inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_FILTER); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_FILTER); + return; } } @@ -6459,16 +6470,16 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (methAttr & CORINFO_FLG_DONT_INLINE) { - inlineResult.NoteFatal(InlineObservation::CALLEE_IS_NOINLINE); - return false; + inlineResult->NoteFatal(InlineObservation::CALLEE_IS_NOINLINE); + return; } /* Cannot inline synchronized methods */ if (methAttr & CORINFO_FLG_SYNCH) { - inlineResult.NoteFatal(InlineObservation::CALLEE_IS_SYNCHRONIZED); - return false; + inlineResult->NoteFatal(InlineObservation::CALLEE_IS_SYNCHRONIZED); + return; } /* Check legality of PInvoke callsite (for inlining of marshalling code) */ @@ -6479,17 +6490,17 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, BasicBlock* block = compIsForInlining() ? impInlineInfo->iciBlock : compCurBB; if (!impCanPInvokeInlineCallSite(block)) { - inlineResult.NoteFatal(InlineObservation::CALLSITE_PINVOKE_EH); - return false; + inlineResult->NoteFatal(InlineObservation::CALLSITE_PINVOKE_EH); + return; } } InlineCandidateInfo* inlineCandidateInfo = nullptr; - impCheckCanInline(call, candidateIndex, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, &inlineResult); + impCheckCanInline(call, candidateIndex, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, inlineResult); - if (inlineResult.IsFailure()) + if (inlineResult->IsFailure()) { - return false; + return; } // The old value should be null OR this call should be a guarded devirtualization candidate. @@ -6528,9 +6539,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, // Since we're not actually inlining yet, and this call site is // still just an inline candidate, there's nothing to report. - inlineResult.SetSuccessResult(INLINE_CHECK_CAN_INLINE_SUCCESS); - - return true; + inlineResult->SetSuccessResult(INLINE_CHECK_CAN_INLINE_SUCCESS); } /******************************************************************************/ From dbf28407571f262f85a77f9e8535d4ba08d5815e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 26 May 2023 11:10:51 +0200 Subject: [PATCH 17/17] Address Andy's feedback --- src/coreclr/jit/importercalls.cpp | 63 +++++++++++++++---------------- src/coreclr/jit/jitconfigvalues.h | 2 +- 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b3705276ece92..819316a761cf4 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5886,7 +5886,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // where we know the exact number of classes implementing the given base in compile-time. // For now, let's only do this when we don't have any PGO data. In future, we should be able to benefit // from both. - if (!hasPgoData && (baseClass != NO_CLASS_HANDLE)) + if (!hasPgoData && (baseClass != NO_CLASS_HANDLE) && JitConfig.JitEnableExactDevirtualization()) { int maxTypeChecks = min(JitConfig.JitGuardedDevirtualizationMaxTypeChecks(), MAX_GDV_TYPE_CHECKS); @@ -6212,41 +6212,39 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsGuardedDevirtualizationCandidate()) { - //const uint8_t candidatesCount = call->GetInlineCandidatesCount(); - //assert(candidatesCount > 0); - //for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) - //{ - // InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); - - // // Do the actual evaluation - // impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, - // exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); - - // if (!inlineResult.IsSuccess()) - // { - // if (candidatesCount > 1) - // { - // // TODO: we should not give up if one of the candidates fails to inline while others succeed. - // // - // JITDUMP( - // "We had multiple inline candidates but have to give up on them since one of them didn't pass" - // "inline checks") - // call->ClearInlineInfo(); - // call->ClearGuardedDevirtualizationCandidate(); - // } - // break; - // } - //} - call->ClearInlineInfo(); - call->ClearGuardedDevirtualizationCandidate(); - return; + const uint8_t candidatesCount = call->GetInlineCandidatesCount(); + assert(candidatesCount > 0); + for (uint8_t candidateId = 0; candidateId < candidatesCount; candidateId++) + { + InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); + + // Do the actual evaluation + impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, + ilOffset, &inlineResult); + + if (!inlineResult.IsCandidate()) + { + if (candidatesCount > 1) + { + // TODO: we should not give up if one of the candidates fails to inline while others succeed. + // + JITDUMP( + "We had multiple inline candidates but have to give up on them since one of them didn't pass" + "inline checks") + call->ClearInlineInfo(); + call->ClearGuardedDevirtualizationCandidate(); + } + break; + } + } } else { const uint8_t candidatesCount = call->GetInlineCandidatesCount(); assert(candidatesCount <= 1); - InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); - impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); + InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); + impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, + &inlineResult); } // If this call is an inline candidate or is not a guarded devirtualization @@ -6524,8 +6522,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - assert(call->GetGDVCandidateInfo(candidateIndex) != nullptr); - *call->GetGDVCandidateInfo(candidateIndex) = *inlineCandidateInfo; + assert(call->GetGDVCandidateInfo(candidateIndex) == inlineCandidateInfo); call->gtFlags |= GTF_CALL_INLINE_CANDIDATE; } else diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 1ca8959540d45..6ab73d6abde37 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -528,7 +528,7 @@ CONFIG_INTEGER(JitEnableGuardedDevirtualization, W("JitEnableGuardedDevirtualiza #define MAX_GDV_TYPE_CHECKS 5 // Number of types to probe for polymorphic virtual call-sites to devirtualize them, // Max number is MAX_GDV_TYPE_CHECKS defined above ^ -CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 5) +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 3) // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75