From aff9029cdd746f3179293e1b83b11cdcc638be32 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 26 May 2023 20:52:28 +0200 Subject: [PATCH 1/9] Implement multiple GDV for JIT --- src/coreclr/jit/compiler.h | 7 +- src/coreclr/jit/importercalls.cpp | 259 ++++++++++++++++++------------ 2 files changed, 158 insertions(+), 108 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index c363ad7d9c7eed..bcb3fb9ad144c4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7128,9 +7128,10 @@ class Compiler void pickGDV(GenTreeCall* call, IL_OFFSET ilOffset, bool isInterface, - CORINFO_CLASS_HANDLE* classGuess, - CORINFO_METHOD_HANDLE* methodGuess, - unsigned* likelihood); + CORINFO_CLASS_HANDLE* classGuesses, + CORINFO_METHOD_HANDLE* methodGuesses, + int* candidatesCount, + unsigned* likelihoods); void considerGuardedDevirtualization(GenTreeCall* call, IL_OFFSET ilOffset, diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 819316a761cf45..b49c1dc888054d 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5594,15 +5594,14 @@ void Compiler::addFatPointerCandidate(GenTreeCall* call) void Compiler::pickGDV(GenTreeCall* call, IL_OFFSET ilOffset, bool isInterface, - CORINFO_CLASS_HANDLE* classGuess, - CORINFO_METHOD_HANDLE* methodGuess, - unsigned* likelihood) + CORINFO_CLASS_HANDLE* classGuesses, + CORINFO_METHOD_HANDLE* methodGuesses, + int* candidatesCount, + unsigned* likelihoods) { - *classGuess = NO_CLASS_HANDLE; - *methodGuess = NO_METHOD_HANDLE; - *likelihood = 0; + *candidatesCount = 0; - const int maxLikelyClasses = 32; + const int maxLikelyClasses = MAX_GDV_TYPE_CHECKS; LikelyClassMethodRecord likelyClasses[maxLikelyClasses]; unsigned numberOfClasses = 0; if (call->IsVirtualStub() || call->IsVirtualVtable()) @@ -5611,7 +5610,7 @@ void Compiler::pickGDV(GenTreeCall* call, getLikelyClasses(likelyClasses, maxLikelyClasses, fgPgoSchema, fgPgoSchemaCount, fgPgoData, ilOffset); } - const int maxLikelyMethods = 32; + const int maxLikelyMethods = MAX_GDV_TYPE_CHECKS; LikelyClassMethodRecord likelyMethods[maxLikelyMethods]; unsigned numberOfMethods = 0; @@ -5702,16 +5701,20 @@ void Compiler::pickGDV(GenTreeCall* call, unsigned index = static_cast(random->Next(static_cast(numberOfClasses + numberOfMethods))); if (index < numberOfClasses) { - *classGuess = (CORINFO_CLASS_HANDLE)likelyClasses[index].handle; - *likelihood = 100; - JITDUMP("Picked random class for GDV: %p (%s)\n", *classGuess, eeGetClassName(*classGuess)); + classGuesses[0] = (CORINFO_CLASS_HANDLE)likelyClasses[index].handle; + likelihoods[0] = 100; + *candidatesCount = 1; + // TODO: report multiple candidates + JITDUMP("Picked random class for GDV: %p (%s)\n", classGuesses[0], eeGetClassName(classGuesses[0])); return; } else { - *methodGuess = (CORINFO_METHOD_HANDLE)likelyMethods[index - numberOfClasses].handle; - *likelihood = 100; - JITDUMP("Picked random method for GDV: %p (%s)\n", *methodGuess, eeGetMethodFullName(*methodGuess)); + methodGuesses[0] = (CORINFO_METHOD_HANDLE)likelyMethods[index - numberOfClasses].handle; + likelihoods[0] = 100; + *candidatesCount = 1; + // TODO: report multiple candidates + JITDUMP("Picked random method for GDV: %p (%s)\n", methodGuesses[0], eeGetMethodFullName(methodGuesses[0])); return; } } @@ -5720,25 +5723,62 @@ void Compiler::pickGDV(GenTreeCall* call, // Prefer class guess as it is cheaper if (numberOfClasses > 0) { - unsigned likelihoodThreshold = isInterface ? 25 : 30; - if (likelyClasses[0].likelihood >= likelihoodThreshold) + const int maxNumberOfGuesses = min(MAX_GDV_TYPE_CHECKS, JitConfig.JitGuardedDevirtualizationMaxTypeChecks()); + if (maxNumberOfGuesses == 0) { - *classGuess = (CORINFO_CLASS_HANDLE)likelyClasses[0].handle; - *likelihood = likelyClasses[0].likelihood; + // DOTNET_JitGuardedDevirtualizationMaxTypeChecks=0 means we don't want to do any guarded devirtualization + // Although, we expect users to disable GDV by setting DOTNET_JitEnableGuardedDevirtualization=0 return; } - JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", - isInterface ? "interface" : "virtual", likelihoodThreshold); + assert((maxNumberOfGuesses > 0) && (maxNumberOfGuesses <= MAX_GDV_TYPE_CHECKS)); + + unsigned likelihoodThreshold; + if (maxNumberOfGuesses == 1) + { + // We're allowed to make only a single guess - it means we want to work only with dominating types + likelihoodThreshold = isInterface ? 25 : 30; + } + else if (maxNumberOfGuesses == 2) + { + // Two guesses - slightly relax the thresholds + likelihoodThreshold = isInterface ? 12 : 15; + } + else + { + // We're allowed to make more than 2 guesses - pick all types with likelihood >= 10% + likelihoodThreshold = 10; + } + + assert(*candidatesCount == 0); + for (unsigned guessIdx = 0; guessIdx < min((unsigned)maxNumberOfGuesses, numberOfClasses); guessIdx++) + { + if (likelyClasses[guessIdx].likelihood >= likelihoodThreshold) + { + classGuesses[guessIdx] = (CORINFO_CLASS_HANDLE)likelyClasses[guessIdx].handle; + likelihoods[guessIdx] = likelyClasses[guessIdx].likelihood; + *candidatesCount = *candidatesCount + 1; + JITDUMP("Accepting type %s with likelihood %u as a candidate\n", eeGetClassName(classGuesses[guessIdx]), + likelihoods[guessIdx]) + } + else + { + // The candidates are sorted by likelihood so the rest of the + // guesses will have even lower likelihoods + break; + } + } } if (numberOfMethods > 0) { + // For method guessing we only support a single target for now unsigned likelihoodThreshold = 30; if (likelyMethods[0].likelihood >= likelihoodThreshold) { - *methodGuess = (CORINFO_METHOD_HANDLE)likelyMethods[0].handle; - *likelihood = likelyMethods[0].likelihood; + methodGuesses[0] = (CORINFO_METHOD_HANDLE)likelyMethods[0].handle; + likelihoods[0] = likelyMethods[0].likelihood; + *candidatesCount = 1; return; } @@ -5862,9 +5902,10 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, bool hasPgoData = true; - CORINFO_CLASS_HANDLE likelyClass = NO_CLASS_HANDLE; - CORINFO_METHOD_HANDLE likelyMethod = NO_METHOD_HANDLE; - unsigned likelihood = 0; + CORINFO_CLASS_HANDLE likelyClasses[MAX_GDV_TYPE_CHECKS] = {}; + CORINFO_METHOD_HANDLE likelyMethodes[MAX_GDV_TYPE_CHECKS] = {}; + unsigned likelihoods[MAX_GDV_TYPE_CHECKS] = {}; + int candidatesCount = 0; // We currently only get likely class guesses when there is PGO data // with class profiles. @@ -5875,8 +5916,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, } else { - pickGDV(call, ilOffset, isInterface, &likelyClass, &likelyMethod, &likelihood); - if ((likelyClass == NO_CLASS_HANDLE) && (likelyMethod == NO_METHOD_HANDLE)) + pickGDV(call, ilOffset, isInterface, likelyClasses, likelyMethodes, &candidatesCount, likelihoods); + if (candidatesCount == 0) { hasPgoData = false; } @@ -5964,103 +6005,111 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, return; } - uint32_t likelyClassAttribs = 0; - if (likelyClass != NO_CLASS_HANDLE) + for (int candidateId = 0; candidateId < candidatesCount; candidateId++) { - likelyClassAttribs = info.compCompHnd->getClassAttribs(likelyClass); + CORINFO_CLASS_HANDLE likelyClass = likelyClasses[candidateId]; + CORINFO_METHOD_HANDLE likelyMethod = likelyMethodes[candidateId]; + unsigned likelihood = likelihoods[candidateId]; - if ((likelyClassAttribs & CORINFO_FLG_ABSTRACT) != 0) + uint32_t likelyClassAttribs = 0; + if (likelyClass != NO_CLASS_HANDLE) { - // We may see an abstract likely class, if we have a stale profile. - // No point guessing for this. + likelyClassAttribs = info.compCompHnd->getClassAttribs(likelyClass); + + if ((likelyClassAttribs & CORINFO_FLG_ABSTRACT) != 0) + { + // We may see an abstract likely class, if we have a stale profile. + // No point guessing for this. + // + JITDUMP("Not guessing for class; abstract (stale profile)\n"); + return; + } + + // Figure out which method will be called. // - JITDUMP("Not guessing for class; abstract (stale profile)\n"); - return; - } + CORINFO_DEVIRTUALIZATION_INFO dvInfo; + dvInfo.virtualMethod = baseMethod; + dvInfo.objClass = likelyClass; + dvInfo.context = *pContextHandle; + dvInfo.exactContext = *pContextHandle; + dvInfo.pResolvedTokenVirtualMethod = nullptr; - // Figure out which method will be called. - // - CORINFO_DEVIRTUALIZATION_INFO dvInfo; - dvInfo.virtualMethod = baseMethod; - dvInfo.objClass = likelyClass; - dvInfo.context = *pContextHandle; - dvInfo.exactContext = *pContextHandle; - dvInfo.pResolvedTokenVirtualMethod = nullptr; + const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo); - const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo); + if (!canResolve) + { + JITDUMP("Can't figure out which method would be invoked, sorry\n"); + return; + } - if (!canResolve) - { - JITDUMP("Can't figure out which method would be invoked, sorry\n"); - return; + likelyMethod = dvInfo.devirtualizedMethod; } - likelyMethod = dvInfo.devirtualizedMethod; - } - - uint32_t likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod); + uint32_t likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod); - if (likelyClass == NO_CLASS_HANDLE) - { - // For method GDV do a few more checks that we get for free in the - // resolve call above for class-based GDV. - if ((likelyMethodAttribs & CORINFO_FLG_STATIC) != 0) + if (likelyClass == NO_CLASS_HANDLE) { - assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) || call->IsDelegateInvoke()); - JITDUMP("Cannot currently handle devirtualizing static delegate calls, sorry\n"); - return; - } + // For method GDV do a few more checks that we get for free in the + // resolve call above for class-based GDV. + if ((likelyMethodAttribs & CORINFO_FLG_STATIC) != 0) + { + assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) || call->IsDelegateInvoke()); + JITDUMP("Cannot currently handle devirtualizing static delegate calls, sorry\n"); + return; + } - CORINFO_CLASS_HANDLE definingClass = info.compCompHnd->getMethodClass(likelyMethod); - likelyClassAttribs = info.compCompHnd->getClassAttribs(definingClass); + CORINFO_CLASS_HANDLE definingClass = info.compCompHnd->getMethodClass(likelyMethod); + likelyClassAttribs = info.compCompHnd->getClassAttribs(definingClass); - // For instance methods on value classes we need an extended check to - // check for the unboxing stub. This is NYI. - // Note: For dynamic PGO likelyMethod above will be the unboxing stub - // which would fail GDV for other reasons. - // However, with static profiles or textual PGO input it is still - // possible that likelyMethod is not the unboxing stub. So we do need - // this explicit check. - if ((likelyClassAttribs & CORINFO_FLG_VALUECLASS) != 0) - { - JITDUMP("Cannot currently handle devirtualizing delegate calls on value types, sorry\n"); - return; - } + // For instance methods on value classes we need an extended check to + // check for the unboxing stub. This is NYI. + // Note: For dynamic PGO likelyMethod above will be the unboxing stub + // which would fail GDV for other reasons. + // However, with static profiles or textual PGO input it is still + // possible that likelyMethod is not the unboxing stub. So we do need + // this explicit check. + if ((likelyClassAttribs & CORINFO_FLG_VALUECLASS) != 0) + { + JITDUMP("Cannot currently handle devirtualizing delegate calls on value types, sorry\n"); + return; + } - // Verify that the call target and args look reasonable so that the JIT - // does not blow up during inlining/call morphing. - // - // NOTE: Once we want to support devirtualization of delegate calls to - // static methods and remove the check above we will start failing here - // for delegates pointing to static methods that have the first arg - // bound. For example: - // - // public static void E(this C c) ... - // Action a = new C().E; - // - // The delegate instance looks exactly like one pointing to an instance - // method in this case and the call will have zero args while the - // signature has 1 arg. - // - if (!isCompatibleMethodGDV(call, likelyMethod)) - { - JITDUMP("Target for method-based GDV is incompatible (stale profile?)\n"); - assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) && "Unexpected stale profile in dynamic PGO data"); - return; + // Verify that the call target and args look reasonable so that the JIT + // does not blow up during inlining/call morphing. + // + // NOTE: Once we want to support devirtualization of delegate calls to + // static methods and remove the check above we will start failing here + // for delegates pointing to static methods that have the first arg + // bound. For example: + // + // public static void E(this C c) ... + // Action a = new C().E; + // + // The delegate instance looks exactly like one pointing to an instance + // method in this case and the call will have zero args while the + // signature has 1 arg. + // + if (!isCompatibleMethodGDV(call, likelyMethod)) + { + JITDUMP("Target for method-based GDV is incompatible (stale profile?)\n"); + assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) && + "Unexpected stale profile in dynamic PGO data"); + return; + } } - } #ifdef DEBUG - char buffer[256]; - JITDUMP("%s call would invoke method %s\n", - isInterface ? "interface" : call->IsDelegateInvoke() ? "delegate" : "virtual", - eeGetMethodFullName(likelyMethod, true, true, buffer, sizeof(buffer))); + char buffer[256]; + JITDUMP("%s call would invoke method %s\n", + isInterface ? "interface" : call->IsDelegateInvoke() ? "delegate" : "virtual", + eeGetMethodFullName(likelyMethod, true, true, buffer, sizeof(buffer))); #endif - // Add this as a potential candidate. - // - addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs, - likelihood); + // Add this as a potential candidate. + // + addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs, + likelihood); + } } //------------------------------------------------------------------------ From 862a23b19b660e012afadfe62bf4ece1d752c9cf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 26 May 2023 23:10:35 +0200 Subject: [PATCH 2/9] Clean up --- src/coreclr/jit/importercalls.cpp | 47 +++++++++++++++------ src/coreclr/jit/indirectcalltransformer.cpp | 15 +++++-- 2 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b49c1dc888054d..15b3ead984c9e3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5584,12 +5584,13 @@ void Compiler::addFatPointerCandidate(GenTreeCall* call) // pickGDV: Use profile information to pick a GDV candidate for a call site. // // Arguments: -// call - the call -// ilOffset - exact IL offset of the call -// isInterface - whether or not the call target is defined on an interface -// classGuess - [out] the class to guess for (mutually exclusive with methodGuess) -// methodGuess - [out] the method to guess for (mutually exclusive with classGuess) -// likelihood - [out] an estimate of the likelihood that the guess will succeed +// call - the call +// ilOffset - exact IL offset of the call +// isInterface - whether or not the call target is defined on an interface +// classGuesses - [out] the classes to guess for (mutually exclusive with methodGuess) +// methodGuesses - [out] the methods to guess for (mutually exclusive with classGuess) +// candidatesCount - [out] number of guesses +// likelihoods - [out] estimates of the likelihoods that the guesses will succeed // void Compiler::pickGDV(GenTreeCall* call, IL_OFFSET ilOffset, @@ -5704,7 +5705,10 @@ void Compiler::pickGDV(GenTreeCall* call, classGuesses[0] = (CORINFO_CLASS_HANDLE)likelyClasses[index].handle; likelihoods[0] = 100; *candidatesCount = 1; - // TODO: report multiple candidates + // TODO: report multiple random candidates. For now we don't do it because with the current impl + // we might give up on all candidates if one of them is not inlinable, so we don't want to reduce + // testing coverage. + // JITDUMP("Picked random class for GDV: %p (%s)\n", classGuesses[0], eeGetClassName(classGuesses[0])); return; } @@ -5713,7 +5717,10 @@ void Compiler::pickGDV(GenTreeCall* call, methodGuesses[0] = (CORINFO_METHOD_HANDLE)likelyMethods[index - numberOfClasses].handle; likelihoods[0] = 100; *candidatesCount = 1; - // TODO: report multiple candidates + // TODO: report multiple random candidates. For now we don't do it because with the current impl + // we might give up on all candidates if one of them is not inlinable, so we don't want to reduce + // testing coverage. + // JITDUMP("Picked random method for GDV: %p (%s)\n", methodGuesses[0], eeGetMethodFullName(methodGuesses[0])); return; } @@ -5917,6 +5924,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, else { pickGDV(call, ilOffset, isInterface, likelyClasses, likelyMethodes, &candidatesCount, likelihoods); + assert((unsigned)candidatesCount <= MAX_GDV_TYPE_CHECKS); + assert((unsigned)candidatesCount <= JitConfig.JitGuardedDevirtualizationMaxTypeChecks()); if (candidatesCount == 0) { hasPgoData = false; @@ -5974,7 +5983,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (!info.compCompHnd->resolveVirtualMethod(&dvInfo)) { JITDUMP("Can't figure out which method would be invoked, sorry\n"); - return; + // Maybe other candidates will be resolved. + // Although, we no longer can remove the fallback (we never do it currently anyway) + break; } CORINFO_METHOD_HANDLE exactMethod = dvInfo.devirtualizedMethod; @@ -6005,6 +6016,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, return; } + // Iterate over the guesses for (int candidateId = 0; candidateId < candidatesCount; candidateId++) { CORINFO_CLASS_HANDLE likelyClass = likelyClasses[candidateId]; @@ -6022,7 +6034,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // No point guessing for this. // JITDUMP("Not guessing for class; abstract (stale profile)\n"); - return; + + // Continue checking other candidates, maybe some of them aren't stale. + break; } // Figure out which method will be called. @@ -6039,7 +6053,9 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (!canResolve) { JITDUMP("Can't figure out which method would be invoked, sorry\n"); - return; + + // Continue checking other candidates, maybe some of them will succeed. + break; } likelyMethod = dvInfo.devirtualizedMethod; @@ -6049,13 +6065,16 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (likelyClass == NO_CLASS_HANDLE) { + // We don't support multiple candidates for method guessing yet. + assert(candidateId == 0); + // For method GDV do a few more checks that we get for free in the // resolve call above for class-based GDV. if ((likelyMethodAttribs & CORINFO_FLG_STATIC) != 0) { assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) || call->IsDelegateInvoke()); JITDUMP("Cannot currently handle devirtualizing static delegate calls, sorry\n"); - return; + break; } CORINFO_CLASS_HANDLE definingClass = info.compCompHnd->getMethodClass(likelyMethod); @@ -6071,7 +6090,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if ((likelyClassAttribs & CORINFO_FLG_VALUECLASS) != 0) { JITDUMP("Cannot currently handle devirtualizing delegate calls on value types, sorry\n"); - return; + break; } // Verify that the call target and args look reasonable so that the JIT @@ -6094,7 +6113,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, JITDUMP("Target for method-based GDV is incompatible (stale profile?)\n"); assert((fgPgoSource != ICorJitInfo::PgoSource::Dynamic) && "Unexpected stale profile in dynamic PGO data"); - return; + break; } } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 15042ce6d744e3..fdeccaa0f5c4de 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -587,9 +587,18 @@ class IndirectCallTransformer prevCheckBlock->bbJumpDest = checkBlock; compiler->fgAddRefPred(checkBlock, prevCheckBlock); - // Weight for the new secondary check is the difference between the previous check and the thenBlock. - checkBlock->inheritWeightPercentage(prevCheckBlock, - 100 - origCall->GetGDVCandidateInfo(checkIdx)->likelihood); + // Calculate the total likelihood for this check as a sum of likelihoods + // of all previous candidates (thenBlocks) + unsigned checkLikelihood = 100; + for (uint8_t previousCandidate = 0; previousCandidate < checkIdx; previousCandidate++) + { + checkLikelihood -= origCall->GetGDVCandidateInfo(previousCandidate)->likelihood; + } + + // Make sure we didn't overflow + assert(checkLikelihood <= 100); + + checkBlock->inheritWeightPercentage(currBlock, checkLikelihood); } // Find last arg with a side effect. All args with any effect From 7ed025bac8adf2f25b04d326c8b443be86bfcb67 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 26 May 2023 23:18:06 +0200 Subject: [PATCH 3/9] Clean up --- src/coreclr/jit/importercalls.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 15b3ead984c9e3..3e08155d0070b7 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5749,7 +5749,7 @@ void Compiler::pickGDV(GenTreeCall* call, else if (maxNumberOfGuesses == 2) { // Two guesses - slightly relax the thresholds - likelihoodThreshold = isInterface ? 12 : 15; + likelihoodThreshold = isInterface ? 15 : 20; } else { @@ -5757,8 +5757,14 @@ void Compiler::pickGDV(GenTreeCall* call, likelihoodThreshold = 10; } + // We have 'maxNumberOfGuesses' number of classes available + // and we're allowed to make 'maxNumberOfGuesses' number of guesses + // Iterate over the available classes to find classes with likelihoods bigger than + // a specific threshold + // assert(*candidatesCount == 0); - for (unsigned guessIdx = 0; guessIdx < min((unsigned)maxNumberOfGuesses, numberOfClasses); guessIdx++) + unsigned totalGuesses = min((unsigned)maxNumberOfGuesses, numberOfClasses); + for (unsigned guessIdx = 0; guessIdx < totalGuesses; guessIdx++) { if (likelyClasses[guessIdx].likelihood >= likelihoodThreshold) { @@ -5925,7 +5931,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, { pickGDV(call, ilOffset, isInterface, likelyClasses, likelyMethodes, &candidatesCount, likelihoods); assert((unsigned)candidatesCount <= MAX_GDV_TYPE_CHECKS); - assert((unsigned)candidatesCount <= JitConfig.JitGuardedDevirtualizationMaxTypeChecks()); + assert((unsigned)candidatesCount <= (unsigned)JitConfig.JitGuardedDevirtualizationMaxTypeChecks()); if (candidatesCount == 0) { hasPgoData = false; From ce787adcf416a947fb110dcdeaa42386af428cc6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 27 May 2023 01:37:22 +0200 Subject: [PATCH 4/9] Clean up --- src/coreclr/jit/jitconfigvalues.h | 2 +- src/tests/Common/testenvironment.proj | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 6ab73d6abde378..3d3d8d9c538f65 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) +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 1) // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75 diff --git a/src/tests/Common/testenvironment.proj b/src/tests/Common/testenvironment.proj index 6caa56d38e91bb..4fb1c57a0a1e84 100644 --- a/src/tests/Common/testenvironment.proj +++ b/src/tests/Common/testenvironment.proj @@ -70,6 +70,7 @@ DOTNET_JitRandomEdgeCounts; DOTNET_JitRandomOnStackReplacement; DOTNET_JitRandomlyCollect64BitCounts; + DOTNET_JitGuardedDevirtualizationMaxTypeChecks; DOTNET_TieredPGO_InstrumentedTierAlwaysOptimized; DOTNET_JitForceControlFlowGuard; DOTNET_JitCFGUseDispatcher; @@ -225,7 +226,7 @@ - + From 03bc8f40a2f99fac71560057981732234ce2a1ac Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 27 May 2023 20:55:49 +0200 Subject: [PATCH 5/9] Fix asserts --- src/coreclr/jit/gentree.cpp | 18 +++++++++++++---- src/coreclr/jit/indirectcalltransformer.cpp | 22 ++++++++++++++++----- src/coreclr/jit/jitconfigvalues.h | 3 ++- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 833278adf22292..c3a592da0f1329 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -12400,11 +12400,21 @@ void Compiler::gtDispTree(GenTree* tree, printf(" (FramesRoot last use)"); } - if (((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) && - (call->GetSingleInlineCandidateInfo() != nullptr) && - (call->GetSingleInlineCandidateInfo()->exactContextHnd != nullptr)) + if ((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) { - printf(" (exactContextHnd=0x%p)", dspPtr(call->GetSingleInlineCandidateInfo()->exactContextHnd)); + InlineCandidateInfo* inlineInfo; + if (call->IsGuardedDevirtualizationCandidate()) + { + inlineInfo = call->GetGDVCandidateInfo(0); + } + else + { + inlineInfo = call->GetSingleInlineCandidateInfo(); + } + if ((inlineInfo != nullptr) && (inlineInfo->exactContextHnd != nullptr)) + { + printf(" (exactContextHnd=0x%p)", dspPtr(inlineInfo->exactContextHnd)); + } } gtDispCommonEndLine(tree); diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index fdeccaa0f5c4de..fdb7b1b3bdd04f 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -483,9 +483,7 @@ class IndirectCallTransformer assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); - // TODO: implement chaining for multiple GDV candidates - const bool canChainGdv = GetChecksCount() == 1; - if (canChainGdv) + if (IsChainingSupported()) { const bool isChainedGdv = (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0; @@ -518,6 +516,12 @@ class IndirectCallTransformer return "GuardedDevirtualization"; } + bool IsChainingSupported() + { + // TODO: implement chaining for multiple GDV candidates + return GetChecksCount() == 1; + } + //------------------------------------------------------------------------ // GetCall: find a call in a statement. // @@ -848,7 +852,11 @@ class IndirectCallTransformer // special candidate helper and we need to use the new 'this'. GenTreeCall* call = compiler->gtCloneCandidateCall(origCall); call->gtArgs.GetThisArg()->SetEarlyNode(compiler->gtNewLclvNode(thisTemp, TYP_REF)); - call->SetIsGuarded(); + + if (IsChainingSupported()) + { + call->SetIsGuarded(); + } JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), block->bbNum); @@ -1009,7 +1017,11 @@ class IndirectCallTransformer Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - call->SetIsGuarded(); + + if (IsChainingSupported()) + { + call->SetIsGuarded(); + } JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), elseBlock->bbNum); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 3d3d8d9c538f65..02721cb4132732 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -528,7 +528,8 @@ 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) // TODO: revert + // to 1 for now // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75 From 91fc31deb78d0af3ceaa2905238655395fad3747 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 28 May 2023 00:07:51 +0200 Subject: [PATCH 6/9] Update jitconfigvalues.h --- src/coreclr/jit/jitconfigvalues.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 02721cb4132732..3d3d8d9c538f65 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -528,8 +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) // TODO: revert - // to 1 for now +CONFIG_INTEGER(JitGuardedDevirtualizationMaxTypeChecks, W("JitGuardedDevirtualizationMaxTypeChecks"), 1) // Various policies for GuardedDevirtualization CONFIG_INTEGER(JitGuardedDevirtualizationChainLikelihood, W("JitGuardedDevirtualizationChainLikelihood"), 0x4B) // 75 From 03bda0dd9a1f8148cd574a32ad28efa70adc6d97 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 1 Jun 2023 14:45:14 +0200 Subject: [PATCH 7/9] Address feedback --- src/coreclr/jit/indirectcalltransformer.cpp | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index fdb7b1b3bdd04f..f6a48c3a458986 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -483,7 +483,9 @@ class IndirectCallTransformer assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); - if (IsChainingSupported()) + // TODO: implement chaining for multiple GDV candidates + const bool canChainGdv = GetChecksCount() == 1; + if (canChainGdv) { const bool isChainedGdv = (origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0; @@ -516,12 +518,6 @@ class IndirectCallTransformer return "GuardedDevirtualization"; } - bool IsChainingSupported() - { - // TODO: implement chaining for multiple GDV candidates - return GetChecksCount() == 1; - } - //------------------------------------------------------------------------ // GetCall: find a call in a statement. // @@ -853,10 +849,7 @@ class IndirectCallTransformer GenTreeCall* call = compiler->gtCloneCandidateCall(origCall); call->gtArgs.GetThisArg()->SetEarlyNode(compiler->gtNewLclvNode(thisTemp, TYP_REF)); - if (IsChainingSupported()) - { - call->SetIsGuarded(); - } + call->SetIsGuarded(); JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), block->bbNum); @@ -1018,10 +1011,7 @@ class IndirectCallTransformer call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - if (IsChainingSupported()) - { - call->SetIsGuarded(); - } + call->SetIsGuarded(); JITDUMP("Residual call [%06u] moved to block " FMT_BB "\n", compiler->dspTreeID(call), elseBlock->bbNum); From 4dfb1b995937c8ef66281102965422ef96f76431 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 1 Jun 2023 15:03:40 +0200 Subject: [PATCH 8/9] Address feedback --- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/indirectcalltransformer.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index aba0186a0f77f3..4afb8c55535495 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6343,7 +6343,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, JITDUMP("Revoking guarded devirtualization candidacy for call [%06u]: target method can't be inlined\n", dspTreeID(call)); - call->ClearGuardedDevirtualizationCandidate(); + call->ClearInlineInfo(); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index f6a48c3a458986..1a3e3ef021d287 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -534,12 +534,9 @@ class IndirectCallTransformer return call; } - //------------------------------------------------------------------------ - // ClearFlag: clear guarded devirtualization candidate flag from the original call. - // virtual void ClearFlag() { - origCall->ClearGuardedDevirtualizationCandidate(); + // We remove the GDV flag from the call in the CreateElse } virtual UINT8 GetChecksCount() @@ -986,6 +983,9 @@ class IndirectCallTransformer // virtual void CreateElse() { + // Remove everything related to inlining from the original call + origCall->ClearInlineInfo(); + elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; From 61f61b2b3c904f161c382b6e9c050a41b17a3ada Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 1 Jun 2023 21:53:00 +0200 Subject: [PATCH 9/9] Do `ClearInlineInfo` after we calculate elseBlock's likelihood - it used to produce unexpected diffs --- src/coreclr/jit/indirectcalltransformer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 1a3e3ef021d287..bb2f316bb327d6 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -983,9 +983,6 @@ class IndirectCallTransformer // virtual void CreateElse() { - // Remove everything related to inlining from the original call - origCall->ClearInlineInfo(); - elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; @@ -1004,6 +1001,9 @@ class IndirectCallTransformer // Make sure it didn't overflow assert(elseLikelihood <= 100); + // Remove everything related to inlining from the original call + origCall->ClearInlineInfo(); + elseBlock->inheritWeightPercentage(currBlock, elseLikelihood); GenTreeCall* call = origCall;