From e13c6ce5832320a574c5754592e56b6293d75627 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 30 Jan 2025 16:52:13 -0500 Subject: [PATCH 1/5] Remove fgMoveHotJumps --- src/coreclr/jit/compiler.h | 3 - src/coreclr/jit/fgopt.cpp | 203 ------------------------------------- 2 files changed, 206 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4a639c7e639f9e..b388c6c5a465bc 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6348,9 +6348,6 @@ class Compiler void Run(); }; - template - void fgMoveHotJumps(); - bool fgFuncletsAreCold(); PhaseStatus fgDetermineFirstColdBlock(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 244d8bcd9efc1f..170fcb2f43ec2a 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4445,205 +4445,6 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif -//----------------------------------------------------------------------------- -// fgMoveHotJumps: Try to move jumps to fall into their successors, if the jump is sufficiently hot. -// -// Template parameters: -// hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions -// -template -void Compiler::fgMoveHotJumps() -{ -#ifdef DEBUG - if (verbose) - { - printf("*************** In fgMoveHotJumps()\n"); - - printf("\nInitial BasicBlocks"); - fgDispBasicBlocks(verboseTrees); - printf("\n"); - } -#endif // DEBUG - - assert(m_dfsTree != nullptr); - BitVecTraits traits(m_dfsTree->PostOrderTraits()); - BitVec visitedBlocks = BitVecOps::MakeEmpty(&traits); - - // If we have a funclet region, don't bother reordering anything in it. - // - BasicBlock* next; - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = next) - { - next = block->Next(); - if (!m_dfsTree->Contains(block)) - { - continue; - } - - BitVecOps::AddElemD(&traits, visitedBlocks, block->bbPostorderNum); - - // Don't bother trying to move cold blocks - // - if (block->isBBWeightCold(this)) - { - continue; - } - - FlowEdge* targetEdge; - FlowEdge* unlikelyEdge; - - if (block->KindIs(BBJ_ALWAYS)) - { - targetEdge = block->GetTargetEdge(); - unlikelyEdge = nullptr; - } - else if (block->KindIs(BBJ_COND)) - { - // Consider conditional block's most likely branch for moving - // - if (block->GetTrueEdge()->getLikelihood() > 0.5) - { - targetEdge = block->GetTrueEdge(); - unlikelyEdge = block->GetFalseEdge(); - } - else - { - targetEdge = block->GetFalseEdge(); - unlikelyEdge = block->GetTrueEdge(); - } - - // If we aren't sure which successor is hotter, and we already fall into one of them, - // do nothing - if ((unlikelyEdge->getLikelihood() == 0.5) && block->NextIs(unlikelyEdge->getDestinationBlock())) - { - continue; - } - } - else - { - // Don't consider other block kinds - // - continue; - } - - BasicBlock* target = targetEdge->getDestinationBlock(); - bool isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); - assert(m_dfsTree->Contains(target)); - - if (isBackwardJump) - { - // We don't want to change the first block, so if block is a backward jump to the first block, - // don't try moving block before it. - // - if (target->IsFirst()) - { - continue; - } - - if (block->KindIs(BBJ_COND)) - { - // This could be a loop exit, so don't bother moving this block up. - // Instead, try moving the unlikely target up to create fallthrough. - // - targetEdge = unlikelyEdge; - target = targetEdge->getDestinationBlock(); - isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); - assert(m_dfsTree->Contains(target)); - - if (isBackwardJump) - { - continue; - } - } - // Check for single-block loop case - // - else if (block == target) - { - continue; - } - } - - // Check if block already falls into target - // - if (block->NextIs(target)) - { - continue; - } - - if (target->isBBWeightCold(this)) - { - // If target is block's most-likely successor, and block is not rarely-run, - // perhaps the profile data is misleading, and we need to run profile repair? - // - continue; - } - - if (hasEH) - { - // Don't move blocks in different EH regions - // - if (!BasicBlock::sameEHRegion(block, target)) - { - continue; - } - - if (isBackwardJump) - { - // block and target are in the same try/handler regions, and target is behind block, - // so block cannot possibly be the start of the region. - // - assert(!bbIsTryBeg(block) && !bbIsHandlerBeg(block)); - - // Don't change the entry block of an EH region - // - if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) - { - continue; - } - } - else - { - // block and target are in the same try/handler regions, and block is behind target, - // so target cannot possibly be the start of the region. - // - assert(!bbIsTryBeg(target) && !bbIsHandlerBeg(target)); - } - } - - // If moving block will break up existing fallthrough behavior into target, make sure it's worth it - // - FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev()); - if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= targetEdge->getLikelyWeight())) - { - continue; - } - - if (isBackwardJump) - { - // Move block to before target - // - fgUnlinkBlock(block); - fgInsertBBbefore(target, block); - } - else if (hasEH && target->isBBCallFinallyPair()) - { - // target is a call-finally pair, so move the pair up to block - // - fgUnlinkRange(target, target->Next()); - fgMoveBlocksAfter(target, target->Next(), block); - next = target->Next(); - } - else - { - // Move target up to block - // - fgUnlinkBlock(target); - fgInsertBBafter(block, target); - next = target; - } - } -} - //----------------------------------------------------------------------------- // fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal, // taking care to keep loop bodies compact. @@ -4699,8 +4500,6 @@ void Compiler::fgDoReversePostOrderLayout() } } - fgMoveHotJumps(); - return; } @@ -4771,8 +4570,6 @@ void Compiler::fgDoReversePostOrderLayout() fgUnlinkBlock(pair.callFinallyRet); fgInsertBBafter(pair.callFinally, pair.callFinallyRet); } - - fgMoveHotJumps(); } //----------------------------------------------------------------------------- From d32aab12ce30cb714b2f854b4d21495697bc6e05 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 5 Feb 2025 22:01:59 -0500 Subject: [PATCH 2/5] Revert "Remove fgMoveHotJumps" This reverts commit e13c6ce5832320a574c5754592e56b6293d75627. --- src/coreclr/jit/compiler.h | 3 + src/coreclr/jit/fgopt.cpp | 203 +++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b388c6c5a465bc..4a639c7e639f9e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6348,6 +6348,9 @@ class Compiler void Run(); }; + template + void fgMoveHotJumps(); + bool fgFuncletsAreCold(); PhaseStatus fgDetermineFirstColdBlock(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 170fcb2f43ec2a..244d8bcd9efc1f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4445,6 +4445,205 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif +//----------------------------------------------------------------------------- +// fgMoveHotJumps: Try to move jumps to fall into their successors, if the jump is sufficiently hot. +// +// Template parameters: +// hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions +// +template +void Compiler::fgMoveHotJumps() +{ +#ifdef DEBUG + if (verbose) + { + printf("*************** In fgMoveHotJumps()\n"); + + printf("\nInitial BasicBlocks"); + fgDispBasicBlocks(verboseTrees); + printf("\n"); + } +#endif // DEBUG + + assert(m_dfsTree != nullptr); + BitVecTraits traits(m_dfsTree->PostOrderTraits()); + BitVec visitedBlocks = BitVecOps::MakeEmpty(&traits); + + // If we have a funclet region, don't bother reordering anything in it. + // + BasicBlock* next; + for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = next) + { + next = block->Next(); + if (!m_dfsTree->Contains(block)) + { + continue; + } + + BitVecOps::AddElemD(&traits, visitedBlocks, block->bbPostorderNum); + + // Don't bother trying to move cold blocks + // + if (block->isBBWeightCold(this)) + { + continue; + } + + FlowEdge* targetEdge; + FlowEdge* unlikelyEdge; + + if (block->KindIs(BBJ_ALWAYS)) + { + targetEdge = block->GetTargetEdge(); + unlikelyEdge = nullptr; + } + else if (block->KindIs(BBJ_COND)) + { + // Consider conditional block's most likely branch for moving + // + if (block->GetTrueEdge()->getLikelihood() > 0.5) + { + targetEdge = block->GetTrueEdge(); + unlikelyEdge = block->GetFalseEdge(); + } + else + { + targetEdge = block->GetFalseEdge(); + unlikelyEdge = block->GetTrueEdge(); + } + + // If we aren't sure which successor is hotter, and we already fall into one of them, + // do nothing + if ((unlikelyEdge->getLikelihood() == 0.5) && block->NextIs(unlikelyEdge->getDestinationBlock())) + { + continue; + } + } + else + { + // Don't consider other block kinds + // + continue; + } + + BasicBlock* target = targetEdge->getDestinationBlock(); + bool isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); + assert(m_dfsTree->Contains(target)); + + if (isBackwardJump) + { + // We don't want to change the first block, so if block is a backward jump to the first block, + // don't try moving block before it. + // + if (target->IsFirst()) + { + continue; + } + + if (block->KindIs(BBJ_COND)) + { + // This could be a loop exit, so don't bother moving this block up. + // Instead, try moving the unlikely target up to create fallthrough. + // + targetEdge = unlikelyEdge; + target = targetEdge->getDestinationBlock(); + isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); + assert(m_dfsTree->Contains(target)); + + if (isBackwardJump) + { + continue; + } + } + // Check for single-block loop case + // + else if (block == target) + { + continue; + } + } + + // Check if block already falls into target + // + if (block->NextIs(target)) + { + continue; + } + + if (target->isBBWeightCold(this)) + { + // If target is block's most-likely successor, and block is not rarely-run, + // perhaps the profile data is misleading, and we need to run profile repair? + // + continue; + } + + if (hasEH) + { + // Don't move blocks in different EH regions + // + if (!BasicBlock::sameEHRegion(block, target)) + { + continue; + } + + if (isBackwardJump) + { + // block and target are in the same try/handler regions, and target is behind block, + // so block cannot possibly be the start of the region. + // + assert(!bbIsTryBeg(block) && !bbIsHandlerBeg(block)); + + // Don't change the entry block of an EH region + // + if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) + { + continue; + } + } + else + { + // block and target are in the same try/handler regions, and block is behind target, + // so target cannot possibly be the start of the region. + // + assert(!bbIsTryBeg(target) && !bbIsHandlerBeg(target)); + } + } + + // If moving block will break up existing fallthrough behavior into target, make sure it's worth it + // + FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev()); + if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= targetEdge->getLikelyWeight())) + { + continue; + } + + if (isBackwardJump) + { + // Move block to before target + // + fgUnlinkBlock(block); + fgInsertBBbefore(target, block); + } + else if (hasEH && target->isBBCallFinallyPair()) + { + // target is a call-finally pair, so move the pair up to block + // + fgUnlinkRange(target, target->Next()); + fgMoveBlocksAfter(target, target->Next(), block); + next = target->Next(); + } + else + { + // Move target up to block + // + fgUnlinkBlock(target); + fgInsertBBafter(block, target); + next = target; + } + } +} + //----------------------------------------------------------------------------- // fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal, // taking care to keep loop bodies compact. @@ -4500,6 +4699,8 @@ void Compiler::fgDoReversePostOrderLayout() } } + fgMoveHotJumps(); + return; } @@ -4570,6 +4771,8 @@ void Compiler::fgDoReversePostOrderLayout() fgUnlinkBlock(pair.callFinallyRet); fgInsertBBafter(pair.callFinally, pair.callFinallyRet); } + + fgMoveHotJumps(); } //----------------------------------------------------------------------------- From a90c994146b4b621dd0838f2fc1d270c8d5b8759 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 6 Feb 2025 00:40:23 -0500 Subject: [PATCH 3/5] Add ThreeOptLayout::CompactHotJumps --- src/coreclr/jit/compiler.h | 6 +- src/coreclr/jit/fgopt.cpp | 155 +++++++++++++++++++++++++++++++------ 2 files changed, 136 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6a7298cf3d2373..6f42d59455de35 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6360,14 +6360,16 @@ class Compiler #endif // DEBUG weight_t GetCost(BasicBlock* block, BasicBlock* next); - weight_t GetPartitionCostDelta(unsigned s1Start, unsigned s2Start, unsigned s3Start, unsigned s3End, unsigned s4End); + weight_t GetPartitionCostDelta(unsigned s2Start, unsigned s3Start, unsigned s3End, unsigned s4End); void SwapPartitions(unsigned s1Start, unsigned s2Start, unsigned s3Start, unsigned s3End, unsigned s4End); - void ConsiderEdge(FlowEdge* edge); + template + bool ConsiderEdge(FlowEdge* edge); void AddNonFallthroughSuccs(unsigned blockPos); void AddNonFallthroughPreds(unsigned blockPos); bool RunGreedyThreeOptPass(unsigned startPos, unsigned endPos); + bool CompactHotJumps(); bool RunThreeOptPass(); public: diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 8c8376694281f6..acc0d8678cee64 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4456,6 +4456,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) template void Compiler::fgMoveHotJumps() { + return; #ifdef DEBUG if (verbose) { @@ -4998,7 +4999,6 @@ weight_t Compiler::ThreeOptLayout::GetCost(BasicBlock* block, BasicBlock* next) // and the cost of swapping S2 and S3, returning the difference between them. // // Parameters: -// s1Start - The starting position of the first partition // s2Start - The starting position of the second partition // s3Start - The starting position of the third partition // s3End - The ending position (inclusive) of the third partition @@ -5008,8 +5008,10 @@ weight_t Compiler::ThreeOptLayout::GetCost(BasicBlock* block, BasicBlock* next) // The difference in cost between the current and proposed layouts. // A negative delta indicates the proposed layout is an improvement. // -weight_t Compiler::ThreeOptLayout::GetPartitionCostDelta( - unsigned s1Start, unsigned s2Start, unsigned s3Start, unsigned s3End, unsigned s4End) +weight_t Compiler::ThreeOptLayout::GetPartitionCostDelta(unsigned s2Start, + unsigned s3Start, + unsigned s3End, + unsigned s4End) { BasicBlock* const s2Block = blockOrder[s2Start]; BasicBlock* const s2BlockPrev = blockOrder[s2Start - 1]; @@ -5088,6 +5090,12 @@ void Compiler::ThreeOptLayout::SwapPartitions( std::swap(blockOrder, tempOrder); + // Update the ordinals for the blocks we moved + for (unsigned i = s2Start; i <= s4End; i++) + { + blockOrder[i]->bbPostorderNum = i; + } + #ifdef DEBUG // Don't bother checking if the cost improved for exceptionally costly layouts. // Imprecision from summing large floating-point values can falsely trigger the below assert. @@ -5111,15 +5119,22 @@ void Compiler::ThreeOptLayout::SwapPartitions( // Parameters: // edge - The branch to consider creating fallthrough for // -void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) +// Template parameters: +// addToQueue - If true, adds valid edges to the 'cutPoints' queue +// +// Returns: +// True if 'edge' can be considered for aligning, false otherwise +// +template +bool Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) { assert(edge != nullptr); // Don't add an edge that we've already considered // (For exceptionally branchy methods, we want to avoid exploding 'cutPoints' in size) - if (edge->visited()) + if (addToQueue && edge->visited()) { - return; + return false; } BasicBlock* const srcBlk = edge->getSourceBlock(); @@ -5128,7 +5143,7 @@ void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) // Ignore cross-region branches if (!BasicBlock::sameTryRegion(srcBlk, dstBlk)) { - return; + return false; } // Don't waste time reordering within handler regions. @@ -5136,7 +5151,7 @@ void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) // we should have cloned it into the main method body already. if (srcBlk->hasHndIndex() || dstBlk->hasHndIndex()) { - return; + return false; } // For backward jumps, we will consider partitioning before 'srcBlk'. @@ -5144,7 +5159,7 @@ void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) // Thus, don't consider edges out of BBJ_CALLFINALLYRET blocks. if (srcBlk->KindIs(BBJ_CALLFINALLYRET)) { - return; + return false; } const unsigned srcPos = srcBlk->bbPostorderNum; @@ -5155,29 +5170,34 @@ void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge) // Don't consider edges to or from outside the hot range (i.e. ordinal doesn't match 'blockOrder' position). if ((srcPos >= numCandidateBlocks) || (srcBlk != blockOrder[srcPos])) { - return; + return false; } if ((dstPos >= numCandidateBlocks) || (dstBlk != blockOrder[dstPos])) { - return; + return false; } // Don't consider edges to blocks outside the hot range (i.e. ordinal number isn't set), // or backedges to the first block in a region; we don't want to change the entry point. if ((dstPos == 0) || compiler->bbIsTryBeg(dstBlk)) { - return; + return false; } // Don't consider backedges for single-block loops if (srcPos == dstPos) { - return; + return false; + } + + if (addToQueue) + { + edge->markVisited(); + cutPoints.Push(edge); } - edge->markVisited(); - cutPoints.Push(edge); + return true; } //----------------------------------------------------------------------------- @@ -5273,7 +5293,8 @@ void Compiler::ThreeOptLayout::Run() block->bbPostorderNum = numCandidateBlocks++; } - const bool modified = RunThreeOptPass(); + bool modified = CompactHotJumps(); + modified |= RunThreeOptPass(); if (modified) { @@ -5398,7 +5419,7 @@ bool Compiler::ThreeOptLayout::RunGreedyThreeOptPass(unsigned startPos, unsigned s2Start = srcPos + 1; s3Start = dstPos; s3End = endPos; - costChange = GetPartitionCostDelta(startPos, s2Start, s3Start, s3End, endPos); + costChange = GetPartitionCostDelta(s2Start, s3Start, s3End, endPos); } else { @@ -5472,12 +5493,6 @@ bool Compiler::ThreeOptLayout::RunGreedyThreeOptPass(unsigned startPos, unsigned SwapPartitions(startPos, s2Start, s3Start, s3End, endPos); - // Update the ordinals for the blocks we moved - for (unsigned i = s2Start; i <= endPos; i++) - { - blockOrder[i]->bbPostorderNum = i; - } - // Ensure this move created fallthrough from 'srcBlk' to 'dstBlk' assert((srcBlk->bbPostorderNum + 1) == dstBlk->bbPostorderNum); @@ -5537,6 +5552,100 @@ bool Compiler::ThreeOptLayout::RunThreeOptPass() return modified; } +//----------------------------------------------------------------------------- +// Compiler::ThreeOptLayout::CompactHotJumps: Move blocks in the candidate span +// closer to their most-likely successors. +// +// Returns: +// True if we reordered anything, false otherwise +// +bool Compiler::ThreeOptLayout::CompactHotJumps() +{ + JITDUMP("Compacting hot jumps\n"); + bool modified = false; + + for (unsigned i = 0; i < numCandidateBlocks; i++) + { + BasicBlock* const block = blockOrder[i]; + FlowEdge* edge; + + if (block->KindIs(BBJ_ALWAYS)) + { + edge = block->GetTargetEdge(); + } + else if (block->KindIs(BBJ_COND)) + { + // Consider conditional block's most likely branch for moving + edge = (block->GetTrueEdge()->getLikelihood() > 0.5) ? block->GetTrueEdge() : block->GetFalseEdge(); + } + else + { + // Don't consider other block kinds + continue; + } + + // Ensure we won't break any ordering invariants by creating fallthrough on this edge + if (!ConsiderEdge(edge)) + { + continue; + } + + BasicBlock* const target = edge->getDestinationBlock(); + const unsigned srcPos = block->bbPostorderNum; + const unsigned dstPos = target->bbPostorderNum; + + // We don't need to do anything if this edge already falls through + if ((srcPos + 1) == dstPos) + { + continue; + } + + const bool isForwardJump = (srcPos < dstPos); + const unsigned startPos = 0; + const unsigned endPos = numCandidateBlocks - 1; + unsigned s2Start, s3Start, s3End; + + if (isForwardJump) + { + // Before swap: | ..srcBlk | ... | dstBlk | ... | + // After swap: | ..srcBlk | dstBlk | ... | + s2Start = srcPos + 1; + s3Start = dstPos; + s3End = dstPos; + } + else + { + // Before swap: | ... | dstBlk.. | srcBlk | ... | + // After swap: | ... | srcBlk | dstBlk.. | ... | + s2Start = dstPos; + s3Start = srcPos; + s3End = srcPos; + } + + // Don't align this branch if it isn't profitable + const weight_t costChange = GetPartitionCostDelta(s2Start, s3Start, s3End, endPos); + if ((costChange >= BB_ZERO_WEIGHT) || Compiler::fgProfileWeightsEqual(costChange, BB_ZERO_WEIGHT, 0.001)) + { + continue; + } + + JITDUMP("Swapping partitions [" FMT_BB ", " FMT_BB "] and [" FMT_BB ", " FMT_BB "] (cost change = %f)\n", + blockOrder[s2Start]->bbNum, blockOrder[s3Start - 1]->bbNum, blockOrder[s3Start]->bbNum, + blockOrder[s3End]->bbNum, costChange); + + SwapPartitions(startPos, s2Start, s3Start, s3End, endPos); + modified = true; + } + + // Write back to 'tempOrder' so the changes made above aren't lost next time we swap 'tempOrder' and 'blockOrder' + if (modified) + { + memcpy(tempOrder, blockOrder, sizeof(BasicBlock*) * numCandidateBlocks); + } + + return modified; +} + //----------------------------------------------------------------------------- // fgSearchImprovedLayout: Try to improve upon RPO-based layout with the 3-opt method: // - Identify a range of hot blocks to reorder within From 420c470663688ac67c04d87077a775b0d1298603 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 6 Feb 2025 00:40:53 -0500 Subject: [PATCH 4/5] Remove fgMoveHotJumps --- src/coreclr/jit/compiler.h | 3 - src/coreclr/jit/fgopt.cpp | 204 ------------------------------------- 2 files changed, 207 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6f42d59455de35..af79239f85aa89 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6377,9 +6377,6 @@ class Compiler void Run(); }; - template - void fgMoveHotJumps(); - bool fgFuncletsAreCold(); PhaseStatus fgDetermineFirstColdBlock(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index acc0d8678cee64..fb16ac704cc161 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4447,206 +4447,6 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif -//----------------------------------------------------------------------------- -// fgMoveHotJumps: Try to move jumps to fall into their successors, if the jump is sufficiently hot. -// -// Template parameters: -// hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions -// -template -void Compiler::fgMoveHotJumps() -{ - return; -#ifdef DEBUG - if (verbose) - { - printf("*************** In fgMoveHotJumps()\n"); - - printf("\nInitial BasicBlocks"); - fgDispBasicBlocks(verboseTrees); - printf("\n"); - } -#endif // DEBUG - - assert(m_dfsTree != nullptr); - BitVecTraits traits(m_dfsTree->PostOrderTraits()); - BitVec visitedBlocks = BitVecOps::MakeEmpty(&traits); - - // If we have a funclet region, don't bother reordering anything in it. - // - BasicBlock* next; - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = next) - { - next = block->Next(); - if (!m_dfsTree->Contains(block)) - { - continue; - } - - BitVecOps::AddElemD(&traits, visitedBlocks, block->bbPostorderNum); - - // Don't bother trying to move cold blocks - // - if (block->isBBWeightCold(this)) - { - continue; - } - - FlowEdge* targetEdge; - FlowEdge* unlikelyEdge; - - if (block->KindIs(BBJ_ALWAYS)) - { - targetEdge = block->GetTargetEdge(); - unlikelyEdge = nullptr; - } - else if (block->KindIs(BBJ_COND)) - { - // Consider conditional block's most likely branch for moving - // - if (block->GetTrueEdge()->getLikelihood() > 0.5) - { - targetEdge = block->GetTrueEdge(); - unlikelyEdge = block->GetFalseEdge(); - } - else - { - targetEdge = block->GetFalseEdge(); - unlikelyEdge = block->GetTrueEdge(); - } - - // If we aren't sure which successor is hotter, and we already fall into one of them, - // do nothing - if ((unlikelyEdge->getLikelihood() == 0.5) && block->NextIs(unlikelyEdge->getDestinationBlock())) - { - continue; - } - } - else - { - // Don't consider other block kinds - // - continue; - } - - BasicBlock* target = targetEdge->getDestinationBlock(); - bool isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); - assert(m_dfsTree->Contains(target)); - - if (isBackwardJump) - { - // We don't want to change the first block, so if block is a backward jump to the first block, - // don't try moving block before it. - // - if (target->IsFirst()) - { - continue; - } - - if (block->KindIs(BBJ_COND)) - { - // This could be a loop exit, so don't bother moving this block up. - // Instead, try moving the unlikely target up to create fallthrough. - // - targetEdge = unlikelyEdge; - target = targetEdge->getDestinationBlock(); - isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum); - assert(m_dfsTree->Contains(target)); - - if (isBackwardJump) - { - continue; - } - } - // Check for single-block loop case - // - else if (block == target) - { - continue; - } - } - - // Check if block already falls into target - // - if (block->NextIs(target)) - { - continue; - } - - if (target->isBBWeightCold(this)) - { - // If target is block's most-likely successor, and block is not rarely-run, - // perhaps the profile data is misleading, and we need to run profile repair? - // - continue; - } - - if (hasEH) - { - // Don't move blocks in different EH regions - // - if (!BasicBlock::sameEHRegion(block, target)) - { - continue; - } - - if (isBackwardJump) - { - // block and target are in the same try/handler regions, and target is behind block, - // so block cannot possibly be the start of the region. - // - assert(!bbIsTryBeg(block) && !bbIsHandlerBeg(block)); - - // Don't change the entry block of an EH region - // - if (bbIsTryBeg(target) || bbIsHandlerBeg(target)) - { - continue; - } - } - else - { - // block and target are in the same try/handler regions, and block is behind target, - // so target cannot possibly be the start of the region. - // - assert(!bbIsTryBeg(target) && !bbIsHandlerBeg(target)); - } - } - - // If moving block will break up existing fallthrough behavior into target, make sure it's worth it - // - FlowEdge* const fallthroughEdge = fgGetPredForBlock(target, target->Prev()); - if ((fallthroughEdge != nullptr) && (fallthroughEdge->getLikelyWeight() >= targetEdge->getLikelyWeight())) - { - continue; - } - - if (isBackwardJump) - { - // Move block to before target - // - fgUnlinkBlock(block); - fgInsertBBbefore(target, block); - } - else if (hasEH && target->isBBCallFinallyPair()) - { - // target is a call-finally pair, so move the pair up to block - // - fgUnlinkRange(target, target->Next()); - fgMoveBlocksAfter(target, target->Next(), block); - next = target->Next(); - } - else - { - // Move target up to block - // - fgUnlinkBlock(target); - fgInsertBBafter(block, target); - next = target; - } - } -} - //----------------------------------------------------------------------------- // fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal, // taking care to keep loop bodies compact. @@ -4702,8 +4502,6 @@ void Compiler::fgDoReversePostOrderLayout() } } - fgMoveHotJumps(); - return; } @@ -4774,8 +4572,6 @@ void Compiler::fgDoReversePostOrderLayout() fgUnlinkBlock(pair.callFinallyRet); fgInsertBBafter(pair.callFinally, pair.callFinallyRet); } - - fgMoveHotJumps(); } //----------------------------------------------------------------------------- From cfd4a893fe452884ada82d61abed70155ad3ca69 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 6 Feb 2025 10:56:58 -0500 Subject: [PATCH 5/5] Don't break up call-finally pairs --- src/coreclr/jit/fgopt.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index fb16ac704cc161..d7b23fc7d50c0f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5357,6 +5357,11 @@ bool Compiler::ThreeOptLayout::RunThreeOptPass() // bool Compiler::ThreeOptLayout::CompactHotJumps() { + if (!compiler->m_dfsTree->HasCycle()) + { + return false; + } + JITDUMP("Compacting hot jumps\n"); bool modified = false; @@ -5408,6 +5413,12 @@ bool Compiler::ThreeOptLayout::CompactHotJumps() s2Start = srcPos + 1; s3Start = dstPos; s3End = dstPos; + + // Call-finally pairs need to stick together, so include the tail in the partition + if (target->isBBCallFinallyPair()) + { + s3End++; + } } else {