From a096415c795c8ff857e40d942da8fb75254ff3fd Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 2 Jun 2023 14:40:50 +0200 Subject: [PATCH 1/6] Omit GDV fallback on NativeAOT --- src/coreclr/jit/gentree.cpp | 4 +++ src/coreclr/jit/gentree.h | 3 +- src/coreclr/jit/importercalls.cpp | 9 ++++++ src/coreclr/jit/indirectcalltransformer.cpp | 31 +++++++++++++++++++-- src/coreclr/jit/jitconfigvalues.h | 2 +- 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index c3a592da0f1329..cb107d748bb94a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2212,6 +2212,7 @@ InlineCandidateInfo* GenTreeCall::GetGDVCandidateInfo(uint8_t index) // void GenTreeCall::AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candidateInfo) { + assert((gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT) == 0); assert(gtInlineInfoCount < MAX_GDV_TYPE_CHECKS); assert(candidateInfo != nullptr); @@ -2248,6 +2249,9 @@ void GenTreeCall::AddGDVCandidateInfo(Compiler* comp, InlineCandidateInfo* candi // void GenTreeCall::RemoveGDVCandidateInfo(Compiler* comp, uint8_t index) { + // We change the number of candidates so it's no longer "doesn't need a fallback" + gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT_EXACT; + assert(index < gtInlineInfoCount); if (gtInlineInfoCount == 1) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index e5f3e11f426792..bb3ce3064a9a34 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4045,6 +4045,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 without a fallback 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 @@ -5367,7 +5368,7 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { - gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; + gtCallMoreFlags &= ~(GTF_CALL_M_GUARDED_DEVIRT | GTF_CALL_M_GUARDED_DEVIRT_EXACT); } void SetIsGuarded() diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 4afb8c55535495..0b93a7d3061289 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5988,6 +5988,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (!info.compCompHnd->resolveVirtualMethod(&dvInfo)) { + skipped++; JITDUMP("Can't figure out which method would be invoked, sorry\n"); // Maybe other candidates will be resolved. // Although, we no longer can remove the fallback (we never do it currently anyway) @@ -6012,6 +6013,14 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, likelyHood); } + + if (skipped == 0) + { + assert((numExactClasses > 0) && (call->GetInlineCandidatesCount() == numExactClasses)); + call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; + // NOTE: we have to drop this flag if we change the number of candidates before we expand. + } + return; } } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index bb2f316bb327d6..8b0318b935c82b 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -484,7 +484,8 @@ class IndirectCallTransformer JITDUMP("Likelihood of correct guess is %u\n", likelihood); // TODO: implement chaining for multiple GDV candidates - const bool canChainGdv = GetChecksCount() == 1; + const bool canChainGdv = + (GetChecksCount() == 1) && ((origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT) == 0); if (canChainGdv) { const bool isChainedGdv = (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0; @@ -644,6 +645,17 @@ class IndirectCallTransformer // lastStmt = checkBlock->lastStmt(); + // In case if GDV candidates are "exact" (e.g. we have the full list of classes implementing + // the given interface in the app - NativeAOT only at this moment) we assume the last + // check will always be true, so we just simplify the block to BBJ_NONE + const bool isLastCheck = (checkIdx == origCall->GetInlineCandidatesCount() - 1); + if (isLastCheck && ((origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT) != 0)) + { + checkBlock->bbJumpDest = nullptr; + checkBlock->bbJumpKind = BBJ_NONE; + return; + } + InlineCandidateInfo* guardedInfo = origCall->GetGDVCandidateInfo(checkIdx); // Create comparison. On success we will jump to do the indirect call. @@ -986,10 +998,23 @@ class IndirectCallTransformer elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + // CheckBlock flows into elseBlock unless we deal with the case + // where we know the last check is always true (in case of "exact" GDV) + if (checkBlock->KindIs(BBJ_COND)) + { + checkBlock->bbJumpDest = elseBlock; + compiler->fgAddRefPred(elseBlock, checkBlock); + } + else + { + // In theory, we could simplify the IR here, but since it's a rare case + // and is NativeAOT-only, we just assume the unreached block will be removed + // by other phases. + assert(origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT); + } + // elseBlock always flows into remainderBlock - checkBlock->bbJumpDest = elseBlock; compiler->fgAddRefPred(remainderBlock, elseBlock); - compiler->fgAddRefPred(elseBlock, checkBlock); // Calculate the likelihood of the else block as a remainder of the sum // of all the other likelihoods. diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 8657940e5a00c5..354e3a8f23ff84 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"), 1) +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 3) // CI: test // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75 From d3d3a7edc81faec80c8653ebb9797ee92d7b9567 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 2 Jun 2023 15:26:19 +0200 Subject: [PATCH 2/6] Unrelated fix for getLikelyClassesOrMethods --- src/coreclr/jit/likelyclass.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/likelyclass.cpp b/src/coreclr/jit/likelyclass.cpp index 86f4784b7e6a81..c03d4c8be59b0b 100644 --- a/src/coreclr/jit/likelyclass.cpp +++ b/src/coreclr/jit/likelyclass.cpp @@ -230,7 +230,8 @@ static unsigned getLikelyClassesOrMethods(LikelyClassMethodRecord* LikelyClassMethodHistogramEntry sortedEntries[HISTOGRAM_MAX_SIZE_COUNT]; // Since this method can be invoked without a jit instance we can't use any existing allocators - unsigned knownHandles = 0; + unsigned knownHandles = 0; + unsigned containsUnknownHandles = false; for (unsigned m = 0; m < h.countHistogramElements; m++) { LikelyClassMethodHistogramEntry const hist = h.HistogramEntryAt(m); @@ -238,6 +239,10 @@ static unsigned getLikelyClassesOrMethods(LikelyClassMethodRecord* { sortedEntries[knownHandles++] = hist; } + else + { + containsUnknownHandles = true; + } } if (knownHandles == 0) @@ -268,7 +273,7 @@ static unsigned getLikelyClassesOrMethods(LikelyClassMethodRecord* // Distribute the rounding error and just apply it to the first entry. // Assume that there is no error If we have unknown handles. - if (numberOfClasses == h.m_totalCount) + if (!containsUnknownHandles) { assert(numberOfClasses > 0); assert(totalLikelihood > 0); From 19aed83882b8ac576529a224558a208ac579fb6f Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 2 Jun 2023 20:02:37 +0200 Subject: [PATCH 3/6] Update importercalls.cpp --- src/coreclr/jit/importercalls.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 0b93a7d3061289..5036858f74533f 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5962,7 +5962,6 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, assert((numExactClasses > 0) && (numExactClasses <= maxTypeChecks)); JITDUMP("We have exactly %d classes implementing %s:\n", numExactClasses, eeGetClassName(baseClass)); - int skipped = 0; for (int exactClsIdx = 0; exactClsIdx < numExactClasses; exactClsIdx++) { CORINFO_CLASS_HANDLE exactCls = exactClasses[exactClsIdx]; @@ -5988,7 +5987,6 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (!info.compCompHnd->resolveVirtualMethod(&dvInfo)) { - skipped++; JITDUMP("Can't figure out which method would be invoked, sorry\n"); // Maybe other candidates will be resolved. // Although, we no longer can remove the fallback (we never do it currently anyway) @@ -6014,9 +6012,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, likelyHood); } - if (skipped == 0) + if (call->GetInlineCandidatesCount() == numExactClasses) { - assert((numExactClasses > 0) && (call->GetInlineCandidatesCount() == numExactClasses)); + assert(numExactClasses > 0); call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; // NOTE: we have to drop this flag if we change the number of candidates before we expand. } From 6a3aea05defbe9c27160ba9a82b0a68e06956885 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 4 Jun 2023 02:30:59 +0200 Subject: [PATCH 4/6] Update importercalls.cpp --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 5036858f74533f..6b6af68c326deb 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6015,7 +6015,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (call->GetInlineCandidatesCount() == numExactClasses) { assert(numExactClasses > 0); - call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; + // call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; // NOTE: we have to drop this flag if we change the number of candidates before we expand. } From 84c046ca9044eb6ebda164cbd430eac71db540eb Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 11 Jun 2023 01:12:04 +0200 Subject: [PATCH 5/6] Update importercalls.cpp --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ed059d8767d5a8..c262320e228f43 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6015,7 +6015,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (call->GetInlineCandidatesCount() == numExactClasses) { assert(numExactClasses > 0); - // call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; + call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_EXACT; // NOTE: we have to drop this flag if we change the number of candidates before we expand. } From 32d2c03208b37a6a29f30bbac429d1c6065a2cd1 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 11 Jun 2023 11:34:11 +0200 Subject: [PATCH 6/6] Update jitconfigvalues.h --- src/coreclr/jit/jitconfigvalues.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 354e3a8f23ff84..8657940e5a00c5 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"), 3) // CI: test +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 1) // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75