From 64b4091c051a1eb73959912b67f74e78debdc503 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 27 Feb 2024 13:23:13 -0800 Subject: [PATCH 1/5] JIT: start checking sum of successor likelihoods Add the ability to check that uccessor edge likelihoods sum to 1.0 Verify that successor edge likelihoods sum to 1.0 for the phases up through and including `fgAddInternal`. Fixed a few problems * importer branch folding needs to update likelihoods * gdv likelihood needed some adjustments, especially for the multi-guess case * profile incorporation for OSR methods also needed some fixing. Here we may have inconsistencies so the right answer is not always clear. With this change we now trust the successor edge counts even if they do not sum to the block count. Contributes to #93020. --- src/coreclr/jit/block.cpp | 11 ++ src/coreclr/jit/block.h | 8 +- src/coreclr/jit/compiler.h | 7 +- src/coreclr/jit/fginline.cpp | 3 + src/coreclr/jit/fgprofile.cpp | 121 ++++++-------------- src/coreclr/jit/importer.cpp | 5 +- src/coreclr/jit/indirectcalltransformer.cpp | 84 ++++++++++---- 7 files changed, 118 insertions(+), 121 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 273c6f045123d..905c5232e7627 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -51,6 +51,17 @@ FlowEdge* ShuffleHelper(unsigned hash, FlowEdge* res) return head; } +void FlowEdge::setLikelihood(weight_t likelihood) +{ + assert(likelihood >= 0.0); + assert(likelihood <= 1.0); + m_likelihoodSet = true; + m_likelihood = likelihood; + + JITDUMP("setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT "\n", m_sourceBlock->bbNum, m_destBlock->bbNum, + m_likelihood); +} + unsigned SsaStressHashHelper() { // hash = 0: turned off, hash = 1: use method hash, hash = *: use custom hash. diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 208eb07e583fa..f236d75c8f342 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -2175,13 +2175,7 @@ struct FlowEdge return m_likelihood; } - void setLikelihood(weight_t likelihood) - { - assert(likelihood >= 0.0); - assert(likelihood <= 1.0); - m_likelihoodSet = true; - m_likelihood = likelihood; - } + void setLikelihood(weight_t likelihood); void clearLikelihood() { diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 364f51c2b529b..d31c18b7ede83 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1583,9 +1583,10 @@ enum class ProfileChecks : unsigned int CHECK_NONE = 0, CHECK_CLASSIC = 1 << 0, // check "classic" jit weights CHECK_HASLIKELIHOOD = 1 << 1, // check all FlowEdges for hasLikelihood - CHECK_LIKELY = 1 << 2, // fully check likelihood based weights - RAISE_ASSERT = 1 << 3, // assert on check failure - CHECK_ALL_BLOCKS = 1 << 4, // check blocks even if bbHasProfileWeight is false + CHECK_LIKELIHOODSUM = 1 << 2, // check block succesor likelihoods sum to 1 + CHECK_LIKELY = 1 << 3, // fully check likelihood based weights + RAISE_ASSERT = 1 << 4, // assert on check failure + CHECK_ALL_BLOCKS = 1 << 5, // check blocks even if bbHasProfileWeight is false }; inline constexpr ProfileChecks operator ~(ProfileChecks a) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 54003637e7e36..1d8cedf0cd760 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -684,6 +684,9 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorfgRemoveRefPred(block->GetFalseTarget(), block); block->SetKind(BBJ_ALWAYS); } + + FlowEdge* const edge = m_compiler->fgGetPredForBlock(block->GetTarget(), block); + edge->setLikelihood(1.0); } } else diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index a1c9da833fd3f..fa8b6918ec1b3 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -3862,18 +3862,22 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block, { // We expect one pseudo-edge and at least one normal edge. // - Edge* pseudoEdge = nullptr; - unsigned nEdges = 0; + Edge* pseudoEdge = nullptr; + weight_t pseudoEdgeWeight = 0; + unsigned nEdges = 0; + weight_t successorWeight = BB_ZERO_WEIGHT; for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge) { if (edge->m_isPseudoEdge) { assert(pseudoEdge == nullptr); - pseudoEdge = edge; + pseudoEdge = edge; + pseudoEdgeWeight = edge->m_weight; continue; } + successorWeight += edge->m_weight; nEdges++; } @@ -3890,28 +3894,25 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block, assert(nEdges == nSucc); - if (info->m_weight == BB_ZERO_WEIGHT) + if ((info->m_weight == BB_ZERO_WEIGHT) || (successorWeight == BB_ZERO_WEIGHT)) { - JITDUMP("\nPropagate: OSR entry block weight is zero\n"); + JITDUMP("\nPropagate: OSR entry block or successor weight is zero\n"); EntryWeightZero(); return; } // Transfer model edge weight onto the FlowEdges as likelihoods. // - assert(nEdges == nSucc); - weight_t totalLikelihood = 0; + JITDUMP("Normalizing OSR successor likelihoods with factor 1/" FMT_WT "\n", successorWeight); for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge) { assert(block == edge->m_sourceBlock); - // The pseudo edge doesn't correspond to a flow edge, - // but it carries away some flow. + // The pseudo edge doesn't correspond to a flow edge. // if (edge == pseudoEdge) { - totalLikelihood += edge->m_weight / info->m_weight; continue; } @@ -3927,51 +3928,19 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block, if (nEdges == 1) { - // Conceptually we could assert(edge->m_weight == info->m_weight); - // but we can have inconsistencies. - // // Go with what we know for sure, edge should be 100% likely. // likelihood = 1.0; JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (uniq)\n", block->bbNum, edge->m_targetBlock->bbNum, likelihood); flowEdge->setLikelihood(likelihood); - totalLikelihood += likelihood; break; } - assert(info->m_weight != BB_ZERO_WEIGHT); - - // We may see nonsensical weights here, cap likelihood. - // - bool capped = false; - if (edge->m_weight > info->m_weight) - { - capped = true; - likelihood = 1.0; - } - else - { - likelihood = edge->m_weight / info->m_weight; - } - JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (%s)\n", block->bbNum, - edge->m_targetBlock->bbNum, likelihood, capped ? "pgo -- capped" : "pgo"); + likelihood = edge->m_weight / successorWeight; + JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (pgo)\n", block->bbNum, + edge->m_targetBlock->bbNum, likelihood); flowEdge->setLikelihood(likelihood); - totalLikelihood += likelihood; - } - - // Note we expect real flow imbalances here as it's likely there - // was no observed flow from the OSR entry to some of its successors. - // Since we added in the pseudo edge likelihood above, the check below - // probably won't flag this. - // - // Seems like for OSR we will always want to run synthesis/repair. - // - if (totalLikelihood != 1.0) - { - // Consider what to do here... flag this method as needing immediate profile repairs? - // - JITDUMP(FMT_BB " total outgoing likelihood inaccurate: " FMT_WT "\n", block->bbNum, totalLikelihood); } } @@ -3984,10 +3953,6 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block, // info - model info for the block // nSucc - number of successors of the block in the flow graph // -// Notes: -// This block requires special handling because original method flow -// was interrupted here. -// void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInfo* info, unsigned nSucc) { // There is at least one FlowEdge. @@ -3995,8 +3960,9 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf // Check the reconstruction graph edges. For normal blocks, if we have // any pseudo-edges there should be only one pseudo-edge, and no regular edges. // - Edge* pseudoEdge = nullptr; - unsigned nEdges = 0; + Edge* pseudoEdge = nullptr; + unsigned nEdges = 0; + weight_t successorWeight = BB_ZERO_WEIGHT; for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge) { @@ -4007,6 +3973,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf continue; } + successorWeight += edge->m_weight; nEdges++; } @@ -4046,7 +4013,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf // // (TODO: use synthesis here) // - if ((nEdges != nSucc) || (info->m_weight == BB_ZERO_WEIGHT)) + if ((nEdges != nSucc) || (info->m_weight == BB_ZERO_WEIGHT) || (successorWeight == BB_ZERO_WEIGHT)) { JITDUMP(FMT_BB " %s , setting outgoing likelihoods heuristically\n", block->bbNum, (nEdges != nSucc) ? "has inaccurate flow model" : "has zero weight"); @@ -4067,7 +4034,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf // Transfer model edge weight onto the FlowEdges as likelihoods. // assert(nEdges == nSucc); - weight_t totalLikelihood = 0; + JITDUMP("Normalizing successor likelihoods with factor 1/" FMT_WT "\n", successorWeight); for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge) { @@ -4079,45 +4046,17 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf if (nEdges == 1) { assert(nSucc == 1); - - // Conceptually we could assert(edge->m_weight == info->m_weight); - // but we can have inconsistencies. - // - // Go with what we know for sure, edge should be 100% likely. - // likelihood = 1.0; JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (uniq)\n", block->bbNum, edge->m_targetBlock->bbNum, likelihood); flowEdge->setLikelihood(likelihood); - totalLikelihood += likelihood; break; } - assert(info->m_weight != BB_ZERO_WEIGHT); - - // We may see nonsensical weights here, cap likelihood. - // - bool capped = false; - if (edge->m_weight > info->m_weight) - { - capped = true; - likelihood = 1.0; - } - else - { - likelihood = edge->m_weight / info->m_weight; - } - JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (%s)\n", block->bbNum, - edge->m_targetBlock->bbNum, likelihood, capped ? "pgo -- capped" : "pgo"); + likelihood = edge->m_weight / successorWeight; + JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (pgo)\n", block->bbNum, + edge->m_targetBlock->bbNum, likelihood); flowEdge->setLikelihood(likelihood); - totalLikelihood += likelihood; - } - - if (totalLikelihood != 1.0) - { - // Consider what to do here... flag this method as needing immediate profile repairs? - // - JITDUMP(FMT_BB " total outgoing likelihood inaccurate: " FMT_WT "\n", block->bbNum, totalLikelihood); } } @@ -5296,7 +5235,8 @@ void Compiler::fgDebugCheckProfileWeights() } else { - ProfileChecks checks = ProfileChecks::CHECK_HASLIKELIHOOD | ProfileChecks::RAISE_ASSERT; + ProfileChecks checks = + ProfileChecks::CHECK_HASLIKELIHOOD | ProfileChecks::CHECK_LIKELIHOODSUM | ProfileChecks::RAISE_ASSERT; fgDebugCheckProfileWeights(checks); } } @@ -5328,6 +5268,7 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC); const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); + const bool verifyLikelihoodSum = hasFlag(checks, ProfileChecks::CHECK_LIKELIHOODSUM); const bool assertOnFailure = hasFlag(checks, ProfileChecks::RAISE_ASSERT); const bool checkAllBlocks = hasFlag(checks, ProfileChecks::CHECK_ALL_BLOCKS); @@ -5478,6 +5419,10 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) JITDUMP("Profile is self-consistent (%d profiled blocks, %d unprofiled)\n", profiledBlocks, unprofiledBlocks); } + else if (verifyLikelihoodSum) + { + JITDUMP("All block successor flow edge likelihoods sum to 1.0\n"); + } else if (verifyHasLikelihood) { JITDUMP("All flow edges have likelihoods\n"); @@ -5618,10 +5563,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks checks) { const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC); - const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); + const bool verifyLikelihoodSum = hasFlag(checks, ProfileChecks::CHECK_LIKELIHOODSUM); - if (!(verifyClassicWeights || verifyLikelyWeights || verifyHasLikelihood)) + if (!(verifyClassicWeights || verifyHasLikelihood || verifyLikelihoodSum)) { return true; } @@ -5711,7 +5656,7 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks } } - if (verifyLikelyWeights) + if (verifyLikelihoodSum) { if (!fgProfileWeightsConsistent(outgoingLikelihood, 1.0)) { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 22bc4a2ba7337..4e737e835c883 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7270,11 +7270,13 @@ void Compiler::impImportBlockCode(BasicBlock* block) JITDUMP("\nThe block jumps to the next " FMT_BB "\n", block->Next()->bbNum); fgRemoveRefPred(block->GetTrueTarget(), block); block->SetKindAndTarget(BBJ_ALWAYS, block->Next()); - // TODO-NoFallThrough: Once bbFalseTarget can diverge from bbNext, it may not make sense // to set BBF_NONE_QUIRK block->SetFlags(BBF_NONE_QUIRK); } + + FlowEdge* const edge = fgGetPredForBlock(block->GetTarget(), block); + edge->setLikelihood(1.0); } break; @@ -7536,6 +7538,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) { // transform the basic block into a BBJ_ALWAYS block->SetKindAndTarget(BBJ_ALWAYS, curEdge->getDestinationBlock()); + curEdge->setLikelihood(1.0); foundVal = true; } else diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index fbbe08d66fc3c..29930b318dc9e 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -613,6 +613,9 @@ class IndirectCallTransformer assert(checkLikelihood <= 100); weight_t checkLikelihoodWt = ((weight_t)checkLikelihood) / 100.0; + JITDUMP("Level %u Check block " FMT_BB " success likelihood " FMT_WT "\n", checkIdx, checkBlock->bbNum, + checkLikelihoodWt); + // prevCheckBlock is expected to jump to this new check (if its type check doesn't succeed) prevCheckBlock->SetCond(checkBlock, prevCheckBlock->Next()); FlowEdge* const checkEdge = compiler->fgAddRefPred(checkBlock, prevCheckBlock); @@ -1016,25 +1019,57 @@ class IndirectCallTransformer { // Compute likelihoods // - unsigned const thenLikelihood = origCall->GetGDVCandidateInfo(checkIdx)->likelihood; - weight_t thenLikelihoodWt = min(((weight_t)thenLikelihood) / 100.0, 100.0); - weight_t elseLikelihoodWt = max(1.0 - thenLikelihoodWt, 0.0); + // If this is the first check the likelihood is just the candidate likelihood. + // If there are multiple checks things are a bit more complicated. + // + // Say we had three candidates with likelihoods 0.5, 0.3, and 0.1. + // + // The first one's likelihood is 0.5. + // + // The second one (given that we've already checked the first and failed) + // is (0.3) / (1.0 - 0.5) = 0.6. + // + // The third one is (0.1) / (1.0 - (0.5 + 0.2)) = 0.5 + // + // So to figure out the proper divisor, we start with 1.0 and subtract off each + // preceeding test's likelihood of success. + // + unsigned const thenLikelihood = origCall->GetGDVCandidateInfo(checkIdx)->likelihood; + unsigned baseLikelihood = 100; + + for (uint8_t i = 0; i < checkIdx; i++) + { + baseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; + } + + assert(baseLikelihood > 0); + weight_t adjustedThenLikelihood = min(((weight_t)thenLikelihood) / baseLikelihood, 100.0); + JITDUMP("For check in " FMT_BB ": orig likelihood " FMT_WT ", base likelihood " FMT_WT + ", adjusted likelihood " FMT_WT "\n", + checkBlock->bbNum, (weight_t)thenLikelihood / 100.0, (weight_t)baseLikelihood / 100.0, + adjustedThenLikelihood); // thenBlock always jumps to remainderBlock + // thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock, remainderBlock); thenBlock->CopyFlags(currBlock, BBF_SPLIT_GAINED); - thenBlock->inheritWeightPercentage(currBlock, thenLikelihood); + thenBlock->inheritWeight(currBlock); + thenBlock->scaleBBWeight(adjustedThenLikelihood); + FlowEdge* const thenRemainderEdge = compiler->fgAddRefPred(remainderBlock, thenBlock); + thenRemainderEdge->setLikelihood(1.0); - // Also, thenBlock has a single pred - last checkBlock + // thenBlock has a single pred - last checkBlock. + // assert(checkBlock->KindIs(BBJ_ALWAYS)); checkBlock->SetTarget(thenBlock); checkBlock->SetFlags(BBF_NONE_QUIRK); assert(checkBlock->JumpsToNext()); FlowEdge* const thenEdge = compiler->fgAddRefPred(thenBlock, checkBlock); - thenEdge->setLikelihood(thenLikelihoodWt); - FlowEdge* const elseEdge = compiler->fgAddRefPred(remainderBlock, thenBlock); - elseEdge->setLikelihood(elseLikelihoodWt); + thenEdge->setLikelihood(adjustedThenLikelihood); + // We will set the "else edge" likelihood in CreateElse later, + // based on the thenEdge likelihood. + // DevirtualizeCall(thenBlock, checkIdx); } @@ -1043,28 +1078,27 @@ class IndirectCallTransformer // virtual void CreateElse() { - // Calculate the likelihood of the else block as a remainder of the sum - // of all the other likelihoods. - unsigned elseLikelihood = 100; - for (uint8_t i = 0; i < origCall->GetInlineCandidatesCount(); i++) - { - elseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; - } - // Make sure it didn't overflow - assert(elseLikelihood <= 100); - weight_t elseLikelihoodDbl = ((weight_t)elseLikelihood) / 100.0; - elseBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, thenBlock, thenBlock->Next()); elseBlock->CopyFlags(currBlock, BBF_SPLIT_GAINED); elseBlock->SetFlags(BBF_NONE_QUIRK); + // We computed the "then" likelihood in CreateThen, so we + // just use that to figure out the "else" likelihood. + // + assert(checkBlock->KindIs(BBJ_ALWAYS)); + BasicBlock* const thenBlock = checkBlock->GetTarget(); + FlowEdge* const thenEdge = compiler->fgGetPredForBlock(thenBlock, checkBlock); + assert(thenEdge->hasLikelihood()); + weight_t elseLikelihood = max(0.0, 1.0 - thenEdge->getLikelihood()); + // CheckBlock flows into elseBlock unless we deal with the case // where we know the last check is always true (in case of "exact" GDV) + // if (!checkFallsThrough) { checkBlock->SetCond(elseBlock, checkBlock->Next()); FlowEdge* const checkEdge = compiler->fgAddRefPred(elseBlock, checkBlock); - checkEdge->setLikelihood(elseLikelihoodDbl); + checkEdge->setLikelihood(elseLikelihood); } else { @@ -1072,6 +1106,11 @@ class IndirectCallTransformer // and is NativeAOT-only, we just assume the unreached block will be removed // by other phases. assert(origCall->gtCallMoreFlags & GTF_CALL_M_GUARDED_DEVIRT_EXACT); + + // Since we're not modifying the check block a BBJ_COND, update the + // then edge likelihood (it should already have the right value, so perhaps assert instead?) + // + thenEdge->setLikelihood(1.0); } // elseBlock always flows into remainderBlock @@ -1081,7 +1120,8 @@ class IndirectCallTransformer // Remove everything related to inlining from the original call origCall->ClearInlineInfo(); - elseBlock->inheritWeightPercentage(currBlock, elseLikelihood); + elseBlock->inheritWeight(checkBlock); + elseBlock->scaleBBWeight(elseLikelihood); GenTreeCall* call = origCall; Statement* newStmt = compiler->gtNewStmt(call, stmt->GetDebugInfo()); @@ -1181,7 +1221,7 @@ class IndirectCallTransformer compiler->fgAddRefPred(elseBlock, coldBlock, oldEdge); } - // When the current candidate hads sufficiently high likelihood, scan + // When the current candidate has sufficiently high likelihood, scan // the remainer block looking for another GDV candidate. // // (also consider: if currBlock has sufficiently high execution frequency) From 31592768f0566f5755f149edd43407c2a0cf4c3e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 27 Feb 2024 17:51:00 -0800 Subject: [PATCH 2/5] move method so it's available in release --- src/coreclr/jit/block.cpp | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 905c5232e7627..db19f1598a545 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -51,17 +51,6 @@ FlowEdge* ShuffleHelper(unsigned hash, FlowEdge* res) return head; } -void FlowEdge::setLikelihood(weight_t likelihood) -{ - assert(likelihood >= 0.0); - assert(likelihood <= 1.0); - m_likelihoodSet = true; - m_likelihood = likelihood; - - JITDUMP("setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT "\n", m_sourceBlock->bbNum, m_destBlock->bbNum, - m_likelihood); -} - unsigned SsaStressHashHelper() { // hash = 0: turned off, hash = 1: use method hash, hash = *: use custom hash. @@ -79,6 +68,24 @@ unsigned SsaStressHashHelper() } #endif +//------------------------------------------------------------------------ +// setLikelihood: set the likelihood of aflow edge +// +// Arguments: +// likelihood -- value in range [0.0, 1.0] indicating how likely +// the source block is to transfer control along this edge. +// +void FlowEdge::setLikelihood(weight_t likelihood) +{ + assert(likelihood >= 0.0); + assert(likelihood <= 1.0); + m_likelihoodSet = true; + m_likelihood = likelihood; + + JITDUMP("setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT "\n", m_sourceBlock->bbNum, m_destBlock->bbNum, + m_likelihood); +} + //------------------------------------------------------------------------ // AllSuccessorEnumerator: Construct an instance of the enumerator. // From 490ab8a8b6634c747c8a2c00fbb3367d51b79fb3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 28 Feb 2024 08:21:48 -0800 Subject: [PATCH 3/5] post-merge cleanup, also rework last bit of multi-guess gdv --- src/coreclr/jit/indirectcalltransformer.cpp | 40 ++++++++++----------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 84c6e318d9db1..3210c8a5c42d1 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -603,28 +603,25 @@ class IndirectCallTransformer checkBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, thenBlock); checkFallsThrough = false; - // Calculate the total likelihood for this check as a sum of likelihoods - // of all previous candidates (thenBlocks) - unsigned checkLikelihood = 100; - for (uint8_t previousCandidate = 0; previousCandidate < checkIdx; previousCandidate++) - { - checkLikelihood -= origCall->GetGDVCandidateInfo(previousCandidate)->likelihood; - } - - // Make sure we didn't overflow - assert(checkLikelihood <= 100); - weight_t checkLikelihoodWt = ((weight_t)checkLikelihood) / 100.0; + // We computed the "then" likelihood in CreateThen, so we + // just use that to figure out the "else" likelihood. + // + assert(prevCheckBlock->KindIs(BBJ_ALWAYS)); + assert(prevCheckBlock->JumpsToNext()); + FlowEdge* const prevCheckThenEdge = prevCheckBlock->GetTargetEdge(); + assert(prevCheckThenEdge->hasLikelihood()); + weight_t checkLikelihood = max(0.0, 1.0 - prevCheckThenEdge->getLikelihood()); JITDUMP("Level %u Check block " FMT_BB " success likelihood " FMT_WT "\n", checkIdx, checkBlock->bbNum, - checkLikelihoodWt); + checkLikelihood); // prevCheckBlock is expected to jump to this new check (if its type check doesn't succeed) - assert(prevCheckBlock->KindIs(BBJ_ALWAYS)); - assert(prevCheckBlock->JumpsToNext()); - FlowEdge* const checkEdge = compiler->fgAddRefPred(checkBlock, prevCheckBlock); - checkEdge->setLikelihood(checkLikelihoodWt); - checkBlock->inheritWeightPercentage(currBlock, checkLikelihood); - prevCheckBlock->SetCond(checkEdge, prevCheckBlock->GetTargetEdge()); + // + FlowEdge* const prevCheckCheckEdge = compiler->fgAddRefPred(checkBlock, prevCheckBlock); + prevCheckCheckEdge->setLikelihood(checkLikelihood); + checkBlock->inheritWeight(prevCheckBlock); + checkBlock->scaleBBWeight(checkLikelihood); + prevCheckBlock->SetCond(prevCheckCheckEdge, prevCheckThenEdge); } // Find last arg with a side effect. All args with any effect @@ -1060,7 +1057,7 @@ class IndirectCallTransformer thenBlock->inheritWeight(currBlock); thenBlock->scaleBBWeight(adjustedThenLikelihood); FlowEdge* const thenRemainderEdge = compiler->fgAddRefPred(remainderBlock, thenBlock); - thenBlock->setTargetEdge(thenRemainderEdge); + thenBlock->SetTargetEdge(thenRemainderEdge); thenRemainderEdge->setLikelihood(1.0); // thenBlock has a single pred - last checkBlock. @@ -1091,8 +1088,7 @@ class IndirectCallTransformer // just use that to figure out the "else" likelihood. // assert(checkBlock->KindIs(BBJ_ALWAYS)); - BasicBlock* const thenBlock = checkBlock->GetTarget(); - FlowEdge* const checkThenEdge = compiler->fgGetPredForBlock(thenBlock, checkBlock); + FlowEdge* const checkThenEdge = checkBlock->GetTargetEdge(); assert(checkThenEdge->hasLikelihood()); weight_t elseLikelihood = max(0.0, 1.0 - checkThenEdge->getLikelihood()); @@ -1105,7 +1101,7 @@ class IndirectCallTransformer assert(checkBlock->JumpsToNext()); FlowEdge* const checkElseEdge = compiler->fgAddRefPred(elseBlock, checkBlock); checkElseEdge->setLikelihood(elseLikelihood); - checkBlock->SetCond(checkElseEdge, checkBlock->GetTargetEdge()); + checkBlock->SetCond(checkElseEdge, checkThenEdge); } else { From e4a83a81d545373c4384225fc198e22bf36fccb6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 28 Feb 2024 08:25:39 -0800 Subject: [PATCH 4/5] fix chained likelhood example computation --- 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 3210c8a5c42d1..95b32c2cd4a1c 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -1030,7 +1030,7 @@ class IndirectCallTransformer // The second one (given that we've already checked the first and failed) // is (0.3) / (1.0 - 0.5) = 0.6. // - // The third one is (0.1) / (1.0 - (0.5 + 0.2)) = 0.5 + // The third one is (0.1) / (1.0 - (0.5 + 0.3)) = (0.1)/(0.2) = 0.5 // // So to figure out the proper divisor, we start with 1.0 and subtract off each // preceeding test's likelihood of success. From 753a207c5d9ee5a08bd122b0217d982648a61f11 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 28 Feb 2024 09:38:39 -0800 Subject: [PATCH 5/5] adjust likelihood adjustment --- src/coreclr/jit/indirectcalltransformer.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 95b32c2cd4a1c..1a295384f464c 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -1036,14 +1036,15 @@ class IndirectCallTransformer // preceeding test's likelihood of success. // unsigned const thenLikelihood = origCall->GetGDVCandidateInfo(checkIdx)->likelihood; - unsigned baseLikelihood = 100; + unsigned baseLikelihood = 0; for (uint8_t i = 0; i < checkIdx; i++) { - baseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; + baseLikelihood += origCall->GetGDVCandidateInfo(i)->likelihood; } + assert(baseLikelihood < 100); + baseLikelihood = 100 - baseLikelihood; - assert(baseLikelihood > 0); weight_t adjustedThenLikelihood = min(((weight_t)thenLikelihood) / baseLikelihood, 100.0); JITDUMP("For check in " FMT_BB ": orig likelihood " FMT_WT ", base likelihood " FMT_WT ", adjusted likelihood " FMT_WT "\n",