From c1c9626d98967267fbb28e90d69806196e7211b5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Sep 2021 21:59:25 +0300 Subject: [PATCH 01/26] GuardedDevirt: multiple type checks --- src/coreclr/jit/compiler.h | 2 - src/coreclr/jit/compmemkind.h | 1 + src/coreclr/jit/gentree.cpp | 6 +- src/coreclr/jit/gentree.h | 2 + src/coreclr/jit/importer.cpp | 127 +++++++++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 88 +++++++++++--- src/coreclr/jit/jitconfigvalues.h | 3 + src/coreclr/jit/morph.cpp | 1 + 8 files changed, 150 insertions(+), 80 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 351e49636dfded..795f5fc0f7936f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7301,8 +7301,6 @@ class Compiler void addGuardedDevirtualizationCandidate(GenTreeCall* call, CORINFO_METHOD_HANDLE methodHandle, CORINFO_CLASS_HANDLE classHandle, - unsigned methodAttr, - unsigned classAttr, unsigned likelihood); bool doesMethodHaveExpRuntimeLookup() diff --git a/src/coreclr/jit/compmemkind.h b/src/coreclr/jit/compmemkind.h index 969b0c3dc2a7b5..b70b2d0d57bb75 100644 --- a/src/coreclr/jit/compmemkind.h +++ b/src/coreclr/jit/compmemkind.h @@ -60,6 +60,7 @@ CompMemKindMacro(TailMergeThrows) CompMemKindMacro(EarlyProp) CompMemKindMacro(ZeroInit) CompMemKindMacro(Pgo) +CompMemKindMacro(GDVCandidates) //clang-format on #undef CompMemKindMacro diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 48a1e04d189b48..50c3890d63a61e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6424,8 +6424,9 @@ GenTreeCall* Compiler::gtNewCallNode( { node->gtInlineCandidateInfo = nullptr; } - node->gtCallLateArgs = nullptr; - node->gtReturnType = type; + node->gtCallLateArgs = nullptr; + node->gtReturnType = type; + node->gtGDVCandidatesCount = 0; #ifdef FEATURE_READYTORUN node->gtEntryPoint.addr = nullptr; @@ -8309,6 +8310,7 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree, { copy->gtCallMethHnd = tree->gtCallMethHnd; copy->gtInlineCandidateInfo = nullptr; + copy->gtGDVCandidatesCount = 0; } if (tree->fgArgInfo) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b54475bd2ce88b..81ad691f27023f 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4788,6 +4788,8 @@ struct GenTreeCall final : public GenTree IL_OFFSET gtRawILOffset; #endif // defined(DEBUG) || defined(INLINE_DATA) + UINT8 gtGDVCandidatesCount; + bool IsHelperCall() const { return gtCallType == CT_HELPER; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f31f5df4309806..8c15fa1092bed9 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21902,11 +21902,8 @@ void Compiler::considerGuardedDevirtualization( // See if there's a likely guess for the class. // const unsigned likelihoodThreshold = isInterface ? 25 : 30; - unsigned likelihood = 0; unsigned numberOfClasses = 0; - CORINFO_CLASS_HANDLE likelyClass = NO_CLASS_HANDLE; - bool doRandomDevirt = false; const int maxLikelyClasses = 32; @@ -21940,17 +21937,12 @@ void Compiler::considerGuardedDevirtualization( // For now we only use the most popular type - likelihood = likelyClasses[0].likelihood; - likelyClass = likelyClasses[0].clsHandle; - if (numberOfClasses < 1) { JITDUMP("No likely class, sorry\n"); return; } - assert(likelyClass != NO_CLASS_HANDLE); - // Print all likely classes JITDUMP("%s classes for %p (%s):\n", doRandomDevirt ? "Random" : "Likely", dspPtr(objClass), objClassName) for (UINT32 i = 0; i < numberOfClasses; i++) @@ -21959,44 +21951,53 @@ void Compiler::considerGuardedDevirtualization( eeGetClassName(likelyClasses[i].clsHandle), likelyClasses[i].likelihood); } - // Todo: a more advanced heuristic using likelihood, number of - // classes, and the profile count for this block. - // - // For now we will guess if the likelihood is at least 25%/30% (intfc/virt), as studies - // have shown this transformation should pay off even if we guess wrong sometimes. - // - if (likelihood < likelihoodThreshold) + assert(call->gtGDVCandidatesCount == 0); + + for (UINT i = 0; i < numberOfClasses; i++) { - JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, likelihoodThreshold); - return; - } + if (call->gtGDVCandidatesCount >= (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount()) + { + break; + } - // 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; + assert(likelyClasses[i].clsHandle != NO_CLASS_HANDLE); + + // Todo: a more advanced heuristic using likelihood, number of + // classes, and the profile count for this block. + // + // For now we will guess if the likelihood is at least 25%/30% (intfc/virt), as studies + // have shown this transformation should pay off even if we guess wrong sometimes. + // + if ((likelyClasses[i].likelihood < likelihoodThreshold / (i + 1))) + { + JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, likelihoodThreshold / (i + 1)); + break; + } - const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo); + // Figure out which method will be called. + // + CORINFO_DEVIRTUALIZATION_INFO dvInfo = {}; + dvInfo.virtualMethod = baseMethod; + dvInfo.objClass = likelyClasses[i].clsHandle; + dvInfo.context = *pContextHandle; + dvInfo.exactContext = *pContextHandle; + dvInfo.pResolvedTokenVirtualMethod = nullptr; - if (!canResolve) - { - JITDUMP("Can't figure out which method would be invoked, sorry\n"); - return; - } + const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo); - CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; - JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); + if (!canResolve) + { + JITDUMP("Can't figure out which method would be invoked, sorry\n"); + continue; + } - // Add this as a potential candidate. - // - uint32_t const likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod); - uint32_t const likelyClassAttribs = info.compCompHnd->getClassAttribs(likelyClass); - addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs, - likelihood); + CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; + JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); + + // Add this as a potential candidate. + // + addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle, likelyClasses[i].likelihood); + } } //------------------------------------------------------------------------ @@ -22016,15 +22017,11 @@ void Compiler::considerGuardedDevirtualization( // call - potential guarded devirtualization candidate // methodHandle - method that will be invoked if the class test succeeds // classHandle - class that will be tested for at runtime -// methodAttr - attributes of the method -// classAttr - attributes of the class // likelihood - odds that this class is the class seen at runtime // void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, CORINFO_METHOD_HANDLE methodHandle, CORINFO_CLASS_HANDLE classHandle, - unsigned methodAttr, - unsigned classAttr, unsigned likelihood) { // This transformation only makes sense for virtual calls @@ -22091,17 +22088,17 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, // Gather some information for later. Note we actually allocate InlineCandidateInfo // here, as the devirtualized half of this call will likely become an inline candidate. // - GuardedDevirtualizationCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo; + InlineCandidateInfo pInfo = {}; - pInfo->guardedMethodHandle = methodHandle; - pInfo->guardedMethodUnboxedEntryHandle = nullptr; - pInfo->guardedClassHandle = classHandle; - pInfo->likelihood = likelihood; - pInfo->requiresInstMethodTableArg = false; + pInfo.guardedMethodHandle = methodHandle; + pInfo.guardedMethodUnboxedEntryHandle = nullptr; + pInfo.guardedClassHandle = classHandle; + pInfo.likelihood = likelihood; + pInfo.requiresInstMethodTableArg = false; // If the guarded class is a value class, look for an unboxed entry point. // - if ((classAttr & CORINFO_FLG_VALUECLASS) != 0) + if ((info.compCompHnd->getClassAttribs(classHandle) & CORINFO_FLG_VALUECLASS) != 0) { JITDUMP(" ... class is a value class, looking for unboxed entry\n"); bool requiresInstMethodTableArg = false; @@ -22111,8 +22108,8 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, if (unboxedEntryMethodHandle != nullptr) { JITDUMP(" ... updating GDV candidate with unboxed entry info\n"); - pInfo->guardedMethodUnboxedEntryHandle = unboxedEntryMethodHandle; - pInfo->requiresInstMethodTableArg = requiresInstMethodTableArg; + pInfo.guardedMethodUnboxedEntryHandle = unboxedEntryMethodHandle; + pInfo.requiresInstMethodTableArg = requiresInstMethodTableArg; } } @@ -22121,14 +22118,32 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, if (call->IsVirtualStub()) { JITDUMP("Saving stub addr %p in candidate info\n", dspPtr(call->gtStubCallStubAddr)); - pInfo->stubAddr = call->gtStubCallStubAddr; + pInfo.stubAddr = call->gtStubCallStubAddr; } else { - pInfo->stubAddr = nullptr; + pInfo.stubAddr = nullptr; } - call->gtGuardedDevirtualizationCandidateInfo = pInfo; + const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); + const UINT8 candidatesCount = ++call->gtGDVCandidatesCount; + assert(maxCandidates >= candidatesCount); + + if (candidatesCount == 1) + { + // in 90% cases virtual calls are monomorphic so let's avoid allocating too much + call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1] { pInfo }; + } + else if (candidatesCount == 2) + { + // Ok, re-alloc to store multiple candidates. + InlineCandidateInfo firstInfo = call->gtInlineCandidateInfo[0]; + call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[maxCandidates]{ firstInfo, pInfo }; + } + else + { + call->gtInlineCandidateInfo[candidatesCount - 1] = pInfo; + } } void Compiler::addExpRuntimeLookupCandidate(GenTreeCall* call) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 6be64a86b6ebbb..755f223787bf52 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -209,8 +209,11 @@ class IndirectCallTransformer FixupRetExpr(); ClearFlag(); CreateRemainder(); - CreateCheck(); - CreateThen(); + for (UINT8 i = 0; i < GetChecksCount(); i++) + { + CreateCheck(i); + CreateThen(i); + } CreateElse(); RemoveOldStatement(); SetWeights(); @@ -233,7 +236,7 @@ class IndirectCallTransformer remainderBlock->bbFlags |= BBF_INTERNAL; } - virtual void CreateCheck() = 0; + virtual void CreateCheck(UINT8 checkIdx) = 0; //------------------------------------------------------------------------ // CreateAndInsertBasicBlock: ask compiler to create new basic block. @@ -252,7 +255,7 @@ class IndirectCallTransformer return block; } - virtual void CreateThen() = 0; + virtual void CreateThen(UINT8 checkIdx) = 0; virtual void CreateElse() = 0; //------------------------------------------------------------------------ @@ -273,6 +276,14 @@ class IndirectCallTransformer thenBlock->inheritWeightPercentage(currBlock, likelihood); elseBlock->inheritWeightPercentage(currBlock, 100 - likelihood); } + + //------------------------------------------------------------------------ + // GetChecksCount: Get number of Check-Then nodes + // + virtual UINT8 GetChecksCount() + { + return 1; + } //------------------------------------------------------------------------ // ChainFlow: link new blocks into correct cfg. @@ -355,7 +366,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateCheck: create check block, that checks fat pointer bit set. // - virtual void CreateCheck() + virtual void CreateCheck(UINT8 checkIdx) { checkBlock = CreateAndInsertBasicBlock(BBJ_COND, currBlock); GenTree* fatPointerMask = new (compiler, GT_CNS_INT) GenTreeIntCon(TYP_I_IMPL, FAT_POINTER_MASK); @@ -372,7 +383,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 checkIdx) { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); Statement* copyOfOriginalStmt = compiler->gtCloneStmt(stmt); @@ -579,12 +590,22 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateCheck: create check block and check method table // - virtual void CreateCheck() + virtual void CreateCheck(UINT8 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) + { + checkBlock = currBlock; + checkBlock->bbJumpKind = BBJ_COND; + } + else + { + BasicBlock* prevCheckBlock = checkBlock; + checkBlock = CreateAndInsertBasicBlock(BBJ_COND, thenBlock); + checkBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + prevCheckBlock->bbJumpDest = checkBlock; + } // Fetch method table from object arg to call. GenTree* thisTree = compiler->gtCloneExpr(origCall->gtCallThisArg->GetNode()); @@ -615,10 +636,9 @@ class IndirectCallTransformer // Find target method table // - GenTree* methodTable = compiler->gtNewMethodTableLookup(thisTree); - GuardedDevirtualizationCandidateInfo* guardedInfo = origCall->gtGuardedDevirtualizationCandidateInfo; - CORINFO_CLASS_HANDLE clsHnd = guardedInfo->guardedClassHandle; - GenTree* targetMethodTable = compiler->gtNewIconEmbClsHndNode(clsHnd); + GenTree* methodTable = compiler->gtNewMethodTableLookup(thisTree); + CORINFO_CLASS_HANDLE clsHnd = origCall->gtInlineCandidateInfo[checkIdx].guardedClassHandle; + GenTree* targetMethodTable = compiler->gtNewIconEmbClsHndNode(clsHnd); // Compare and jump to else (which does the indirect call) if NOT equal // @@ -719,15 +739,24 @@ class IndirectCallTransformer } } + //------------------------------------------------------------------------ + // GetChecksCount: Get number of Check-Then + // + virtual UINT8 GetChecksCount() override + { + return origCall->gtGDVCandidatesCount; + } + //------------------------------------------------------------------------ // CreateThen: create then block with direct call to method // - virtual void CreateThen() + virtual void CreateThen(UINT8 checkIdx) { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; + thenBlock->bbJumpDest = remainderBlock; - InlineCandidateInfo* inlineInfo = origCall->gtInlineCandidateInfo; + InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[checkIdx]; CORINFO_CLASS_HANDLE clsHnd = inlineInfo->guardedClassHandle; // copy 'this' to temp with exact type. @@ -760,11 +789,30 @@ class IndirectCallTransformer // assert(!call->IsVirtual()); + if (checkIdx > 0) + { + call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; + call->gtInlineCandidateInfo = nullptr; + + if (returnTemp != BAD_VAR_NUM) + { + GenTree* const assign = compiler->gtNewTempAssign(returnTemp, call); + compiler->fgNewStmtAtEnd(thenBlock, assign); + } + else + { + compiler->fgNewStmtAtEnd(thenBlock, call); + } + return; + } + // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. // + // Also, give up on inlining secondary guess. TODO: figure out what is missing to be able to inline them. + // CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; - if ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd)) + if ((checkIdx > 0) || ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd))) { // Demote this call to a non-inline candidate // @@ -1024,10 +1072,10 @@ class IndirectCallTransformer GenTreeCall* const call = root->AsCall(); if (call->IsGuardedDevirtualizationCandidate() && - (call->gtGuardedDevirtualizationCandidateInfo->likelihood >= gdvChainLikelihood)) + (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", - compiler->dspTreeID(call), call->gtGuardedDevirtualizationCandidateInfo->likelihood, + compiler->dspTreeID(call), call->gtInlineCandidateInfo->likelihood, gdvChainLikelihood, chainStatementDup, chainNodeDup); call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_CHAIN; @@ -1157,7 +1205,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateCheck: create check blocks, that checks dictionary size and does null test. // - virtual void CreateCheck() override + virtual void CreateCheck(UINT8 checkIdx) override { GenTreeCall::UseIterator argsIter = origCall->Args().begin(); GenTree* nullCheck = argsIter.GetUse()->GetNode(); @@ -1184,7 +1232,7 @@ class IndirectCallTransformer // CreateThen: create then block, that is executed if the checks succeed. // This simply returns the handle. // - virtual void CreateThen() override + virtual void CreateThen(UINT8 checkIdx) override { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock2); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index c86c600acb6563..5578fecc199612 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -496,6 +496,9 @@ CONFIG_STRING(JitGuardedDevirtualizationRange, W("JitGuardedDevirtualizationRang CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualization"), 0) #endif // DEBUG +// Number of type checks for a virtual call +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 2) + // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) // Initial patchpoint counter value used by jitted code diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b58559b1bf43e4..407396c8e7f0d6 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -74,6 +74,7 @@ GenTree* Compiler::fgMorphIntoHelperCall(GenTree* tree, int helper, GenTreeCall: call->gtCallMoreFlags = GTF_CALL_M_EMPTY; call->gtInlineCandidateInfo = nullptr; call->gtControlExpr = nullptr; + call->gtGDVCandidatesCount = 0; #ifdef UNIX_X86_ABI call->gtFlags |= GTF_CALL_POP_ARGS; #endif // UNIX_X86_ABI From f58f93066989c80bd12a1afe03df1ebc39ed4b3b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Sep 2021 22:36:00 +0300 Subject: [PATCH 02/26] Fix weights --- src/coreclr/jit/importer.cpp | 1 + src/coreclr/jit/indirectcalltransformer.cpp | 19 +++++++++++++++++++ src/coreclr/jit/jitconfigvalues.h | 2 +- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8c15fa1092bed9..9fe5217f869f26 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -20661,6 +20661,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { + // TODO: there can be multiple candidates if (call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr) { fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 755f223787bf52..698c53858feb56 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -598,6 +598,7 @@ class IndirectCallTransformer { checkBlock = currBlock; checkBlock->bbJumpKind = BBJ_COND; + checkBlock->inheritWeight(currBlock); } else { @@ -605,6 +606,8 @@ class IndirectCallTransformer checkBlock = CreateAndInsertBasicBlock(BBJ_COND, thenBlock); checkBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; prevCheckBlock->bbJumpDest = checkBlock; + + checkBlock->inheritWeightPercentage(thenBlock, 50); // not sure in this one } // Fetch method table from object arg to call. @@ -755,6 +758,7 @@ class IndirectCallTransformer thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; thenBlock->bbJumpDest = remainderBlock; + thenBlock->inheritWeightPercentage(currBlock, origCall->gtInlineCandidateInfo[checkIdx].likelihood); InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[checkIdx]; CORINFO_CLASS_HANDLE clsHnd = inlineInfo->guardedClassHandle; @@ -892,6 +896,13 @@ class IndirectCallTransformer newStmt->SetRootNode(assign); } + UINT32 elseLikelihood = 100; + for (UINT8 i = 0; i < origCall->gtGDVCandidatesCount; i++) + { + elseLikelihood -= origCall->gtInlineCandidateInfo[i].likelihood; + } + elseBlock->inheritWeightPercentage(currBlock, elseLikelihood > 100 /*overflow*/ ? 0 : elseLikelihood); + // For stub calls, restore the stub address. For everything else, // null out the candidate info field. if (call->IsVirtualStub()) @@ -911,6 +922,14 @@ class IndirectCallTransformer stmt->SetRootNode(compiler->gtNewNothingNode()); } + //------------------------------------------------------------------------ + // SetWeights: set weights for new blocks. + // + virtual void SetWeights() override + { + remainderBlock->inheritWeight(currBlock); + } + // For chained gdv, we modify the expansion as follows: // // We verify the check block has two BBJ_NONE/ALWAYS predecessors, one of diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 5578fecc199612..844944d63bd5b6 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -497,7 +497,7 @@ CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualiza #endif // DEBUG // Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 2) +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 1) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) From ad6daec9d83bc999578e984604c48582c92aa303 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Sep 2021 22:48:33 +0300 Subject: [PATCH 03/26] remove redundant block --- src/coreclr/jit/indirectcalltransformer.cpp | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 698c53858feb56..56c6605e20af07 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -793,23 +793,6 @@ class IndirectCallTransformer // assert(!call->IsVirtual()); - if (checkIdx > 0) - { - call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - call->gtInlineCandidateInfo = nullptr; - - if (returnTemp != BAD_VAR_NUM) - { - GenTree* const assign = compiler->gtNewTempAssign(returnTemp, call); - compiler->fgNewStmtAtEnd(thenBlock, assign); - } - else - { - compiler->fgNewStmtAtEnd(thenBlock, call); - } - return; - } - // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. // From 7d7bbac1d4211243973f318b693adb78d3e9ae14 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Sep 2021 23:39:04 +0300 Subject: [PATCH 04/26] fix build --- src/coreclr/jit/indirectcalltransformer.cpp | 16 ++++++++-------- src/coreclr/jit/jitconfigvalues.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 56c6605e20af07..d8a601941a3e33 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -518,7 +518,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // Run: transform the statement as described above. // - virtual void Run() + virtual void Run() override { origCall = GetCall(stmt); @@ -558,7 +558,7 @@ class IndirectCallTransformer } protected: - virtual const char* Name() + virtual const char* Name() override { return "GuardedDevirtualization"; } @@ -571,7 +571,7 @@ class IndirectCallTransformer // // Return Value: // call tree node pointer. - virtual GenTreeCall* GetCall(Statement* callStmt) + virtual GenTreeCall* GetCall(Statement* callStmt) override { GenTree* tree = callStmt->GetRootNode(); assert(tree->IsCall()); @@ -582,7 +582,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // ClearFlag: clear guarded devirtualization candidate flag from the original call. // - virtual void ClearFlag() + virtual void ClearFlag() override { origCall->ClearGuardedDevirtualizationCandidate(); } @@ -590,7 +590,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateCheck: create check block and check method table // - virtual void CreateCheck(UINT8 checkIdx) + virtual void CreateCheck(UINT8 checkIdx) override { // There's no need for a new block here. We can just append to currBlock. // @@ -654,7 +654,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // FixupRetExpr: set up to repair return value placeholder from call // - virtual void FixupRetExpr() + virtual void FixupRetExpr() override { // If call returns a value, we need to copy it to a temp, and // bash the associated GT_RET_EXPR to refer to the temp instead @@ -753,7 +753,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateThen: create then block with direct call to method // - virtual void CreateThen(UINT8 checkIdx) + virtual void CreateThen(UINT8 checkIdx) override { thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; @@ -861,7 +861,7 @@ class IndirectCallTransformer //------------------------------------------------------------------------ // CreateElse: create else block. This executes the unaltered indirect call. // - virtual void CreateElse() + virtual void CreateElse() override { elseBlock = CreateAndInsertBasicBlock(BBJ_NONE, thenBlock); elseBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 844944d63bd5b6..a3ad38d3d587b7 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -497,7 +497,7 @@ CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualiza #endif // DEBUG // Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 1) +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 3) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) From c629fa1274559514a72115e8dd1c185534b5dfb3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 26 Sep 2021 00:17:14 +0300 Subject: [PATCH 05/26] Fix weights --- src/coreclr/jit/indirectcalltransformer.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index d8a601941a3e33..d1a1b6d42db618 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -606,8 +606,7 @@ class IndirectCallTransformer checkBlock = CreateAndInsertBasicBlock(BBJ_COND, thenBlock); checkBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; prevCheckBlock->bbJumpDest = checkBlock; - - checkBlock->inheritWeightPercentage(thenBlock, 50); // not sure in this one + checkBlock->setBBProfileWeight(prevCheckBlock->bbWeight - thenBlock->bbWeight); } // Fetch method table from object arg to call. From c2cbe9d56255778d2647413a788708bccbcc9143 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 26 Sep 2021 00:18:56 +0300 Subject: [PATCH 06/26] jit-format --- src/coreclr/jit/importer.cpp | 23 ++++++++++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 12 +++++------ 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 9fe5217f869f26..874599eab8252b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21971,18 +21971,19 @@ void Compiler::considerGuardedDevirtualization( // if ((likelyClasses[i].likelihood < likelihoodThreshold / (i + 1))) { - JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, likelihoodThreshold / (i + 1)); + JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, + likelihoodThreshold / (i + 1)); break; } // Figure out which method will be called. // CORINFO_DEVIRTUALIZATION_INFO dvInfo = {}; - dvInfo.virtualMethod = baseMethod; - dvInfo.objClass = likelyClasses[i].clsHandle; - dvInfo.context = *pContextHandle; - dvInfo.exactContext = *pContextHandle; - dvInfo.pResolvedTokenVirtualMethod = nullptr; + dvInfo.virtualMethod = baseMethod; + dvInfo.objClass = likelyClasses[i].clsHandle; + dvInfo.context = *pContextHandle; + dvInfo.exactContext = *pContextHandle; + dvInfo.pResolvedTokenVirtualMethod = nullptr; const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo); @@ -21997,7 +21998,8 @@ void Compiler::considerGuardedDevirtualization( // Add this as a potential candidate. // - addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle, likelyClasses[i].likelihood); + addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle, + likelyClasses[i].likelihood); } } @@ -22126,20 +22128,21 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, pInfo.stubAddr = nullptr; } - const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); + const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); const UINT8 candidatesCount = ++call->gtGDVCandidatesCount; assert(maxCandidates >= candidatesCount); if (candidatesCount == 1) { // in 90% cases virtual calls are monomorphic so let's avoid allocating too much - call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1] { pInfo }; + call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1]{pInfo}; } else if (candidatesCount == 2) { // Ok, re-alloc to store multiple candidates. InlineCandidateInfo firstInfo = call->gtInlineCandidateInfo[0]; - call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[maxCandidates]{ firstInfo, pInfo }; + call->gtInlineCandidateInfo = + new (this, CMK_GDVCandidates) InlineCandidateInfo[maxCandidates]{firstInfo, pInfo}; } else { diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index d1a1b6d42db618..342fc98e39163a 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -256,7 +256,7 @@ class IndirectCallTransformer } virtual void CreateThen(UINT8 checkIdx) = 0; - virtual void CreateElse() = 0; + virtual void CreateElse() = 0; //------------------------------------------------------------------------ // RemoveOldStatement: remove original stmt from current block. @@ -276,7 +276,7 @@ class IndirectCallTransformer thenBlock->inheritWeightPercentage(currBlock, likelihood); elseBlock->inheritWeightPercentage(currBlock, 100 - likelihood); } - + //------------------------------------------------------------------------ // GetChecksCount: Get number of Check-Then nodes // @@ -596,7 +596,7 @@ class IndirectCallTransformer // if (checkIdx == 0) { - checkBlock = currBlock; + checkBlock = currBlock; checkBlock->bbJumpKind = BBJ_COND; checkBlock->inheritWeight(currBlock); } @@ -742,7 +742,7 @@ class IndirectCallTransformer } //------------------------------------------------------------------------ - // GetChecksCount: Get number of Check-Then + // GetChecksCount: Get number of Check-Then // virtual UINT8 GetChecksCount() override { @@ -1076,8 +1076,8 @@ class IndirectCallTransformer (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", - compiler->dspTreeID(call), call->gtInlineCandidateInfo->likelihood, - gdvChainLikelihood, chainStatementDup, chainNodeDup); + compiler->dspTreeID(call), call->gtInlineCandidateInfo->likelihood, gdvChainLikelihood, + chainStatementDup, chainNodeDup); call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_CHAIN; break; From 74959134a43cc1f9146a93ab36478ff4d8e19631 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 26 Sep 2021 01:30:01 +0300 Subject: [PATCH 07/26] Remove gtGuardedDevirtualizationCandidateInfo field --- src/coreclr/jit/gentree.h | 7 +++-- src/coreclr/jit/importer.cpp | 30 ++++++++++++++++----- src/coreclr/jit/indirectcalltransformer.cpp | 16 ++++++++--- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 81ad691f27023f..7f9c6d31bb3a4b 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4758,10 +4758,9 @@ struct GenTreeCall final : public GenTree // only used for CALLI unmanaged calls (CT_INDIRECT) GenTree* gtCallCookie; // gtInlineCandidateInfo is only used when inlining methods - InlineCandidateInfo* gtInlineCandidateInfo; - GuardedDevirtualizationCandidateInfo* gtGuardedDevirtualizationCandidateInfo; - ClassProfileCandidateInfo* gtClassProfileCandidateInfo; - void* gtStubCallStubAddr; // GTF_CALL_VIRT_STUB - these are never inlined + InlineCandidateInfo* gtInlineCandidateInfo; + ClassProfileCandidateInfo* gtClassProfileCandidateInfo; + void* gtStubCallStubAddr; // GTF_CALL_VIRT_STUB - these are never inlined CORINFO_GENERIC_HANDLE compileTimeHelperArgumentHandle; // Used to track type handle argument of dynamic helpers void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen }; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 874599eab8252b..668b0328f8de20 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -6902,8 +6902,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", - dspPtr(call->gtGuardedDevirtualizationCandidateInfo->stubAddr)); - call->gtStubCallStubAddr = call->gtGuardedDevirtualizationCandidateInfo->stubAddr; + dspPtr(call->gtInlineCandidateInfo->stubAddr)); + call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; } } } @@ -20531,8 +20531,8 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", - dspPtr(call->gtGuardedDevirtualizationCandidateInfo->stubAddr)); - call->gtStubCallStubAddr = call->gtGuardedDevirtualizationCandidateInfo->stubAddr; + dspPtr(call->gtInlineCandidateInfo->stubAddr)); + call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; } } @@ -20662,13 +20662,13 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { // TODO: there can be multiple candidates - if (call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr) + if (call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr) { - fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle; + fncHandle = call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle; } else { - fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodHandle; + fncHandle = call->gtInlineCandidateInfo->guardedMethodHandle; } methAttr = info.compCompHnd->getMethodAttribs(fncHandle); } @@ -21996,6 +21996,22 @@ void Compiler::considerGuardedDevirtualization( CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); + if (i > 0) + { + // TODO: skip valuetypes and generics for secondary guesses for now + CORINFO_SIG_INFO sig; + info.compCompHnd->getMethodSig(likelyMethod, &sig); + if ((sig.sigInst.methInstCount != 0) || (sig.sigInst.classInstCount != 0)) + { + break; + } + + if ((info.compCompHnd->getClassAttribs(likelyClasses[i].clsHandle) & CORINFO_FLG_VALUECLASS) != 0) + { + break; + } + } + // Add this as a potential candidate. // addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle, diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 342fc98e39163a..5cfa7a1581b30b 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -534,7 +534,7 @@ class IndirectCallTransformer return; } - likelihood = origCall->gtGuardedDevirtualizationCandidateInfo->likelihood; + likelihood = origCall->gtInlineCandidateInfo->likelihood; assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); @@ -790,7 +790,13 @@ class IndirectCallTransformer // We know this call can devirtualize or we would not have set up GDV here. // So impDevirtualizeCall should succeed in devirtualizing. // - assert(!call->IsVirtual()); + if (call->IsVirtual()) + { + // TODO: fix + assert(checkIdx > 0); + call = compiler->gtCloneCandidateCall(origCall); + call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; + } // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. @@ -806,7 +812,9 @@ class IndirectCallTransformer "inlineable\n"); call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; - call->gtInlineCandidateInfo = nullptr; + + if (!call->IsVirtualStub()) + call->gtInlineCandidateInfo = nullptr; if (returnTemp != BAD_VAR_NUM) { @@ -1072,7 +1080,7 @@ class IndirectCallTransformer { GenTreeCall* const call = root->AsCall(); - if (call->IsGuardedDevirtualizationCandidate() && + if (call->IsGuardedDevirtualizationCandidate() && (call->gtGDVCandidatesCount == 1) && (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", From 781c2e78a67e5077a6ff9826a4edd37b07aef1b9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 2 Oct 2021 16:01:00 +0300 Subject: [PATCH 08/26] fix inlining for void methods --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/fginline.cpp | 1 + src/coreclr/jit/gentree.cpp | 14 +- src/coreclr/jit/gentree.h | 6 + src/coreclr/jit/importer.cpp | 65 +++++++--- src/coreclr/jit/indirectcalltransformer.cpp | 137 ++++++++++---------- src/coreclr/jit/morph.cpp | 22 ++-- 7 files changed, 144 insertions(+), 105 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 795f5fc0f7936f..244a2256ff2122 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4767,7 +4767,8 @@ class Compiler unsigned methAttr, CORINFO_CONTEXT_HANDLE exactContextHnd, InlineCandidateInfo** ppInlineCandidateInfo, - InlineResult* inlineResult); + InlineResult* inlineResult, + UINT8 gdvCandidateId); void impInlineRecordArgInfo(InlineInfo* pInlineInfo, GenTree* curArgVal, @@ -4795,6 +4796,7 @@ class Compiler void impMarkInlineCandidateHelper(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, + UINT8 candidateIndex, CORINFO_CALL_INFO* callInfo); bool impTailCallRetTypeCompatible(bool allowWidening, diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index a258c5d8093fed..5bb5faae86ef1a 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -888,6 +888,7 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe inlineInfo.hasSIMDTypeArgLocalOrReturn = false; #endif // FEATURE_SIMD + assert(call->gtGDVCandidatesCount <= 1); InlineCandidateInfo* inlineCandidateInfo = call->gtInlineCandidateInfo; noway_assert(inlineCandidateInfo); // Store the link to inlineCandidateInfo into inlineInfo diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 50c3890d63a61e..f9b3396f3f6045 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6415,6 +6415,9 @@ GenTreeCall* Compiler::gtNewCallNode( node->gtRetClsHnd = nullptr; node->gtControlExpr = nullptr; node->gtCallMoreFlags = GTF_CALL_M_EMPTY; + node->gtCallLateArgs = nullptr; + node->gtReturnType = type; + node->gtGDVCandidatesCount = 0; if (callType == CT_INDIRECT) { @@ -6422,11 +6425,8 @@ GenTreeCall* Compiler::gtNewCallNode( } else { - node->gtInlineCandidateInfo = nullptr; + node->ClearInlineInfo(); } - node->gtCallLateArgs = nullptr; - node->gtReturnType = type; - node->gtGDVCandidatesCount = 0; #ifdef FEATURE_READYTORUN node->gtEntryPoint.addr = nullptr; @@ -8308,9 +8308,9 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree, } else { - copy->gtCallMethHnd = tree->gtCallMethHnd; - copy->gtInlineCandidateInfo = nullptr; - copy->gtGDVCandidatesCount = 0; + copy->gtCallMethHnd = tree->gtCallMethHnd; + copy->gtGDVCandidatesCount = 0; + copy->ClearInlineInfo(); } if (tree->fgArgInfo) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 7f9c6d31bb3a4b..618790e8d9ab0a 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4765,6 +4765,12 @@ struct GenTreeCall final : public GenTree void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen }; + void ClearInlineInfo() + { + //assert(gtGDVCandidatesCount <= 1); + gtInlineCandidateInfo = nullptr; + } + // expression evaluated after args are placed which determines the control target GenTree* gtControlExpr; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 668b0328f8de20..6e2e672ff46c87 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9549,14 +9549,17 @@ var_types Compiler::impImportCall(OPCODE opcode, // Make the call its own tree (spill the stack if needed). impAppendTree(call, (unsigned)CHECK_SPILL_ALL, impCurStmtOffs); - // TODO: Still using the widened type. - GenTree* retExpr = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); - - // Link the retExpr to the call so if necessary we can manipulate it later. - origCall->gtInlineCandidateInfo->retExpr = retExpr; + for (UINT8 i = 0; i < max(1, origCall->gtGDVCandidatesCount); i++) + { + // TODO: Still using the widened type. + GenTree* retExpr = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); + // Link the retExpr to the call so if necessary we can manipulate it later. + InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[i]; + inlineInfo->retExpr = retExpr; + } // Propagate retExpr as the placeholder for the call. - call = retExpr; + call = origCall->gtInlineCandidateInfo[0].retExpr; } else { @@ -19356,7 +19359,8 @@ void Compiler::impCheckCanInline(GenTreeCall* call, unsigned methAttr, CORINFO_CONTEXT_HANDLE exactContextHnd, InlineCandidateInfo** ppInlineCandidateInfo, - InlineResult* inlineResult) + InlineResult* inlineResult, + UINT8 gdvCandidateId) { // Either EE or JIT might throw exceptions below. // If that happens, just don't inline the method. @@ -19370,6 +19374,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd; InlineResult* result; InlineCandidateInfo** ppInlineCandidateInfo; + UINT8 gdvCandidateId; } param; memset(¶m, 0, sizeof(param)); @@ -19380,6 +19385,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, param.exactContextHnd = (exactContextHnd != nullptr) ? exactContextHnd : MAKE_METHODCONTEXT(fncHandle); param.result = inlineResult; param.ppInlineCandidateInfo = ppInlineCandidateInfo; + param.gdvCandidateId = gdvCandidateId; bool success = eeRunWithErrorTrap( [](Param* pParam) { @@ -19505,7 +19511,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, if (pParam->call->IsGuardedDevirtualizationCandidate()) { - pInfo = pParam->call->gtInlineCandidateInfo; + pInfo = &pParam->call->gtInlineCandidateInfo[pParam->gdvCandidateId]; } else { @@ -20504,8 +20510,12 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { GenTreeCall* call = callNode->AsCall(); - // Do the actual evaluation - impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo); + const UINT8 candidates = call->IsGuardedDevirtualizationCandidate() ? call->gtGDVCandidatesCount : 1; + for (UINT8 candidateId = 0; candidateId < candidates; candidateId++) + { + // Do the actual evaluation + impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo); + } // If this call is an inline candidate or is not a guarded devirtualization // candidate, we're done. @@ -20559,6 +20569,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, + UINT8 candidateIndex, CORINFO_CALL_INFO* callInfo) { // Let the strategy know there's another call @@ -20652,6 +20663,9 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, return; } + assert(max(1, call->gtGDVCandidatesCount) > candidateIndex); + InlineCandidateInfo* inlineInfo = call->gtInlineCandidateInfo; + /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less * restricts the inliner to non-expanding inlines. I removed the check to allow for non-expanding * inlining in throw blocks. I should consider the same thing for catch and filter regions. */ @@ -20661,14 +20675,14 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - // TODO: there can be multiple candidates - if (call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr) + inlineInfo = &call->gtInlineCandidateInfo[candidateIndex]; + if (inlineInfo->guardedMethodUnboxedEntryHandle != nullptr) { - fncHandle = call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle; + fncHandle = inlineInfo->guardedMethodUnboxedEntryHandle; } else { - fncHandle = call->gtInlineCandidateInfo->guardedMethodHandle; + fncHandle = inlineInfo->guardedMethodHandle; } methAttr = info.compCompHnd->getMethodAttribs(fncHandle); } @@ -20761,7 +20775,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } InlineCandidateInfo* inlineCandidateInfo = nullptr; - impCheckCanInline(call, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, &inlineResult); + impCheckCanInline(call, fncHandle, methAttr, exactContextHnd, &inlineCandidateInfo, &inlineResult, candidateIndex); if (inlineResult.IsFailure()) { @@ -20769,12 +20783,11 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } // The old value should be null OR this call should be a guarded devirtualization candidate. - assert((call->gtInlineCandidateInfo == nullptr) || call->IsGuardedDevirtualizationCandidate()); + assert((inlineInfo == nullptr) || call->IsGuardedDevirtualizationCandidate()); // The new value should not be null. assert(inlineCandidateInfo != nullptr); inlineCandidateInfo->exactContextNeedsRuntimeLookup = exactContextNeedsRuntimeLookup; - call->gtInlineCandidateInfo = 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. @@ -20787,6 +20800,16 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, inlineCandidateInfo->preexistingSpillTemp); } + if (call->IsGuardedDevirtualizationCandidate()) + { + assert(call->gtInlineCandidateInfo != nullptr); + call->gtInlineCandidateInfo[candidateIndex] = *inlineCandidateInfo; + } + else + { + call->gtInlineCandidateInfo = inlineCandidateInfo; + } + // Mark the call node as inline candidate. call->gtFlags |= GTF_CALL_INLINE_CANDIDATE; @@ -21312,7 +21335,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // Clear the inline candidate info (may be non-null since // it's a union field used for other things by virtual // stubs) - call->gtInlineCandidateInfo = nullptr; + call->ClearInlineInfo(); #if defined(DEBUG) if (verbose) @@ -22005,11 +22028,15 @@ void Compiler::considerGuardedDevirtualization( { break; } - if ((info.compCompHnd->getClassAttribs(likelyClasses[i].clsHandle) & CORINFO_FLG_VALUECLASS) != 0) { break; } + if (!call->TypeIs(TYP_VOID)) + { + // something is messed up with ret_expr currently + break; + } } // Add this as a potential candidate. diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 5cfa7a1581b30b..de68dd63c6c30e 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -661,83 +661,88 @@ 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->gtInlineCandidateInfo; - GenTree* const retExpr = inlineInfo->retExpr; - // Sanity check the ret expr if non-null: it should refer to the original call. - if (retExpr != nullptr) + assert(origCall->gtGDVCandidatesCount > 0); + for (UINT8 candidateId = 0; candidateId < origCall->gtGDVCandidatesCount; candidateId++) { - assert(retExpr->AsRetExpr()->gtInlineCandidate == origCall); - } + InlineCandidateInfo* const inlineInfo = &origCall->gtInlineCandidateInfo[candidateId]; + GenTree* const retExpr = inlineInfo->retExpr; - if (origCall->TypeGet() != TYP_VOID) - { - // If there's a spill temp already associated with this inline candidate, - // use that instead of allocating a new temp. - // - returnTemp = inlineInfo->preexistingSpillTemp; - - if (returnTemp != BAD_VAR_NUM) + // Sanity check the ret expr if non-null: it should refer to the original call. + if (retExpr != nullptr) { - JITDUMP("Reworking call(s) to return value via a existing return temp V%02u\n", returnTemp); + assert(retExpr->AsRetExpr()->gtInlineCandidate == origCall); + } - // We will be introducing multiple defs for this temp, so make sure - // it is no longer marked as single def. - // - // Otherwise, we could make an incorrect type deduction. Say the - // original call site returns a B, but after we devirtualize along the - // GDV happy path we see that method returns a D. We can't then assume that - // the return temp is type D, because we don't know what type the fallback - // path returns. So we have to stick with the current type for B as the - // return type. - // - // Note local vars always live in the root method's symbol table. So we - // need to use the root compiler for lookup here. + if (origCall->TypeGet() != TYP_VOID) + { + // If there's a spill temp already associated with this inline candidate, + // use that instead of allocating a new temp. // - LclVarDsc* const returnTempLcl = compiler->impInlineRoot()->lvaGetDesc(returnTemp); + returnTemp = inlineInfo->preexistingSpillTemp; - if (returnTempLcl->lvSingleDef == 1) + if (returnTemp != BAD_VAR_NUM) { - // In this case it's ok if we already updated the type assuming single def, - // we just don't want any further updates. + JITDUMP("Reworking call(s) to return value via a existing return temp V%02u\n", returnTemp); + + // We will be introducing multiple defs for this temp, so make sure + // it is no longer marked as single def. + // + // Otherwise, we could make an incorrect type deduction. Say the + // original call site returns a B, but after we devirtualize along the + // GDV happy path we see that method returns a D. We can't then assume that + // the return temp is type D, because we don't know what type the fallback + // path returns. So we have to stick with the current type for B as the + // return type. + // + // Note local vars always live in the root method's symbol table. So we + // need to use the root compiler for lookup here. // - JITDUMP("Return temp V%02u is no longer a single def temp\n", returnTemp); - returnTempLcl->lvSingleDef = 0; + LclVarDsc* const returnTempLcl = compiler->impInlineRoot()->lvaGetDesc(returnTemp); + + if (returnTempLcl->lvSingleDef == 1) + { + // In this case it's ok if we already updated the type assuming single def, + // we just don't want any further updates. + // + JITDUMP("Return temp V%02u is no longer a single def temp\n", returnTemp); + returnTempLcl->lvSingleDef = 0; + } } + else + { + returnTemp = compiler->lvaGrabTemp(false DEBUGARG("guarded devirt return temp")); + JITDUMP("Reworking call(s) to return value via a new temp V%02u\n", returnTemp); + } + + if (varTypeIsStruct(origCall)) + { + compiler->lvaSetStruct(returnTemp, origCall->gtRetClsHnd, false); + } + + GenTree* tempTree = compiler->gtNewLclvNode(returnTemp, origCall->TypeGet()); + + JITDUMP("Bashing GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(retExpr), + returnTemp); + + retExpr->ReplaceWith(tempTree, compiler); } - else + else if (retExpr != nullptr) { - returnTemp = compiler->lvaGrabTemp(false DEBUGARG("guarded devirt return temp")); - JITDUMP("Reworking call(s) to return value via a new temp V%02u\n", returnTemp); + // We still oddly produce GT_RET_EXPRs for some void + // returning calls. Just bash the ret expr to a NOP. + // + // Todo: consider bagging creation of these RET_EXPRs. The only possible + // benefit they provide is stitching back larger trees for failed inlines + // of void-returning methods. But then the calls likely sit in commas and + // the benefit of a larger tree is unclear. + JITDUMP("Bashing GT_RET_EXPR [%06u] for VOID return to NOP\n", compiler->dspTreeID(retExpr)); + retExpr->gtBashToNOP(); } - - if (varTypeIsStruct(origCall)) + else { - compiler->lvaSetStruct(returnTemp, origCall->gtRetClsHnd, false); + // We do not produce GT_RET_EXPRs for CTOR calls, so there is nothing to patch. } - - GenTree* tempTree = compiler->gtNewLclvNode(returnTemp, origCall->TypeGet()); - - JITDUMP("Bashing GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(retExpr), - returnTemp); - - retExpr->ReplaceWith(tempTree, compiler); - } - else if (retExpr != nullptr) - { - // We still oddly produce GT_RET_EXPRs for some void - // returning calls. Just bash the ret expr to a NOP. - // - // Todo: consider bagging creation of these RET_EXPRs. The only possible - // benefit they provide is stitching back larger trees for failed inlines - // of void-returning methods. But then the calls likely sit in commas and - // the benefit of a larger tree is unclear. - JITDUMP("Bashing GT_RET_EXPR [%06u] for VOID return to NOP\n", compiler->dspTreeID(retExpr)); - retExpr->gtBashToNOP(); - } - else - { - // We do not produce GT_RET_EXPRs for CTOR calls, so there is nothing to patch. } } @@ -801,10 +806,8 @@ class IndirectCallTransformer // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. // - // Also, give up on inlining secondary guess. TODO: figure out what is missing to be able to inline them. - // CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; - if ((checkIdx > 0) || ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd))) + if (((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd))) { // Demote this call to a non-inline candidate // @@ -814,7 +817,7 @@ class IndirectCallTransformer call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; if (!call->IsVirtualStub()) - call->gtInlineCandidateInfo = nullptr; + call->ClearInlineInfo(); if (returnTemp != BAD_VAR_NUM) { @@ -902,7 +905,7 @@ class IndirectCallTransformer } else { - call->gtInlineCandidateInfo = nullptr; + call->ClearInlineInfo(); } compiler->fgInsertStmtAtEnd(elseBlock, newStmt); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 407396c8e7f0d6..06ad0c14af28b3 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -64,17 +64,17 @@ GenTree* Compiler::fgMorphIntoHelperCall(GenTree* tree, int helper, GenTreeCall: GenTreeCall* call = tree->AsCall(); - call->gtCallType = CT_HELPER; - call->gtCallMethHnd = eeFindHelper(helper); - call->gtCallThisArg = nullptr; - call->gtCallArgs = args; - call->gtCallLateArgs = nullptr; - call->fgArgInfo = nullptr; - call->gtRetClsHnd = nullptr; - call->gtCallMoreFlags = GTF_CALL_M_EMPTY; - call->gtInlineCandidateInfo = nullptr; - call->gtControlExpr = nullptr; - call->gtGDVCandidatesCount = 0; + call->gtCallType = CT_HELPER; + call->gtCallMethHnd = eeFindHelper(helper); + call->gtCallThisArg = nullptr; + call->gtCallArgs = args; + call->gtCallLateArgs = nullptr; + call->fgArgInfo = nullptr; + call->gtRetClsHnd = nullptr; + call->gtCallMoreFlags = GTF_CALL_M_EMPTY; + call->gtControlExpr = nullptr; + call->gtGDVCandidatesCount = 0; + call->ClearInlineInfo(); #ifdef UNIX_X86_ABI call->gtFlags |= GTF_CALL_POP_ARGS; #endif // UNIX_X86_ABI From 07e20cc5402d288213ee54cc84dd81e868a4af36 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 2 Oct 2021 21:37:58 +0300 Subject: [PATCH 09/26] Fix non-void return type --- src/coreclr/jit/importer.cpp | 5 ----- src/coreclr/jit/indirectcalltransformer.cpp | 10 ++++++++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 6e2e672ff46c87..7720b61b8a949c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -22032,11 +22032,6 @@ void Compiler::considerGuardedDevirtualization( { break; } - if (!call->TypeIs(TYP_VOID)) - { - // something is messed up with ret_expr currently - break; - } } // Add this as a potential candidate. diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index de68dd63c6c30e..472ad9501c19c9 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -662,6 +662,8 @@ class IndirectCallTransformer // Note implicit by-ref returns should have already been converted // so any struct copy we induce here should be cheap. + returnTemp = origCall->gtInlineCandidateInfo[0].preexistingSpillTemp; + assert(origCall->gtGDVCandidatesCount > 0); for (UINT8 candidateId = 0; candidateId < origCall->gtGDVCandidatesCount; candidateId++) { @@ -679,7 +681,11 @@ class IndirectCallTransformer // If there's a spill temp already associated with this inline candidate, // use that instead of allocating a new temp. // - returnTemp = inlineInfo->preexistingSpillTemp; + + if (inlineInfo->preexistingSpillTemp != BAD_VAR_NUM) + { + returnTemp = inlineInfo->preexistingSpillTemp; + } if (returnTemp != BAD_VAR_NUM) { @@ -807,7 +813,7 @@ class IndirectCallTransformer // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. // CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; - if (((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd))) + if ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd)) { // Demote this call to a non-inline candidate // From bcf7b4f69152aea60500089704fc88e602510486 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 2 Oct 2021 21:54:16 +0300 Subject: [PATCH 10/26] fix assert --- src/coreclr/jit/indirectcalltransformer.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 472ad9501c19c9..c5319fe48a41fc 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -662,8 +662,6 @@ class IndirectCallTransformer // Note implicit by-ref returns should have already been converted // so any struct copy we induce here should be cheap. - returnTemp = origCall->gtInlineCandidateInfo[0].preexistingSpillTemp; - assert(origCall->gtGDVCandidatesCount > 0); for (UINT8 candidateId = 0; candidateId < origCall->gtGDVCandidatesCount; candidateId++) { @@ -682,7 +680,7 @@ class IndirectCallTransformer // use that instead of allocating a new temp. // - if (inlineInfo->preexistingSpillTemp != BAD_VAR_NUM) + if (returnTemp == BAD_VAR_NUM) { returnTemp = inlineInfo->preexistingSpillTemp; } @@ -784,6 +782,7 @@ class IndirectCallTransformer GenTreeCall* call = compiler->gtCloneCandidateCall(origCall); call->gtCallThisArg = compiler->gtNewCallArgs(compiler->gtNewLclvNode(thisTemp, TYP_REF)); call->SetIsGuarded(); + call->gtGDVCandidatesCount = 0; JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), thenBlock->bbNum); From 81c94af2bd8282794fb1a211c0f7f94da4e02953 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 2 Oct 2021 21:54:47 +0300 Subject: [PATCH 11/26] Disable multi-checks (for CI) --- 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 a3ad38d3d587b7..844944d63bd5b6 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -497,7 +497,7 @@ CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualiza #endif // DEBUG // Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 3) +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 1) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) From c048c643581b3597795708882b99792dfba57747 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 3 Oct 2021 15:49:51 +0300 Subject: [PATCH 12/26] fix more asserts --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/importer.cpp | 52 ++++++++++++++++++++----------- src/coreclr/jit/inline.cpp | 7 +++-- src/coreclr/jit/jitconfigvalues.h | 2 +- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 244a2256ff2122..789c1adb4e5c2b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4793,7 +4793,7 @@ class Compiler bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo); - void impMarkInlineCandidateHelper(GenTreeCall* call, + bool impMarkInlineCandidateHelper(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, UINT8 candidateIndex, diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 7720b61b8a949c..c5d5a39518dee7 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -20510,11 +20510,26 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { GenTreeCall* call = callNode->AsCall(); + bool hasNonInlineableCandidates = false; + bool firstCandidateIsNonInlineable = false; const UINT8 candidates = call->IsGuardedDevirtualizationCandidate() ? call->gtGDVCandidatesCount : 1; for (UINT8 candidateId = 0; candidateId < candidates; candidateId++) { // Do the actual evaluation - impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo); + if (!impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo)) + { + hasNonInlineableCandidates = true; + if (candidateId == 0) + { + firstCandidateIsNonInlineable = true; + } + break; + } + } + + if (hasNonInlineableCandidates && !firstCandidateIsNonInlineable) + { + call->gtGDVCandidatesCount = 1; } // If this call is an inline candidate or is not a guarded devirtualization @@ -20566,7 +20581,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // method may be marked as "noinline" to short-circuit any // future assessments of calls to this method. -void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, +bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd, bool exactContextNeedsRuntimeLookup, UINT8 candidateIndex, @@ -20584,7 +20599,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, * figure out why we did not set MAXOPT for this compile. */ assert(!compIsForInlining()); - return; + return false; } if (compIsForImportOnly()) @@ -20592,7 +20607,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, // Don't bother creating the inline candidate during verification. // Otherwise the call to info.compCompHnd->canInline will trigger a recursive verification // that leads to the creation of multiple instances of Compiler. - return; + return false; } InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); @@ -20601,21 +20616,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. @@ -20623,7 +20638,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsTailPrefixedCall()) { inlineResult.NoteFatal(InlineObservation::CALLSITE_EXPLICIT_TAIL_PREFIX); - return; + return false; } // Tail recursion elimination takes precedence over inlining. @@ -20633,7 +20648,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()) @@ -20643,7 +20658,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (!call->IsGuardedDevirtualizationCandidate()) { inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT); - return; + return false; } } @@ -20653,14 +20668,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; } assert(max(1, call->gtGDVCandidatesCount) > candidateIndex); @@ -20728,7 +20743,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, #endif inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_CATCH); - return; + return false; } if (bbInFilterILRange(compCurBB)) @@ -20741,7 +20756,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, #endif inlineResult.NoteFatal(InlineObservation::CALLSITE_IS_WITHIN_FILTER); - return; + return false; } } @@ -20750,7 +20765,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 */ @@ -20758,7 +20773,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) */ @@ -20770,7 +20785,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (!impCanPInvokeInlineCallSite(block)) { inlineResult.NoteFatal(InlineObservation::CALLSITE_PINVOKE_EH); - return; + return false; } } @@ -20779,7 +20794,7 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (inlineResult.IsFailure()) { - return; + return false; } // The old value should be null OR this call should be a guarded devirtualization candidate. @@ -20819,6 +20834,7 @@ 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.SetReported(); + return true; } /******************************************************************************/ diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 4e86755491b8f0..0549de367bbac0 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -713,8 +713,11 @@ void InlineResult::Report() // a failing observation yet, do so now. if (IsFailure() && (m_Call != nullptr)) { - // compiler should have revoked candidacy on the call by now - assert((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0); + if (m_Call->gtGDVCandidatesCount < 2) + { + // compiler should have revoked candidacy on the call by now + assert((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0); + } if (m_Call->gtInlineObservation == InlineObservation::CALLEE_UNUSED_INITIAL) { diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 844944d63bd5b6..5578fecc199612 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -497,7 +497,7 @@ CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualiza #endif // DEBUG // Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 1) +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 2) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) From 37ca01fb2f6d46da2ea6adf79e4fb32dd33c8895 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 3 Oct 2021 19:19:38 +0300 Subject: [PATCH 13/26] Fix all existing issues --- src/coreclr/jit/fginline.cpp | 2 +- src/coreclr/jit/gentree.cpp | 9 ++- src/coreclr/jit/gentree.h | 15 ++++- src/coreclr/jit/importer.cpp | 64 ++++++++++----------- src/coreclr/jit/indirectcalltransformer.cpp | 12 ++-- src/coreclr/jit/inline.cpp | 2 +- src/coreclr/jit/jitconfigvalues.h | 2 +- src/coreclr/jit/morph.cpp | 1 - 8 files changed, 58 insertions(+), 49 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 4e9dd742a4661c..0cbcb145fe7f0e 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -888,7 +888,7 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe inlineInfo.hasSIMDTypeArgLocalOrReturn = false; #endif // FEATURE_SIMD - assert(call->gtGDVCandidatesCount <= 1); + assert(call->GetGDVCandidatesCount() <= 1); InlineCandidateInfo* inlineCandidateInfo = call->gtInlineCandidateInfo; noway_assert(inlineCandidateInfo); // Store the link to inlineCandidateInfo into inlineInfo diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ac0b51c1d662b7..3a9ec27df4e6b8 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6415,9 +6415,9 @@ GenTreeCall* Compiler::gtNewCallNode( node->gtRetClsHnd = nullptr; node->gtControlExpr = nullptr; node->gtCallMoreFlags = GTF_CALL_M_EMPTY; - node->gtCallLateArgs = nullptr; - node->gtReturnType = type; - node->gtGDVCandidatesCount = 0; + node->gtCallLateArgs = nullptr; + node->gtReturnType = type; + node->SetGDVCandidatesCount(0); if (callType == CT_INDIRECT) { @@ -8309,8 +8309,7 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree, } else { - copy->gtCallMethHnd = tree->gtCallMethHnd; - copy->gtGDVCandidatesCount = 0; + copy->gtCallMethHnd = tree->gtCallMethHnd; copy->ClearInlineInfo(); } diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0f7808fc6e1873..df092a6461b724 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4709,6 +4709,7 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { + SetGDVCandidatesCount(0); gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; } @@ -4774,7 +4775,7 @@ struct GenTreeCall final : public GenTree void ClearInlineInfo() { - //assert(gtGDVCandidatesCount <= 1); + SetGDVCandidatesCount(0); gtInlineCandidateInfo = nullptr; } @@ -4802,6 +4803,18 @@ struct GenTreeCall final : public GenTree UINT8 gtGDVCandidatesCount; + UINT8 GetGDVCandidatesCount() const + { + const bool isGDV = IsGuardedDevirtualizationCandidate(); + assert(((gtGDVCandidatesCount == 0) && !isGDV) ^ (isGDV && (gtGDVCandidatesCount > 0))); + return gtGDVCandidatesCount; + } + + void SetGDVCandidatesCount(UINT8 count) + { + gtGDVCandidatesCount = count; + } + bool IsHelperCall() const { return gtCallType == CT_HELPER; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 1f4a0e9f0c605c..b6a757fb33435c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9557,7 +9557,7 @@ var_types Compiler::impImportCall(OPCODE opcode, // Make the call its own tree (spill the stack if needed). impAppendTree(call, (unsigned)CHECK_SPILL_ALL, impCurStmtOffs); - for (UINT8 i = 0; i < max(1, origCall->gtGDVCandidatesCount); i++) + for (UINT8 i = 0; i < max(1, origCall->GetGDVCandidatesCount()); i++) { // TODO: Still using the widened type. GenTree* retExpr = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); @@ -20517,26 +20517,24 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { GenTreeCall* call = callNode->AsCall(); - bool hasNonInlineableCandidates = false; - bool firstCandidateIsNonInlineable = false; - const UINT8 candidates = call->IsGuardedDevirtualizationCandidate() ? call->gtGDVCandidatesCount : 1; - for (UINT8 candidateId = 0; candidateId < candidates; candidateId++) + if (call->IsGuardedDevirtualizationCandidate()) { - // Do the actual evaluation - if (!impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo)) + UINT8 inlineableGdvCount = 0; + for (UINT8 candidateId = 0; candidateId < call->GetGDVCandidatesCount(); candidateId++) { - hasNonInlineableCandidates = true; - if (candidateId == 0) + if (!impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo)) { - firstCandidateIsNonInlineable = true; + // TODO: remove only non-inlineable candidates here. + // Currently we just ignore all candidates after first non-inlineable one + break; } - break; + inlineableGdvCount++; } + call->SetGDVCandidatesCount(inlineableGdvCount); } - - if (hasNonInlineableCandidates && !firstCandidateIsNonInlineable) + else { - call->gtGDVCandidatesCount = 1; + impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, 0, callInfo); } // If this call is an inline candidate or is not a guarded devirtualization @@ -20685,7 +20683,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, return false; } - assert(max(1, call->gtGDVCandidatesCount) > candidateIndex); + assert(max(1, call->GetGDVCandidatesCount()) > candidateIndex); InlineCandidateInfo* inlineInfo = call->gtInlineCandidateInfo; /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less @@ -21998,11 +21996,13 @@ void Compiler::considerGuardedDevirtualization( eeGetClassName(likelyClasses[i].clsHandle), likelyClasses[i].likelihood); } - assert(call->gtGDVCandidatesCount == 0); + assert(call->GetGDVCandidatesCount() == 0); + + INDEBUG(CORINFO_SIG_INFO prevSig); for (UINT i = 0; i < numberOfClasses; i++) { - if (call->gtGDVCandidatesCount >= (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount()) + if (call->GetGDVCandidatesCount() >= (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount()) { break; } @@ -22042,20 +22042,17 @@ void Compiler::considerGuardedDevirtualization( CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); +#ifdef DEBUG + CORINFO_SIG_INFO sig; + info.compCompHnd->getMethodSig(likelyMethod, &sig); if (i > 0) { - // TODO: skip valuetypes and generics for secondary guesses for now - CORINFO_SIG_INFO sig; - info.compCompHnd->getMethodSig(likelyMethod, &sig); - if ((sig.sigInst.methInstCount != 0) || (sig.sigInst.classInstCount != 0)) - { - break; - } - if ((info.compCompHnd->getClassAttribs(likelyClasses[i].clsHandle) & CORINFO_FLG_VALUECLASS) != 0) - { - break; - } + assert(sig.retType != prevSig.retType); + assert(sig.retTypeClass != prevSig.retTypeClass); + assert(sig.retTypeSigClass != prevSig.retTypeSigClass); } + prevSig = sig; +#endif // Add this as a potential candidate. // @@ -22190,15 +22187,16 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, } const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); - const UINT8 candidatesCount = ++call->gtGDVCandidatesCount; - assert(maxCandidates >= candidatesCount); + call->SetGDVCandidatesCount(call->GetGDVCandidatesCount() + 1); + assert(maxCandidates >= call->GetGDVCandidatesCount()); - if (candidatesCount == 1) + auto x = info.compFullName; + if (call->GetGDVCandidatesCount() == 1) { // in 90% cases virtual calls are monomorphic so let's avoid allocating too much call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1]{pInfo}; } - else if (candidatesCount == 2) + else if (call->GetGDVCandidatesCount() == 2) { // Ok, re-alloc to store multiple candidates. InlineCandidateInfo firstInfo = call->gtInlineCandidateInfo[0]; @@ -22207,7 +22205,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, } else { - call->gtInlineCandidateInfo[candidatesCount - 1] = pInfo; + call->gtInlineCandidateInfo[call->GetGDVCandidatesCount() - 1] = pInfo; } } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index c5319fe48a41fc..7c9f0509d0022e 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -662,8 +662,8 @@ class IndirectCallTransformer // Note implicit by-ref returns should have already been converted // so any struct copy we induce here should be cheap. - assert(origCall->gtGDVCandidatesCount > 0); - for (UINT8 candidateId = 0; candidateId < origCall->gtGDVCandidatesCount; candidateId++) + assert(origCall->GetGDVCandidatesCount() > 0); + for (UINT8 candidateId = 0; candidateId < origCall->GetGDVCandidatesCount(); candidateId++) { InlineCandidateInfo* const inlineInfo = &origCall->gtInlineCandidateInfo[candidateId]; GenTree* const retExpr = inlineInfo->retExpr; @@ -755,7 +755,7 @@ class IndirectCallTransformer // virtual UINT8 GetChecksCount() override { - return origCall->gtGDVCandidatesCount; + return origCall->GetGDVCandidatesCount(); } //------------------------------------------------------------------------ @@ -782,7 +782,7 @@ class IndirectCallTransformer GenTreeCall* call = compiler->gtCloneCandidateCall(origCall); call->gtCallThisArg = compiler->gtNewCallArgs(compiler->gtNewLclvNode(thisTemp, TYP_REF)); call->SetIsGuarded(); - call->gtGDVCandidatesCount = 0; + call->SetGDVCandidatesCount(0); JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), thenBlock->bbNum); @@ -895,7 +895,7 @@ class IndirectCallTransformer } UINT32 elseLikelihood = 100; - for (UINT8 i = 0; i < origCall->gtGDVCandidatesCount; i++) + for (UINT8 i = 0; i < origCall->GetGDVCandidatesCount(); i++) { elseLikelihood -= origCall->gtInlineCandidateInfo[i].likelihood; } @@ -1088,7 +1088,7 @@ class IndirectCallTransformer { GenTreeCall* const call = root->AsCall(); - if (call->IsGuardedDevirtualizationCandidate() && (call->gtGDVCandidatesCount == 1) && + if (call->IsGuardedDevirtualizationCandidate() && (call->GetGDVCandidatesCount() == 1) && (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 0549de367bbac0..4bee4c4b14c897 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -713,7 +713,7 @@ void InlineResult::Report() // a failing observation yet, do so now. if (IsFailure() && (m_Call != nullptr)) { - if (m_Call->gtGDVCandidatesCount < 2) + if (m_Call->GetGDVCandidatesCount() < 2) { // compiler should have revoked candidacy on the call by now assert((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 2873bcfa0d922f..65ce042d647ef7 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -499,7 +499,7 @@ CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualiza #endif // DEBUG // Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 2) +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 4) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0f4171e5a25608..e19c804806dfaf 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -73,7 +73,6 @@ GenTree* Compiler::fgMorphIntoHelperCall(GenTree* tree, int helper, GenTreeCall: call->gtRetClsHnd = nullptr; call->gtCallMoreFlags = GTF_CALL_M_EMPTY; call->gtControlExpr = nullptr; - call->gtGDVCandidatesCount = 0; call->ClearInlineInfo(); #ifdef UNIX_X86_ABI call->gtFlags |= GTF_CALL_POP_ARGS; From 756dfdb85379147c8f0fe20ece3382a09d8fef99 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 3 Oct 2021 21:21:09 +0300 Subject: [PATCH 14/26] disable non-void methods for now --- src/coreclr/jit/gentree.h | 3 --- src/coreclr/jit/importer.cpp | 24 +++++++++++++++------ src/coreclr/jit/indirectcalltransformer.cpp | 1 + 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index df092a6461b724..6c03a594a27d33 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4709,7 +4709,6 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { - SetGDVCandidatesCount(0); gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; } @@ -4805,8 +4804,6 @@ struct GenTreeCall final : public GenTree UINT8 GetGDVCandidatesCount() const { - const bool isGDV = IsGuardedDevirtualizationCandidate(); - assert(((gtGDVCandidatesCount == 0) && !isGDV) ^ (isGDV && (gtGDVCandidatesCount > 0))); return gtGDVCandidatesCount; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b6a757fb33435c..dfb7dd629c3371 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -6909,6 +6909,7 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) { JITDUMP("Disabling GDV for [%06u] because of in-box struct return\n"); call->ClearGuardedDevirtualizationCandidate(); + call->SetGDVCandidatesCount(0); if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", @@ -20555,6 +20556,7 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, dspTreeID(call)); call->ClearGuardedDevirtualizationCandidate(); + call->SetGDVCandidatesCount(0); // If we have a stub address, restore it back into the union that it shares // with the candidate info. @@ -21998,7 +22000,7 @@ void Compiler::considerGuardedDevirtualization( assert(call->GetGDVCandidatesCount() == 0); - INDEBUG(CORINFO_SIG_INFO prevSig); + INDEBUG(CORINFO_SIG_INFO prevSig = {}); for (UINT i = 0; i < numberOfClasses; i++) { @@ -22039,17 +22041,26 @@ void Compiler::considerGuardedDevirtualization( continue; } + if ((call->GetGDVCandidatesCount() > 0) && !call->TypeIs(TYP_VOID)) + { + // TODO: there is an issue somewhere for secondary guesses + // with non-void return type + break; + } + CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); #ifdef DEBUG CORINFO_SIG_INFO sig; info.compCompHnd->getMethodSig(likelyMethod, &sig); - if (i > 0) + if (call->GetGDVCandidatesCount() > 0) { - assert(sig.retType != prevSig.retType); - assert(sig.retTypeClass != prevSig.retTypeClass); - assert(sig.retTypeSigClass != prevSig.retTypeSigClass); + CORINFO_SIG_INFO sig; + info.compCompHnd->getMethodSig(likelyMethod, &sig); + assert(sig.retType == prevSig.retType); + assert(sig.retTypeClass == prevSig.retTypeClass); + assert(sig.retTypeSigClass == prevSig.retTypeSigClass); } prevSig = sig; #endif @@ -22188,9 +22199,8 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); call->SetGDVCandidatesCount(call->GetGDVCandidatesCount() + 1); - assert(maxCandidates >= call->GetGDVCandidatesCount()); + assert(call->GetGDVCandidatesCount() <= maxCandidates); - auto x = info.compFullName; if (call->GetGDVCandidatesCount() == 1) { // in 90% cases virtual calls are monomorphic so let's avoid allocating too much diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 7c9f0509d0022e..cacc224143dbbe 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -209,6 +209,7 @@ class IndirectCallTransformer FixupRetExpr(); ClearFlag(); CreateRemainder(); + assert(GetChecksCount() > 0); for (UINT8 i = 0; i < GetChecksCount(); i++) { CreateCheck(i); From a76b01b959884844c84e0c60bb990615ae4a1ff3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 3 Oct 2021 22:32:02 +0300 Subject: [PATCH 15/26] Disable GDV-Chain if multi-check is enabled --- src/coreclr/jit/importer.cpp | 21 --------------------- src/coreclr/jit/indirectcalltransformer.cpp | 4 +++- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index dfb7dd629c3371..071e54f7250f1f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -22041,30 +22041,9 @@ void Compiler::considerGuardedDevirtualization( continue; } - if ((call->GetGDVCandidatesCount() > 0) && !call->TypeIs(TYP_VOID)) - { - // TODO: there is an issue somewhere for secondary guesses - // with non-void return type - break; - } - CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod; JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr)); -#ifdef DEBUG - CORINFO_SIG_INFO sig; - info.compCompHnd->getMethodSig(likelyMethod, &sig); - if (call->GetGDVCandidatesCount() > 0) - { - CORINFO_SIG_INFO sig; - info.compCompHnd->getMethodSig(likelyMethod, &sig); - assert(sig.retType == prevSig.retType); - assert(sig.retTypeClass == prevSig.retTypeClass); - assert(sig.retTypeSigClass == prevSig.retTypeSigClass); - } - prevSig = sig; -#endif - // Add this as a potential candidate. // addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle, diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index cacc224143dbbe..e85fc0fc41649b 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -1089,7 +1089,9 @@ class IndirectCallTransformer { GenTreeCall* const call = root->AsCall(); - if (call->IsGuardedDevirtualizationCandidate() && (call->GetGDVCandidatesCount() == 1) && + if (call->IsGuardedDevirtualizationCandidate() && + // TODO: for some reason multi-check GDV doesn't work well with GDV-Chain-opt + (JitConfig.JitGuardedDevirtualizationCheckCount() == 1) && (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", From 80c3399086b47c441526cfa6024f4d7ec0a20931 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 3 Oct 2021 22:36:18 +0300 Subject: [PATCH 16/26] Formatting --- src/coreclr/jit/importer.cpp | 10 ++++++---- src/coreclr/jit/morph.cpp | 18 +++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 071e54f7250f1f..8e55c737341876 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9561,7 +9561,8 @@ var_types Compiler::impImportCall(OPCODE opcode, for (UINT8 i = 0; i < max(1, origCall->GetGDVCandidatesCount()); i++) { // TODO: Still using the widened type. - GenTree* retExpr = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); + GenTree* retExpr = + gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); // Link the retExpr to the call so if necessary we can manipulate it later. InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[i]; @@ -20523,7 +20524,8 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, UINT8 inlineableGdvCount = 0; for (UINT8 candidateId = 0; candidateId < call->GetGDVCandidatesCount(); candidateId++) { - if (!impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, callInfo)) + if (!impMarkInlineCandidateHelper(call, exactContextHnd, exactContextNeedsRuntimeLookup, candidateId, + callInfo)) { // TODO: remove only non-inlineable candidates here. // Currently we just ignore all candidates after first non-inlineable one @@ -20687,7 +20689,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, assert(max(1, call->GetGDVCandidatesCount()) > candidateIndex); InlineCandidateInfo* inlineInfo = call->gtInlineCandidateInfo; - + /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less * restricts the inliner to non-expanding inlines. I removed the check to allow for non-expanding * inlining in throw blocks. I should consider the same thing for catch and filter regions. */ @@ -22176,7 +22178,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, pInfo.stubAddr = nullptr; } - const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); + const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); call->SetGDVCandidatesCount(call->GetGDVCandidatesCount() + 1); assert(call->GetGDVCandidatesCount() <= maxCandidates); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e19c804806dfaf..461faecbe87424 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -64,15 +64,15 @@ GenTree* Compiler::fgMorphIntoHelperCall(GenTree* tree, int helper, GenTreeCall: GenTreeCall* call = tree->AsCall(); - call->gtCallType = CT_HELPER; - call->gtCallMethHnd = eeFindHelper(helper); - call->gtCallThisArg = nullptr; - call->gtCallArgs = args; - call->gtCallLateArgs = nullptr; - call->fgArgInfo = nullptr; - call->gtRetClsHnd = nullptr; - call->gtCallMoreFlags = GTF_CALL_M_EMPTY; - call->gtControlExpr = nullptr; + call->gtCallType = CT_HELPER; + call->gtCallMethHnd = eeFindHelper(helper); + call->gtCallThisArg = nullptr; + call->gtCallArgs = args; + call->gtCallLateArgs = nullptr; + call->fgArgInfo = nullptr; + call->gtRetClsHnd = nullptr; + call->gtCallMoreFlags = GTF_CALL_M_EMPTY; + call->gtControlExpr = nullptr; call->ClearInlineInfo(); #ifdef UNIX_X86_ABI call->gtFlags |= GTF_CALL_POP_ARGS; From b9a1a0573862378b27e6586df4a751306362d3f1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Oct 2021 00:45:20 +0300 Subject: [PATCH 17/26] Clean up --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/compmemkind.h | 1 - src/coreclr/jit/fginline.cpp | 6 +- src/coreclr/jit/gentree.cpp | 78 +++++++++++++++++++-- src/coreclr/jit/gentree.h | 6 ++ src/coreclr/jit/importer.cpp | 54 +++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 26 +++---- 7 files changed, 115 insertions(+), 58 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 078b3de401218d..75ec6bce3cf5bd 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4836,7 +4836,7 @@ class Compiler CORINFO_CONTEXT_HANDLE exactContextHnd, InlineCandidateInfo** ppInlineCandidateInfo, InlineResult* inlineResult, - UINT8 gdvCandidateId); + UINT8 candidateId); void impInlineRecordArgInfo(InlineInfo* pInlineInfo, GenTree* curArgVal, diff --git a/src/coreclr/jit/compmemkind.h b/src/coreclr/jit/compmemkind.h index b70b2d0d57bb75..969b0c3dc2a7b5 100644 --- a/src/coreclr/jit/compmemkind.h +++ b/src/coreclr/jit/compmemkind.h @@ -60,7 +60,6 @@ CompMemKindMacro(TailMergeThrows) CompMemKindMacro(EarlyProp) CompMemKindMacro(ZeroInit) CompMemKindMacro(Pgo) -CompMemKindMacro(GDVCandidates) //clang-format on #undef CompMemKindMacro diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 0cbcb145fe7f0e..f3677a504d4f0f 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -889,7 +889,7 @@ void Compiler::fgInvokeInlineeCompiler(GenTreeCall* call, InlineResult* inlineRe #endif // FEATURE_SIMD assert(call->GetGDVCandidatesCount() <= 1); - InlineCandidateInfo* inlineCandidateInfo = call->gtInlineCandidateInfo; + InlineCandidateInfo* inlineCandidateInfo = call->GetInlineCandidateInfo(); noway_assert(inlineCandidateInfo); // Store the link to inlineCandidateInfo into inlineInfo inlineInfo.inlineCandidateInfo = inlineCandidateInfo; @@ -1431,10 +1431,10 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) // but may still be referenced from a GT_RET_EXPR node. We will replace GT_RET_EXPR node // in fgUpdateInlineReturnExpressionPlaceHolder. At that time we will also update the flags // on the basic block of GT_RET_EXPR node. - if (iciCall->gtInlineCandidateInfo->retExpr->OperGet() == GT_RET_EXPR) + if (iciCall->GetInlineCandidateInfo()->retExpr->OperGet() == GT_RET_EXPR) { // Save the basic block flags from the retExpr basic block. - iciCall->gtInlineCandidateInfo->retExpr->AsRetExpr()->bbFlags = pInlineInfo->retBB->bbFlags; + iciCall->GetInlineCandidateInfo()->retExpr->AsRetExpr()->bbFlags = pInlineInfo->retBB->bbFlags; } iciCall->ReplaceWith(pInlineInfo->retExpr, this); } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3a9ec27df4e6b8..46c80956230a24 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -1043,6 +1043,76 @@ bool GenTreeCall::IsHelperCall(Compiler* compiler, unsigned helper) const return IsHelperCall(compiler->eeFindHelper(helper)); } +//------------------------------------------------------------------------- +// SetSingleInlineCandidate: Sets (copies) inline info to the current call. +// +// Arguments: +// compiler - the compiler instance for allocations +// info - inline candidate info to set +// +// Return Value: +// Returns pointer to the copied inline info +// +InlineCandidateInfo* GenTreeCall::SetSingleInlineCandidate(Compiler* comp, const InlineCandidateInfo* info) +{ + assert(info != nullptr); + assert(GetGDVCandidatesCount() == 0); + gtInlineCandidateInfo = new (comp, CMK_Inlining) InlineCandidateInfo[1]{*info}; + return gtInlineCandidateInfo; +} + +//------------------------------------------------------------------------- +// AddGDVInlineCandidate: Adds (copies) given inline candidate info to the list +// of GDV candidates. +// +// Arguments: +// compiler - the compiler instance for allocations +// info - inline candidate info to add +// +// Return Value: +// Returns pointer to the copied inline info +// +InlineCandidateInfo* GenTreeCall::AddGDVInlineCandidate(Compiler* comp, const InlineCandidateInfo* info) +{ + assert(IsGuardedDevirtualizationCandidate()); + const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); + if (GetGDVCandidatesCount() == 0) + { + // In >90% cases we deal with monomorphic calls so let's avoid allocating too much. + SetSingleInlineCandidate(comp, info); + SetGDVCandidatesCount(1); + } + else if (GetGDVCandidatesCount() == 1) + { + // Non-monomorphic case - re-alloc + const InlineCandidateInfo firstInfo = gtInlineCandidateInfo[0]; + gtInlineCandidateInfo = new (comp, CMK_Inlining) InlineCandidateInfo[maxCandidates]{firstInfo, *info}; + SetGDVCandidatesCount(2); + } + else + { + gtInlineCandidateInfo[GetGDVCandidatesCount()] = *info; + SetGDVCandidatesCount(GetGDVCandidatesCount() + 1); + } + assert(GetGDVCandidatesCount() <= maxCandidates); + return GetInlineCandidateInfo(GetGDVCandidatesCount() - 1); +} + +//------------------------------------------------------------------------- +// GetInlineCandidateInfo: Returns an inline candidate for given index. +// +// Arguments: +// index - 0 or index of a candidate for GDV +// +// Return Value: +// Returns an inline candidate for given index. +// +InlineCandidateInfo* GenTreeCall::GetInlineCandidateInfo(UINT8 index) const +{ + assert(max(1, GetGDVCandidatesCount()) > index); + return gtInlineCandidateInfo + index; +} + //------------------------------------------------------------------------ // GenTreeCall::ReplaceCallOperand: // Replaces a given operand to a call node and updates the call @@ -12123,10 +12193,10 @@ void Compiler::gtDispTree(GenTree* tree, printf(" (FramesRoot last use)"); } - if (((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) && (call->gtInlineCandidateInfo != nullptr) && - (call->gtInlineCandidateInfo->exactContextHnd != nullptr)) + if (((call->gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0) && (call->GetInlineCandidateInfo() != nullptr) && + (call->GetInlineCandidateInfo()->exactContextHnd != nullptr)) { - printf(" (exactContextHnd=0x%p)", dspPtr(call->gtInlineCandidateInfo->exactContextHnd)); + printf(" (exactContextHnd=0x%p)", dspPtr(call->GetInlineCandidateInfo()->exactContextHnd)); } gtDispCommonEndLine(tree); @@ -17775,7 +17845,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b { // For inline candidates, we've already cached the return // type class handle in the inline info. - InlineCandidateInfo* inlInfo = call->gtInlineCandidateInfo; + InlineCandidateInfo* inlInfo = call->GetInlineCandidateInfo(); 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 6c03a594a27d33..6d159fcfe55ade 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4778,6 +4778,12 @@ struct GenTreeCall final : public GenTree gtInlineCandidateInfo = nullptr; } + InlineCandidateInfo* SetSingleInlineCandidate(Compiler* comp, const InlineCandidateInfo* info); + + InlineCandidateInfo* AddGDVInlineCandidate(Compiler* comp, const InlineCandidateInfo* info); + + InlineCandidateInfo* GetInlineCandidateInfo(UINT8 index = 0) const; + // expression evaluated after args are placed which determines the control target GenTree* gtControlExpr; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8e55c737341876..ff16e558eb767f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -2528,7 +2528,7 @@ bool Compiler::impSpillStackEntry(unsigned level, { JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); GenTree* call = tree->AsRetExpr()->gtInlineCandidate; - InlineCandidateInfo* ici = call->AsCall()->gtInlineCandidateInfo; + InlineCandidateInfo* ici = call->AsCall()->GetInlineCandidateInfo(); ici->preexistingSpillTemp = tnum; } } @@ -6913,8 +6913,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", - dspPtr(call->gtInlineCandidateInfo->stubAddr)); - call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; + dspPtr(call->GetInlineCandidateInfo()->stubAddr)); + call->gtStubCallStubAddr = call->GetInlineCandidateInfo()->stubAddr; } } } @@ -9565,11 +9565,11 @@ var_types Compiler::impImportCall(OPCODE opcode, gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); // Link the retExpr to the call so if necessary we can manipulate it later. - InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[i]; + InlineCandidateInfo* inlineInfo = origCall->GetInlineCandidateInfo(i); inlineInfo->retExpr = retExpr; } // Propagate retExpr as the placeholder for the call. - call = origCall->gtInlineCandidateInfo[0].retExpr; + call = origCall->GetInlineCandidateInfo()->retExpr; } else { @@ -19369,7 +19369,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd, InlineCandidateInfo** ppInlineCandidateInfo, InlineResult* inlineResult, - UINT8 gdvCandidateId) + UINT8 candidateId) { // Either EE or JIT might throw exceptions below. // If that happens, just don't inline the method. @@ -19383,7 +19383,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContextHnd; InlineResult* result; InlineCandidateInfo** ppInlineCandidateInfo; - UINT8 gdvCandidateId; + UINT8 candidateId; } param; memset(¶m, 0, sizeof(param)); @@ -19394,7 +19394,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, param.exactContextHnd = (exactContextHnd != nullptr) ? exactContextHnd : MAKE_METHODCONTEXT(fncHandle); param.result = inlineResult; param.ppInlineCandidateInfo = ppInlineCandidateInfo; - param.gdvCandidateId = gdvCandidateId; + param.candidateId = candidateId; bool success = eeRunWithErrorTrap( [](Param* pParam) { @@ -19520,7 +19520,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, if (pParam->call->IsGuardedDevirtualizationCandidate()) { - pInfo = &pParam->call->gtInlineCandidateInfo[pParam->gdvCandidateId]; + pInfo = pParam->call->GetInlineCandidateInfo(pParam->candidateId); } else { @@ -20565,8 +20565,8 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", - dspPtr(call->gtInlineCandidateInfo->stubAddr)); - call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; + dspPtr(call->GetInlineCandidateInfo()->stubAddr)); + call->gtStubCallStubAddr = call->GetInlineCandidateInfo()->stubAddr; } } @@ -20688,7 +20688,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } assert(max(1, call->GetGDVCandidatesCount()) > candidateIndex); - InlineCandidateInfo* inlineInfo = call->gtInlineCandidateInfo; + InlineCandidateInfo* inlineInfo = call->GetInlineCandidateInfo(); /* I removed the check for BBJ_THROW. BBJ_THROW is usually marked as rarely run. This more or less * restricts the inliner to non-expanding inlines. I removed the check to allow for non-expanding @@ -20699,7 +20699,7 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - inlineInfo = &call->gtInlineCandidateInfo[candidateIndex]; + inlineInfo = call->GetInlineCandidateInfo(candidateIndex); if (inlineInfo->guardedMethodUnboxedEntryHandle != nullptr) { fncHandle = inlineInfo->guardedMethodUnboxedEntryHandle; @@ -20826,13 +20826,14 @@ bool Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - assert(call->gtInlineCandidateInfo != nullptr); - call->gtInlineCandidateInfo[candidateIndex] = *inlineCandidateInfo; + assert(call->GetInlineCandidateInfo() != nullptr); + *call->GetInlineCandidateInfo(candidateIndex) = *inlineCandidateInfo; } else { - call->gtInlineCandidateInfo = inlineCandidateInfo; + call->SetSingleInlineCandidate(this, inlineCandidateInfo); } + inlineCandidateInfo = nullptr; // Mark the call node as inline candidate. call->gtFlags |= GTF_CALL_INLINE_CANDIDATE; @@ -22178,26 +22179,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, pInfo.stubAddr = nullptr; } - const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); - call->SetGDVCandidatesCount(call->GetGDVCandidatesCount() + 1); - assert(call->GetGDVCandidatesCount() <= maxCandidates); - - if (call->GetGDVCandidatesCount() == 1) - { - // in 90% cases virtual calls are monomorphic so let's avoid allocating too much - call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1]{pInfo}; - } - else if (call->GetGDVCandidatesCount() == 2) - { - // Ok, re-alloc to store multiple candidates. - InlineCandidateInfo firstInfo = call->gtInlineCandidateInfo[0]; - call->gtInlineCandidateInfo = - new (this, CMK_GDVCandidates) InlineCandidateInfo[maxCandidates]{firstInfo, pInfo}; - } - else - { - call->gtInlineCandidateInfo[call->GetGDVCandidatesCount() - 1] = pInfo; - } + call->AddGDVInlineCandidate(this, &pInfo); } void Compiler::addExpRuntimeLookupCandidate(GenTreeCall* call) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index e85fc0fc41649b..49e0f62b71ab05 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -535,7 +535,7 @@ class IndirectCallTransformer return; } - likelihood = origCall->gtInlineCandidateInfo->likelihood; + likelihood = origCall->GetInlineCandidateInfo()->likelihood; assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); @@ -640,7 +640,7 @@ class IndirectCallTransformer // Find target method table // GenTree* methodTable = compiler->gtNewMethodTableLookup(thisTree); - CORINFO_CLASS_HANDLE clsHnd = origCall->gtInlineCandidateInfo[checkIdx].guardedClassHandle; + CORINFO_CLASS_HANDLE clsHnd = origCall->GetInlineCandidateInfo(checkIdx)->guardedClassHandle; GenTree* targetMethodTable = compiler->gtNewIconEmbClsHndNode(clsHnd); // Compare and jump to else (which does the indirect call) if NOT equal @@ -666,7 +666,7 @@ class IndirectCallTransformer assert(origCall->GetGDVCandidatesCount() > 0); for (UINT8 candidateId = 0; candidateId < origCall->GetGDVCandidatesCount(); candidateId++) { - InlineCandidateInfo* const inlineInfo = &origCall->gtInlineCandidateInfo[candidateId]; + InlineCandidateInfo* const inlineInfo = origCall->GetInlineCandidateInfo(candidateId); GenTree* const retExpr = inlineInfo->retExpr; // Sanity check the ret expr if non-null: it should refer to the original call. @@ -767,9 +767,9 @@ class IndirectCallTransformer thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock); thenBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; thenBlock->bbJumpDest = remainderBlock; - thenBlock->inheritWeightPercentage(currBlock, origCall->gtInlineCandidateInfo[checkIdx].likelihood); + thenBlock->inheritWeightPercentage(currBlock, origCall->GetInlineCandidateInfo(checkIdx)->likelihood); - InlineCandidateInfo* inlineInfo = &origCall->gtInlineCandidateInfo[checkIdx]; + InlineCandidateInfo* inlineInfo = origCall->GetInlineCandidateInfo(checkIdx); CORINFO_CLASS_HANDLE clsHnd = inlineInfo->guardedClassHandle; // copy 'this' to temp with exact type. @@ -806,7 +806,7 @@ class IndirectCallTransformer // TODO: fix assert(checkIdx > 0); call = compiler->gtCloneCandidateCall(origCall); - call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; + call->gtStubCallStubAddr = call->GetInlineCandidateInfo()->stubAddr; } // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info @@ -847,7 +847,7 @@ class IndirectCallTransformer inlineInfo->clsHandle = compiler->info.compCompHnd->getMethodClass(methodHnd); inlineInfo->exactContextHnd = context; inlineInfo->preexistingSpillTemp = returnTemp; - call->gtInlineCandidateInfo = inlineInfo; + inlineInfo = call->SetSingleInlineCandidate(compiler, inlineInfo); // If there was a ret expr for this call, we need to create a new one // and append it just after the call. @@ -898,7 +898,7 @@ class IndirectCallTransformer UINT32 elseLikelihood = 100; for (UINT8 i = 0; i < origCall->GetGDVCandidatesCount(); i++) { - elseLikelihood -= origCall->gtInlineCandidateInfo[i].likelihood; + elseLikelihood -= origCall->GetInlineCandidateInfo(i)->likelihood; } elseBlock->inheritWeightPercentage(currBlock, elseLikelihood > 100 /*overflow*/ ? 0 : elseLikelihood); @@ -906,8 +906,8 @@ class IndirectCallTransformer // null out the candidate info field. if (call->IsVirtualStub()) { - JITDUMP("Restoring stub addr %p from candidate info\n", call->gtInlineCandidateInfo->stubAddr); - call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr; + JITDUMP("Restoring stub addr %p from candidate info\n", call->GetInlineCandidateInfo()->stubAddr); + call->gtStubCallStubAddr = call->GetInlineCandidateInfo()->stubAddr; } else { @@ -1092,11 +1092,11 @@ class IndirectCallTransformer if (call->IsGuardedDevirtualizationCandidate() && // TODO: for some reason multi-check GDV doesn't work well with GDV-Chain-opt (JitConfig.JitGuardedDevirtualizationCheckCount() == 1) && - (call->gtInlineCandidateInfo->likelihood >= gdvChainLikelihood)) + (call->GetInlineCandidateInfo()->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", - compiler->dspTreeID(call), call->gtInlineCandidateInfo->likelihood, gdvChainLikelihood, - chainStatementDup, chainNodeDup); + compiler->dspTreeID(call), call->GetInlineCandidateInfo()->likelihood, + gdvChainLikelihood, chainStatementDup, chainNodeDup); call->gtCallMoreFlags |= GTF_CALL_M_GUARDED_DEVIRT_CHAIN; break; From db50163a1439d0bc56c6db6d8278463dfc374fa0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Oct 2021 00:50:48 +0300 Subject: [PATCH 18/26] Clean up --- src/coreclr/jit/inline.cpp | 4 +++- src/coreclr/jit/jitconfigvalues.h | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 4bee4c4b14c897..ed0d2de5f0b0b3 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -715,7 +715,9 @@ void InlineResult::Report() { if (m_Call->GetGDVCandidatesCount() < 2) { - // compiler should have revoked candidacy on the call by now + // Compiler should have revoked candidacy on the call by now + // However, in GDV case we might see cases where some of the + // candidates are inlineable and some are not. assert((m_Call->gtFlags & GTF_CALL_INLINE_CANDIDATE) == 0); } diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 65ce042d647ef7..0efea82200cc4c 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -498,8 +498,8 @@ CONFIG_STRING(JitGuardedDevirtualizationRange, W("JitGuardedDevirtualizationRang CONFIG_INTEGER(JitRandomGuardedDevirtualization, W("JitRandomGuardedDevirtualization"), 0) #endif // DEBUG -// Number of type checks for a virtual call -CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 4) +// Max number of type checks for a virtual call +CONFIG_INTEGER(JitGuardedDevirtualizationCheckCount, W("JitGuardedDevirtualizationCheckCount"), 3) // Enable insertion of patchpoints into Tier0 methods with loops. CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0) From 8e70304e43ca9c989c12d2d494a0a4760dc62389 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Oct 2021 11:13:22 +0300 Subject: [PATCH 19/26] enable chain opt for monomorphic calls --- src/coreclr/jit/indirectcalltransformer.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 49e0f62b71ab05..f59de1029d3f70 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -539,7 +539,10 @@ 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; + const bool isChainedGdv = ((origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0) && + (origCall->GetGDVCandidatesCount() == 1); + + // NOTE: calls with multiple GDV candidates don't support this opt yet. if (isChainedGdv) { @@ -551,11 +554,11 @@ class IndirectCallTransformer if (isChainedGdv) { TransformForChainedGdv(); - } - // Look ahead and see if there's another Gdv we might chain to this one. - // - ScoutForChainedGdv(); + // Look ahead and see if there's another Gdv we might chain to this one. + // + ScoutForChainedGdv(); + } } protected: @@ -1029,8 +1032,6 @@ class IndirectCallTransformer const unsigned maxStatementDup = JitConfig.JitGuardedDevirtualizationChainStatements(); unsigned chainStatementDup = 0; unsigned chainNodeDup = 0; - unsigned chainLikelihood = 0; - GenTreeCall* chainedCall = nullptr; // Helper class to check a statement for uncloneable nodes and count // the total number of nodes @@ -1090,8 +1091,6 @@ class IndirectCallTransformer GenTreeCall* const call = root->AsCall(); if (call->IsGuardedDevirtualizationCandidate() && - // TODO: for some reason multi-check GDV doesn't work well with GDV-Chain-opt - (JitConfig.JitGuardedDevirtualizationCheckCount() == 1) && (call->GetInlineCandidateInfo()->likelihood >= gdvChainLikelihood)) { JITDUMP("GDV call at [%06u] has likelihood %u >= %u; chaining (%u stmts, %u nodes to dup).\n", From 158b7d40121a09d4c46089fdfe9074e574f0d974 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Oct 2021 11:36:35 +0300 Subject: [PATCH 20/26] enable chain opt for monomorphic calls --- src/coreclr/jit/indirectcalltransformer.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index f59de1029d3f70..7df625bc1ad2ea 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -539,8 +539,9 @@ 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) && - (origCall->GetGDVCandidatesCount() == 1); + const bool multipleCandidates = origCall->GetGDVCandidatesCount() > 1; + const bool isChainedGdv = + ((origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_CHAIN) != 0) && !multipleCandidates; // NOTE: calls with multiple GDV candidates don't support this opt yet. @@ -554,7 +555,10 @@ class IndirectCallTransformer if (isChainedGdv) { TransformForChainedGdv(); + } + if (!multipleCandidates) + { // Look ahead and see if there's another Gdv we might chain to this one. // ScoutForChainedGdv(); From e6c989a6445da70777aa82ee56089d6f71480f18 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 4 Oct 2021 21:24:30 +0300 Subject: [PATCH 21/26] Update src/coreclr/jit/importer.cpp Co-authored-by: Andy Ayers --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index ff16e558eb767f..ad1fc9abfa9171 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -22152,7 +22152,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, // If the guarded class is a value class, look for an unboxed entry point. // - if ((info.compCompHnd->getClassAttribs(classHandle) & CORINFO_FLG_VALUECLASS) != 0) + if (info.compCompHnd->isValueClass(classHandle)) { JITDUMP(" ... class is a value class, looking for unboxed entry\n"); bool requiresInstMethodTableArg = false; From 616c97f7e4eadee3f3d91613b56a21e6ff847057 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Oct 2021 21:28:47 +0300 Subject: [PATCH 22/26] Address feedback --- src/coreclr/jit/importer.cpp | 3 +++ src/coreclr/jit/indirectcalltransformer.cpp | 12 +++--------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index ff16e558eb767f..f9385fc7ca848d 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -20580,6 +20580,9 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // exactContextNeedsRuntimeLookup -- true if context required runtime lookup // callInfo -- call info from VM // +// Returns: +// True if the given call is inlineable. +// // Notes: // If callNode is an inline candidate, this method sets the flag // GTF_CALL_INLINE_CANDIDATE, and ensures that helper methods have diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 7df625bc1ad2ea..29fa927cb295d5 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -600,16 +600,16 @@ class IndirectCallTransformer // virtual void CreateCheck(UINT8 checkIdx) override { - // There's no need for a new block here. We can just append to currBlock. - // if (checkIdx == 0) { + // There's no need for a new block here. We can just append to currBlock. checkBlock = currBlock; checkBlock->bbJumpKind = BBJ_COND; checkBlock->inheritWeight(currBlock); } else { + // Append to the previous "Then" block BasicBlock* prevCheckBlock = checkBlock; checkBlock = CreateAndInsertBasicBlock(BBJ_COND, thenBlock); checkBlock->bbFlags |= currBlock->bbFlags & BBF_SPLIT_GAINED; @@ -808,13 +808,7 @@ class IndirectCallTransformer // We know this call can devirtualize or we would not have set up GDV here. // So impDevirtualizeCall should succeed in devirtualizing. // - if (call->IsVirtual()) - { - // TODO: fix - assert(checkIdx > 0); - call = compiler->gtCloneCandidateCall(origCall); - call->gtStubCallStubAddr = call->GetInlineCandidateInfo()->stubAddr; - } + assert(!call->IsVirtual()) // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. From 710c1499edb67fe07dccad09f3747eb94f9a4741 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 4 Oct 2021 22:38:29 +0300 Subject: [PATCH 23/26] Update indirectcalltransformer.cpp --- 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 29fa927cb295d5..1d93315ff488e0 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -808,7 +808,7 @@ class IndirectCallTransformer // We know this call can devirtualize or we would not have set up GDV here. // So impDevirtualizeCall should succeed in devirtualizing. // - assert(!call->IsVirtual()) + assert(!call->IsVirtual()); // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. From 5f68a56c7006c997f66d5c8916cc1d10030f9777 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 5 Oct 2021 15:36:51 +0300 Subject: [PATCH 24/26] Address feedback --- src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/importer.cpp | 22 +++++++++++---------- src/coreclr/jit/indirectcalltransformer.cpp | 8 ++++---- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 6d159fcfe55ade..3818118fd094f0 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4710,6 +4710,7 @@ struct GenTreeCall final : public GenTree void ClearGuardedDevirtualizationCandidate() { gtCallMoreFlags &= ~GTF_CALL_M_GUARDED_DEVIRT; + SetGDVCandidatesCount(0); } void SetGuardedDevirtualizationCandidate() diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b17df6aa071952..725a311e07111c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -6909,7 +6909,6 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) { JITDUMP("Disabling GDV for [%06u] because of in-box struct return\n"); call->ClearGuardedDevirtualizationCandidate(); - call->SetGDVCandidatesCount(0); if (call->IsVirtualStub()) { JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n", @@ -20574,7 +20573,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, dspTreeID(call)); call->ClearGuardedDevirtualizationCandidate(); - call->SetGDVCandidatesCount(0); // If we have a stub address, restore it back into the union that it shares // with the candidate info. @@ -21970,8 +21968,9 @@ void Compiler::considerGuardedDevirtualization( // See if there's a likely guess for the class. // - const unsigned likelihoodThreshold = isInterface ? 25 : 30; - unsigned numberOfClasses = 0; + const unsigned primaryLikelihoodThreshold = isInterface ? 25 : 30; + const unsigned secondaryLikelihoodThreshold = isInterface ? 10 : 15; + unsigned numberOfClasses = 0; bool doRandomDevirt = false; @@ -22022,12 +22021,13 @@ void Compiler::considerGuardedDevirtualization( assert(call->GetGDVCandidatesCount() == 0); - INDEBUG(CORINFO_SIG_INFO prevSig = {}); - for (UINT i = 0; i < numberOfClasses; i++) { - if (call->GetGDVCandidatesCount() >= (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount()) + const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount(); + if (call->GetGDVCandidatesCount() >= maxCandidates) { + JITDUMP("Reaching the JitGuardedDevirtualizationCheckCount=%u limit, other candidates are ignored.", + maxCandidates); break; } @@ -22038,11 +22038,13 @@ void Compiler::considerGuardedDevirtualization( // // For now we will guess if the likelihood is at least 25%/30% (intfc/virt), as studies // have shown this transformation should pay off even if we guess wrong sometimes. + // And smaller likelihoods for secondary guesses - 10%/15% (intfc/virt). // - if ((likelyClasses[i].likelihood < likelihoodThreshold / (i + 1))) + const UINT32 currentThreshold = + call->GetGDVCandidatesCount() == 0 ? primaryLikelihoodThreshold : secondaryLikelihoodThreshold; + if ((likelyClasses[i].likelihood < currentThreshold)) { - JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, - likelihoodThreshold / (i + 1)); + JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, currentThreshold); break; } diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 1d93315ff488e0..e08d39d6e8253b 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -531,7 +531,7 @@ class IndirectCallTransformer if (!origCall->IsInlineCandidate()) { JITDUMP("*** %s Bailing on [%06u] -- not an inline candidate\n", Name(), compiler->dspTreeID(origCall)); - ClearFlag(); + origCall->ClearGuardedDevirtualizationCandidate(); return; } @@ -551,6 +551,7 @@ class IndirectCallTransformer } Transform(); + origCall->ClearGuardedDevirtualizationCandidate(); if (isChainedGdv) { @@ -588,11 +589,10 @@ class IndirectCallTransformer } //------------------------------------------------------------------------ - // ClearFlag: clear guarded devirtualization candidate flag from the original call. + // ClearFlag: not used // virtual void ClearFlag() override { - origCall->ClearGuardedDevirtualizationCandidate(); } //------------------------------------------------------------------------ @@ -790,7 +790,7 @@ class IndirectCallTransformer GenTreeCall* call = compiler->gtCloneCandidateCall(origCall); call->gtCallThisArg = compiler->gtNewCallArgs(compiler->gtNewLclvNode(thisTemp, TYP_REF)); call->SetIsGuarded(); - call->SetGDVCandidatesCount(0); + call->ClearGuardedDevirtualizationCandidate(); JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), thenBlock->bbNum); From 104329012744427eebec49b1ac4512a029f5beda Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 5 Oct 2021 16:37:09 +0300 Subject: [PATCH 25/26] Address feedback --- src/coreclr/jit/importer.cpp | 14 +-- src/coreclr/jit/indirectcalltransformer.cpp | 133 ++++++++++---------- 2 files changed, 70 insertions(+), 77 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 725a311e07111c..54bc3cc2299b18 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9557,18 +9557,14 @@ var_types Compiler::impImportCall(OPCODE opcode, // Make the call its own tree (spill the stack if needed). impAppendTree(call, (unsigned)CHECK_SPILL_ALL, impCurStmtOffs); + // Propagate retExpr as the placeholder for the call. + // TODO: Still using the widened type. + call = gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); for (UINT8 i = 0; i < max(1, origCall->GetGDVCandidatesCount()); i++) { - // TODO: Still using the widened type. - GenTree* retExpr = - gtNewInlineCandidateReturnExpr(call, genActualType(callRetTyp), compCurBB->bbFlags); - - // Link the retExpr to the call so if necessary we can manipulate it later. - InlineCandidateInfo* inlineInfo = origCall->GetInlineCandidateInfo(i); - inlineInfo->retExpr = retExpr; + // Save link to retExpr to all gdv candidates just in case. + origCall->GetInlineCandidateInfo(i)->retExpr = call; } - // Propagate retExpr as the placeholder for the call. - call = origCall->GetInlineCandidateInfo()->retExpr; } else { diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index e08d39d6e8253b..89bbe809577416 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -671,90 +671,87 @@ class IndirectCallTransformer // so any struct copy we induce here should be cheap. assert(origCall->GetGDVCandidatesCount() > 0); - for (UINT8 candidateId = 0; candidateId < origCall->GetGDVCandidatesCount(); candidateId++) + InlineCandidateInfo* const inlineInfo = origCall->GetInlineCandidateInfo(0); + GenTree* const retExpr = inlineInfo->retExpr; + + // Sanity check the ret expr if non-null: it should refer to the original call. + if (retExpr != nullptr) + { + assert(retExpr->AsRetExpr()->gtInlineCandidate == origCall); + } + + if (origCall->TypeGet() != TYP_VOID) { - InlineCandidateInfo* const inlineInfo = origCall->GetInlineCandidateInfo(candidateId); - GenTree* const retExpr = inlineInfo->retExpr; + // If there's a spill temp already associated with this inline candidate, + // use that instead of allocating a new temp. + // - // Sanity check the ret expr if non-null: it should refer to the original call. - if (retExpr != nullptr) + if (returnTemp == BAD_VAR_NUM) { - assert(retExpr->AsRetExpr()->gtInlineCandidate == origCall); + returnTemp = inlineInfo->preexistingSpillTemp; } - if (origCall->TypeGet() != TYP_VOID) + if (returnTemp != BAD_VAR_NUM) { - // If there's a spill temp already associated with this inline candidate, - // use that instead of allocating a new temp. - // + JITDUMP("Reworking call(s) to return value via a existing return temp V%02u\n", returnTemp); - if (returnTemp == BAD_VAR_NUM) - { - returnTemp = inlineInfo->preexistingSpillTemp; - } + // We will be introducing multiple defs for this temp, so make sure + // it is no longer marked as single def. + // + // Otherwise, we could make an incorrect type deduction. Say the + // original call site returns a B, but after we devirtualize along the + // GDV happy path we see that method returns a D. We can't then assume that + // the return temp is type D, because we don't know what type the fallback + // path returns. So we have to stick with the current type for B as the + // return type. + // + // Note local vars always live in the root method's symbol table. So we + // need to use the root compiler for lookup here. + // + LclVarDsc* const returnTempLcl = compiler->impInlineRoot()->lvaGetDesc(returnTemp); - if (returnTemp != BAD_VAR_NUM) + if (returnTempLcl->lvSingleDef == 1) { - JITDUMP("Reworking call(s) to return value via a existing return temp V%02u\n", returnTemp); - - // We will be introducing multiple defs for this temp, so make sure - // it is no longer marked as single def. - // - // Otherwise, we could make an incorrect type deduction. Say the - // original call site returns a B, but after we devirtualize along the - // GDV happy path we see that method returns a D. We can't then assume that - // the return temp is type D, because we don't know what type the fallback - // path returns. So we have to stick with the current type for B as the - // return type. - // - // Note local vars always live in the root method's symbol table. So we - // need to use the root compiler for lookup here. + // In this case it's ok if we already updated the type assuming single def, + // we just don't want any further updates. // - LclVarDsc* const returnTempLcl = compiler->impInlineRoot()->lvaGetDesc(returnTemp); - - if (returnTempLcl->lvSingleDef == 1) - { - // In this case it's ok if we already updated the type assuming single def, - // we just don't want any further updates. - // - JITDUMP("Return temp V%02u is no longer a single def temp\n", returnTemp); - returnTempLcl->lvSingleDef = 0; - } + JITDUMP("Return temp V%02u is no longer a single def temp\n", returnTemp); + returnTempLcl->lvSingleDef = 0; } - else - { - returnTemp = compiler->lvaGrabTemp(false DEBUGARG("guarded devirt return temp")); - JITDUMP("Reworking call(s) to return value via a new temp V%02u\n", returnTemp); - } - - if (varTypeIsStruct(origCall)) - { - compiler->lvaSetStruct(returnTemp, origCall->gtRetClsHnd, false); - } - - GenTree* tempTree = compiler->gtNewLclvNode(returnTemp, origCall->TypeGet()); - - JITDUMP("Bashing GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(retExpr), - returnTemp); - - retExpr->ReplaceWith(tempTree, compiler); } - else if (retExpr != nullptr) + else { - // We still oddly produce GT_RET_EXPRs for some void - // returning calls. Just bash the ret expr to a NOP. - // - // Todo: consider bagging creation of these RET_EXPRs. The only possible - // benefit they provide is stitching back larger trees for failed inlines - // of void-returning methods. But then the calls likely sit in commas and - // the benefit of a larger tree is unclear. - JITDUMP("Bashing GT_RET_EXPR [%06u] for VOID return to NOP\n", compiler->dspTreeID(retExpr)); - retExpr->gtBashToNOP(); + returnTemp = compiler->lvaGrabTemp(false DEBUGARG("guarded devirt return temp")); + JITDUMP("Reworking call(s) to return value via a new temp V%02u\n", returnTemp); } - else + + if (varTypeIsStruct(origCall)) { - // We do not produce GT_RET_EXPRs for CTOR calls, so there is nothing to patch. + compiler->lvaSetStruct(returnTemp, origCall->gtRetClsHnd, false); } + + GenTree* tempTree = compiler->gtNewLclvNode(returnTemp, origCall->TypeGet()); + + JITDUMP("Bashing GT_RET_EXPR [%06u] to refer to temp V%02u\n", compiler->dspTreeID(retExpr), + returnTemp); + + retExpr->ReplaceWith(tempTree, compiler); + } + else if (retExpr != nullptr) + { + // We still oddly produce GT_RET_EXPRs for some void + // returning calls. Just bash the ret expr to a NOP. + // + // Todo: consider bagging creation of these RET_EXPRs. The only possible + // benefit they provide is stitching back larger trees for failed inlines + // of void-returning methods. But then the calls likely sit in commas and + // the benefit of a larger tree is unclear. + JITDUMP("Bashing GT_RET_EXPR [%06u] for VOID return to NOP\n", compiler->dspTreeID(retExpr)); + retExpr->gtBashToNOP(); + } + else + { + // We do not produce GT_RET_EXPRs for CTOR calls, so there is nothing to patch. } } From 910c5c9b98ed9466beba455a69240b8d8249e6e0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 5 Oct 2021 20:59:03 +0300 Subject: [PATCH 26/26] Address feedback --- src/coreclr/jit/indirectcalltransformer.cpp | 23 ++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 89bbe809577416..2863c7288211ed 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -535,9 +535,26 @@ class IndirectCallTransformer return; } - likelihood = origCall->GetInlineCandidateInfo()->likelihood; - assert((likelihood >= 0) && (likelihood <= 100)); - JITDUMP("Likelihood of correct guess is %u\n", likelihood); + assert(origCall->GetGDVCandidatesCount() > 0); + + // likelihood field is only used by ScoutForChainedGdv that only supports monomorphic calls for now, + // hence, we only take likelihood of the first candidate here + likelihood = origCall->GetInlineCandidateInfo(0)->likelihood; + + JITDUMP("\nLikelihoods of GDV candidates:"); + assert(likelihood <= 100); + for (UINT8 candidateId = 0; candidateId < origCall->GetGDVCandidatesCount(); candidateId++) + { + const UINT32 currentLikelihood = origCall->GetInlineCandidateInfo(candidateId)->likelihood; + JITDUMP("\n\tCandidate %u - %u%%", candidateId, currentLikelihood); + if (candidateId > 0) + { + const UINT32 prevLikelihood = origCall->GetInlineCandidateInfo(candidateId - 1)->likelihood; + assert(prevLikelihood >= currentLikelihood); + JITDUMP(" (relative likelihood %0.1f%%)", currentLikelihood * 100.0 / prevLikelihood); + } + } + JITDUMP("\n\n"); const bool multipleCandidates = origCall->GetGDVCandidatesCount() > 1; const bool isChainedGdv =