From 1df4905c0a406db254e9fdd66102657f8c3915cd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 11 Mar 2024 17:56:29 -0700 Subject: [PATCH 1/3] JIT: enable edge checks throughout Fix the last remaining issues for edge likelihoods. Main challenge here was switch lowering, particularly the expansions of switches into a series of tests. The adjustments here are similar to those for multi-guess GDV and type tests -- as we test possibilities one-by-one we have to adjust and scale up likelihoods of remining possibilities. But for switches things are more complex as edges may have dup counts, and we may eventually reach the point where the remaining tests had zero initial likelihood. Contributes to #93020. --- src/coreclr/jit/compiler.cpp | 5 -- src/coreclr/jit/lower.cpp | 152 +++++++++++++++++++++++++++++++---- src/coreclr/jit/lower.h | 8 +- 3 files changed, 144 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 000f2f9ea952af..2c883284e419fd 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5130,11 +5130,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl } #endif // TARGET_ARM - // Disable profile checks now. - // Over time we will move this further and further back in the phase list, as we fix issues. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Assign registers to variables, etc. // Create LinearScan before Lowering, so that Lowering can call LinearScan methods diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 25d736d1d39e8a..3225fa4f0ec675 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -960,11 +960,23 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // Fix the pred for the default case: the default block target still has originalSwitchBB // as a predecessor, but the fgSplitBlockAfterStatement() moved all predecessors to point // to afterDefaultCondBlock. - comp->fgRemoveRefPred(jumpTab[jumpCnt - 1]); - FlowEdge* const trueEdge = comp->fgAddRefPred(defaultBB, originalSwitchBB, jumpTab[jumpCnt - 1]); + + // Note defaultEdge may also be the edge for some switch cases. We only probe edges, + // so assume each possibility is equally likely. + FlowEdge* const defaultEdge = jumpTab[jumpCnt - 1]; + weight_t const defaultLikelihood = defaultEdge->getLikelihood() / defaultEdge->getDupCount(); + comp->fgRemoveRefPred(defaultEdge); + FlowEdge* const trueEdge = comp->fgAddRefPred(defaultBB, originalSwitchBB); + trueEdge->setLikelihood(defaultLikelihood); + defaultEdge->setLikelihood(defaultEdge->getLikelihood() - defaultLikelihood); // Turn originalSwitchBB into a BBJ_COND. - originalSwitchBB->SetCond(trueEdge, originalSwitchBB->GetTargetEdge()); + FlowEdge* const falseEdge = originalSwitchBB->GetTargetEdge(); + weight_t const switchLikelihood = 1.0 - defaultLikelihood; + falseEdge->setLikelihood(1.0 - defaultLikelihood); + originalSwitchBB->SetCond(trueEdge, falseEdge); + afterDefaultCondBlock->inheritWeight(originalSwitchBB); + afterDefaultCondBlock->scaleBBWeight(switchLikelihood); bool useJumpSequence = jumpCnt < minSwitchTabJumpCnt; @@ -1046,14 +1058,36 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // If no case target follows, the last one doesn't need to be a compare/branch: it can be an // unconditional branch. bool fAnyTargetFollows = false; + + // We need to track how much of hte original switch's likelihood has already been + // tested for. We'll use this to adjust the likelihood of the branches we're adding. + // So far we've tested for the default case, so we'll start with that. + weight_t totalTestLikelihood = defaultLikelihood; for (unsigned i = 0; i < jumpCnt - 1; ++i) { assert(currentBlock != nullptr); - BasicBlock* targetBlock = jumpTab[i]->getDestinationBlock(); + BasicBlock* const targetBlock = jumpTab[i]->getDestinationBlock(); // Remove the switch from the predecessor list of this case target's block. // We'll add the proper new predecessor edge later. - comp->fgRemoveRefPred(jumpTab[i]); + FlowEdge* const oldEdge = jumpTab[i]; + + // Compute the likelihood that this test is successful. + // Divide by number of cases still sharing this edge (reduces likelihood) + // Divide by likelihood of reaching this test (increases likelihood). + // But if there is little chance of reaching this test, set the likelihood to 0.5 + // + weight_t const edgeLikelihood = oldEdge->getLikelihood(); + weight_t const caseLikelihood = edgeLikelihood / oldEdge->getDupCount(); + bool const unlikelyToReachThisCase = Compiler::fgProfileWeightsEqual(totalTestLikelihood, 1.0, 0.001); + weight_t const adjustedCaseLikelihood = + unlikelyToReachThisCase ? 0.5 : caseLikelihood / (1.0 - totalTestLikelihood); + comp->fgRemoveRefPred(oldEdge); + + // Decrement the likelihood on the old edge, so if other cases are sharing it, + // they get the right values later. + // + oldEdge->setLikelihood(edgeLikelihood - caseLikelihood); if (targetBlock == followingBB) { @@ -1064,12 +1098,22 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // We need a block to put in the new compare and/or branch. // If we haven't used the afterDefaultCondBlock yet, then use that. + // if (fUsedAfterDefaultCondBlock) { BasicBlock* newBlock = comp->fgNewBBafter(BBJ_ALWAYS, currentBlock, true); newBlock->SetFlags(BBF_NONE_QUIRK); FlowEdge* const falseEdge = comp->fgAddRefPred(newBlock, currentBlock); // The fall-through predecessor. + + // We set the true edge likelihood earlier, use that to figure out the false edge likelihood + // and the block weight. + // + FlowEdge* const trueEdge = newBlock->GetTrueEdge(); + weight_t const falseLikelihood = 1.0 - trueEdge->getLikelihood(); + falseEdge->setLikelihood(falseLikelihood); currentBlock->SetFalseEdge(falseEdge); + newBlock->inheritWeight(currentBlock); + newBlock->scaleBBWeight(falseLikelihood); currentBlock = newBlock; currentBBRange = &LIR::AsRange(currentBlock); } @@ -1079,8 +1123,11 @@ GenTree* Lowering::LowerSwitch(GenTree* node) fUsedAfterDefaultCondBlock = true; } + // Update the total test case likelihood. + totalTestLikelihood += caseLikelihood; + // Wire up the predecessor list for the "branch" case. - FlowEdge* const newEdge = comp->fgAddRefPred(targetBlock, currentBlock, jumpTab[i]); + FlowEdge* const newEdge = comp->fgAddRefPred(targetBlock, currentBlock, oldEdge); if (!fAnyTargetFollows && (i == jumpCnt - 2)) { @@ -1097,6 +1144,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // condition statement. // We will set the false edge in a later iteration of the loop, or after. currentBlock->SetCond(newEdge); + newEdge->setLikelihood(adjustedCaseLikelihood); // Now, build the conditional statement for the current case that is // being evaluated: @@ -1120,6 +1168,12 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // In this case, we need to add one back. FlowEdge* const falseEdge = comp->fgAddRefPred(currentBlock->Next(), currentBlock); currentBlock->SetFalseEdge(falseEdge); + FlowEdge* const trueEdge = currentBlock->GetTrueEdge(); + weight_t const falseLikelihood = 1.0 - trueEdge->getLikelihood(); + falseEdge->setLikelihood(falseLikelihood); + + // The following block weight should remain unchanged. All we've done + // is alter the various paths that can reach it. } if (!fUsedAfterDefaultCondBlock) @@ -1145,9 +1199,14 @@ GenTree* Lowering::LowerSwitch(GenTree* node) LIR::Range& switchBlockRange = LIR::AsRange(afterDefaultCondBlock); switchBlockRange.InsertAtEnd(switchValue); + // We are going to modify the switch, invalidate any desc map. + // + comp->fgInvalidateSwitchDescMapEntry(afterDefaultCondBlock); + // Try generating a bit test based switch first, // if that's not possible a jump table based switch will be generated. - if (!TryLowerSwitchToBitTest(jumpTab, jumpCnt, targetCnt, afterDefaultCondBlock, switchValue)) + if (!TryLowerSwitchToBitTest(jumpTab, jumpCnt, targetCnt, afterDefaultCondBlock, switchValue, + defaultLikelihood)) { JITDUMP("Lowering switch " FMT_BB ": using jump table expansion\n", originalSwitchBB->bbNum); @@ -1167,9 +1226,49 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // this block no longer branches to the default block afterDefaultCondBlock->GetSwitchTargets()->removeDefault(); - } - comp->fgInvalidateSwitchDescMapEntry(afterDefaultCondBlock); + // We need to scale up the likelihood of the remaining switch edges, now that we've peeled off + // the default case. But if the remaining likelihood is zero (default likelihood was 1.0), + // we don't know the case likelihoods. Instead, divide likelihood evenly among all cases. + // + // First, rebuild the unique succ set + // + Compiler::SwitchUniqueSuccSet successors = comp->GetDescriptorForSwitch(afterDefaultCondBlock); + + // Then fix each successor edge + // + if (Compiler::fgProfileWeightsEqual(defaultLikelihood, 1.0, 0.001)) + { + JITDUMP("Zero weight switch block " FMT_BB ", distributing likelihoods equally per case\n", + afterDefaultCondBlock->bbNum); + // jumpCnt-1 here because we peeled the default after copying this value. + weight_t const newLikelihood = 1.0 / (jumpCnt - 1); + for (unsigned i = 0; i < successors.numDistinctSuccs; i++) + { + FlowEdge* const edge = successors.nonDuplicates[i]; + edge->setLikelihood(newLikelihood * edge->getDupCount()); + } + } + else + { + weight_t const scaleFactor = 1.0 / (1.0 - defaultLikelihood); + JITDUMP("Scaling switch block " FMT_BB " likelihoods by " FMT_WT "\n", afterDefaultCondBlock->bbNum, + scaleFactor); + for (unsigned i = 0; i < successors.numDistinctSuccs; i++) + { + FlowEdge* const edge = successors.nonDuplicates[i]; + weight_t newLikelihood = scaleFactor * edge->getLikelihood(); + + if (newLikelihood > 1.0) + { + // tolerate small overflows + assert(Compiler::fgProfileWeightsEqual(newLikelihood, 1.0, 0.001)); + newLikelihood = 1.0; + } + edge->setLikelihood(newLikelihood); + } + } + } } GenTree* next = node->gtNext; @@ -1190,6 +1289,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // targetCount - The number of distinct blocks in the jump table // bbSwitch - The switch block // switchValue - A LclVar node that provides the switch value +// defaultLikelihood - likelihood control flow took the default case (already checked) // // Return value: // true if the switch has been lowered to a bit test @@ -1210,8 +1310,12 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // than the traditional jump table base code. And of course, it also avoids the need // to emit the jump table itself that can reach up to 256 bytes (for 64 entries). // -bool Lowering::TryLowerSwitchToBitTest( - FlowEdge* jumpTable[], unsigned jumpCount, unsigned targetCount, BasicBlock* bbSwitch, GenTree* switchValue) +bool Lowering::TryLowerSwitchToBitTest(FlowEdge* jumpTable[], + unsigned jumpCount, + unsigned targetCount, + BasicBlock* bbSwitch, + GenTree* switchValue, + weight_t defaultLikelihood) { assert(jumpCount >= 2); assert(targetCount >= 2); @@ -1285,6 +1389,8 @@ bool Lowering::TryLowerSwitchToBitTest( return false; } + JITDUMP("Lowering switch " FMT_BB " to bit test\n", bbSwitch->bbNum); + #if defined(TARGET_64BIT) && defined(TARGET_XARCH) // // See if we can avoid a 8 byte immediate on 64 bit targets. If all upper 32 bits are 1 @@ -1309,9 +1415,27 @@ bool Lowering::TryLowerSwitchToBitTest( comp->fgRemoveAllRefPreds(bbCase1, bbSwitch); comp->fgRemoveAllRefPreds(bbCase0, bbSwitch); - // TODO: Use old edges to influence new edge likelihoods? - case0Edge = comp->fgAddRefPred(bbCase0, bbSwitch); - case1Edge = comp->fgAddRefPred(bbCase1, bbSwitch); + case0Edge = comp->fgAddRefPred(bbCase0, bbSwitch, case0Edge); + case1Edge = comp->fgAddRefPred(bbCase1, bbSwitch, case1Edge); + + // If defaultLikelihood is not ~ 1.0 + // up-scale case likelihoods by 1.0 / (1.0 - defaultLikelihood) + // else switch block weight should be zero + // edge likelihoods are unknown, use 0.5 + // + bool const likelyToReachSwitch = !Compiler::fgProfileWeightsEqual(defaultLikelihood, 1.0, 0.001); + + if (likelyToReachSwitch) + { + weight_t const scaleFactor = 1.0 / (1.0 - defaultLikelihood); + case0Edge->setLikelihood(min(1.0, scaleFactor * case0Edge->getLikelihood())); + case1Edge->setLikelihood(min(1.0, scaleFactor * case1Edge->getLikelihood())); + } + else + { + case0Edge->setLikelihood(0.5); + case1Edge->setLikelihood(0.5); + } if (bbSwitch->NextIs(bbCase0)) { diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 6fd7b2e2ceb53d..76124820944f3c 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -351,8 +351,12 @@ class Lowering final : public Phase void TryRetypingFloatingPointStoreToIntegerStore(GenTree* store); GenTree* LowerSwitch(GenTree* node); - bool TryLowerSwitchToBitTest( - FlowEdge* jumpTable[], unsigned jumpCount, unsigned targetCount, BasicBlock* bbSwitch, GenTree* switchValue); + bool TryLowerSwitchToBitTest(FlowEdge* jumpTable[], + unsigned jumpCount, + unsigned targetCount, + BasicBlock* bbSwitch, + GenTree* switchValue, + weight_t defaultLikelihood); void LowerCast(GenTree* node); From f73b801dadf95260e85056f26fc5f847c176850a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 12 Mar 2024 20:06:31 -0700 Subject: [PATCH 2/3] review feedback, fix edge sharing issue --- src/coreclr/jit/lower.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 3225fa4f0ec675..ea0d0d79b23aab 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -973,7 +973,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // Turn originalSwitchBB into a BBJ_COND. FlowEdge* const falseEdge = originalSwitchBB->GetTargetEdge(); weight_t const switchLikelihood = 1.0 - defaultLikelihood; - falseEdge->setLikelihood(1.0 - defaultLikelihood); + falseEdge->setLikelihood(switchLikelihood); originalSwitchBB->SetCond(trueEdge, falseEdge); afterDefaultCondBlock->inheritWeight(originalSwitchBB); afterDefaultCondBlock->scaleBBWeight(switchLikelihood); @@ -1059,7 +1059,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // unconditional branch. bool fAnyTargetFollows = false; - // We need to track how much of hte original switch's likelihood has already been + // We need to track how much of the original switch's likelihood has already been // tested for. We'll use this to adjust the likelihood of the branches we're adding. // So far we've tested for the default case, so we'll start with that. weight_t totalTestLikelihood = defaultLikelihood; @@ -1108,7 +1108,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // We set the true edge likelihood earlier, use that to figure out the false edge likelihood // and the block weight. // - FlowEdge* const trueEdge = newBlock->GetTrueEdge(); + FlowEdge* const trueEdge = currentBlock->GetTrueEdge(); weight_t const falseLikelihood = 1.0 - trueEdge->getLikelihood(); falseEdge->setLikelihood(falseLikelihood); currentBlock->SetFalseEdge(falseEdge); @@ -1120,6 +1120,23 @@ GenTree* Lowering::LowerSwitch(GenTree* node) else { assert(currentBlock == afterDefaultCondBlock); + + // If the first switch case we peel off has the same target as + // other cases (that is, it has nonzero dup count, it's simpler to + // just make a new here block, so that as we peel off cases, + // we're not sharing edges with the original switch. + // + // That is, the call to fgAddRefPred below always creates a new edge. + // + if (oldEdge->getDupCount() > 0) + { + BasicBlock* const newBlock = comp->fgNewBBafter(BBJ_ALWAYS, currentBlock, true); + newBlock->SetFlags(BBF_NONE_QUIRK); + FlowEdge* const newEdge = comp->fgAddRefPred(newBlock, currentBlock); + currentBlock = newBlock; + afterDefaultCondBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + } + fUsedAfterDefaultCondBlock = true; } @@ -1128,6 +1145,8 @@ GenTree* Lowering::LowerSwitch(GenTree* node) // Wire up the predecessor list for the "branch" case. FlowEdge* const newEdge = comp->fgAddRefPred(targetBlock, currentBlock, oldEdge); + // This should truly be a new edge. + assert(newEdge->getDupCount() == 1); if (!fAnyTargetFollows && (i == jumpCnt - 2)) { From 4107a07fe7560d6d3f82a699d7b481302419e923 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 13 Mar 2024 09:42:25 -0700 Subject: [PATCH 3/3] Needed to reset range as well. Also, add some overflow protection. --- src/coreclr/jit/lower.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index ea0d0d79b23aab..591c2b9165879c 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1081,7 +1081,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) weight_t const caseLikelihood = edgeLikelihood / oldEdge->getDupCount(); bool const unlikelyToReachThisCase = Compiler::fgProfileWeightsEqual(totalTestLikelihood, 1.0, 0.001); weight_t const adjustedCaseLikelihood = - unlikelyToReachThisCase ? 0.5 : caseLikelihood / (1.0 - totalTestLikelihood); + unlikelyToReachThisCase ? 0.5 : min(1.0, caseLikelihood / (1.0 - totalTestLikelihood)); comp->fgRemoveRefPred(oldEdge); // Decrement the likelihood on the old edge, so if other cases are sharing it, @@ -1134,6 +1134,7 @@ GenTree* Lowering::LowerSwitch(GenTree* node) newBlock->SetFlags(BBF_NONE_QUIRK); FlowEdge* const newEdge = comp->fgAddRefPred(newBlock, currentBlock); currentBlock = newBlock; + currentBBRange = &LIR::AsRange(currentBlock); afterDefaultCondBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); }