From f58556dce0355e4aa2e6a2b50b5306c5326fc3f2 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sun, 21 Apr 2024 19:25:38 -0400 Subject: [PATCH 01/16] WIP: Start greedy RPO layout --- src/coreclr/jit/block.cpp | 38 ++++++++- src/coreclr/jit/block.h | 47 ++++++++++- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/compiler.hpp | 32 ++++++-- src/coreclr/jit/fgopt.cpp | 146 +++++++++++++++++++++++++++++++++- src/coreclr/jit/flowgraph.cpp | 11 ++- 6 files changed, 263 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 60dbce6aaf00a..cd0175b8a3b3a 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -140,7 +140,7 @@ void FlowEdge::addLikelihood(weight_t addedLikelihood) // comp - Compiler instance // block - The block whose successors are to be iterated // -AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block) +AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile) : m_block(block) { m_numSuccs = 0; @@ -169,6 +169,42 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block } } +//------------------------------------------------------------------------ +// RegularSuccessorEnumerator: Construct an instance of the enumerator. +// +// Arguments: +// comp - Compiler instance +// block - The block whose successors are to be iterated +// +RegularSuccessorEnumerator::RegularSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile) + : m_block(block) +{ + m_numSuccs = 0; + block->VisitRegularSuccs(comp, [this](BasicBlock* succ) { + if (m_numSuccs < ArrLen(m_successors)) + { + m_successors[m_numSuccs] = succ; + } + + m_numSuccs++; + return BasicBlockVisit::Continue; + }, useProfile); + + if (m_numSuccs > ArrLen(m_successors)) + { + m_pSuccessors = new (comp, CMK_BasicBlock) BasicBlock*[m_numSuccs]; + + unsigned numSuccs = 0; + block->VisitRegularSuccs(comp, [this, &numSuccs](BasicBlock* succ) { + assert(numSuccs < m_numSuccs); + m_pSuccessors[numSuccs++] = succ; + return BasicBlockVisit::Continue; + }, useProfile); + + assert(numSuccs == m_numSuccs); + } +} + //------------------------------------------------------------------------ // BlockPredsWithEH: // Return list of predecessors, including due to EH flow. This is logically diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 168f29cca084d..302d6e10f1707 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1826,7 +1826,7 @@ struct BasicBlock : private LIR::Range BasicBlockVisit VisitEHSuccs(Compiler* comp, TFunc func); template - BasicBlockVisit VisitRegularSuccs(Compiler* comp, TFunc func); + BasicBlockVisit VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile = false); bool HasPotentialEHSuccs(Compiler* comp); @@ -2518,7 +2518,50 @@ class AllSuccessorEnumerator public: // Constructs an enumerator of all `block`'s successors. - AllSuccessorEnumerator(Compiler* comp, BasicBlock* block); + AllSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile = false); + + // Gets the block whose successors are enumerated. + BasicBlock* Block() + { + return m_block; + } + + // Returns the next available successor or `nullptr` if there are no more successors. + BasicBlock* NextSuccessor() + { + m_curSucc++; + if (m_curSucc >= m_numSuccs) + { + return nullptr; + } + + if (m_numSuccs <= ArrLen(m_successors)) + { + return m_successors[m_curSucc]; + } + + return m_pSuccessors[m_curSucc]; + } +}; + +// An enumerator of a block's non-EH successors. Used for RPO traversals that exclude EH regions. +class RegularSuccessorEnumerator +{ + BasicBlock* m_block; + union + { + // We store up to 4 successors inline in the enumerator. For ASP.NET + // and libraries.pmi this is enough in 99.7% of cases. + BasicBlock* m_successors[4]; + BasicBlock** m_pSuccessors; + }; + + unsigned m_numSuccs; + unsigned m_curSucc = UINT_MAX; + +public: + // Constructs an enumerator of `block`'s regular successors. + RegularSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile); // Gets the block whose successors are enumerated. BasicBlock* Block() diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d0dd558f2e385..15c3d0740614f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6067,9 +6067,10 @@ class Compiler PhaseStatus fgSetBlockOrder(); bool fgHasCycleWithoutGCSafePoint(); - template + template unsigned fgRunDfs(VisitPreorder assignPreorder, VisitPostorder assignPostorder, VisitEdge visitEdge); + template FlowGraphDfsTree* fgComputeDfs(); void fgInvalidateDfsTree(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 022a92bd740a3..e1c67afc56532 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -703,7 +703,7 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) // Whether or not the visiting was aborted. // template -BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) +BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile) { switch (bbKind) { @@ -721,6 +721,14 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) return BasicBlockVisit::Continue; case BBJ_CALLFINALLY: + if (useProfile && isBBCallFinallyPair()) + { + return func(Next()); + } + else + { + return func(GetTarget()); + } case BBJ_CALLFINALLYRET: case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: @@ -729,10 +737,18 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) return func(GetTarget()); case BBJ_COND: - RETURN_ON_ABORT(func(GetFalseTarget())); - - if (!TrueEdgeIs(GetFalseEdge())) + if (TrueEdgeIs(GetFalseEdge())) + { + RETURN_ON_ABORT(func(GetFalseTarget())); + } + else if (useProfile && (GetTrueEdge()->getLikelihood() < GetFalseEdge()->getLikelihood())) + { + RETURN_ON_ABORT(func(GetTrueTarget())); + RETURN_ON_ABORT(func(GetFalseTarget())); + } + else { + RETURN_ON_ABORT(func(GetFalseTarget())); RETURN_ON_ABORT(func(GetTrueTarget())); } @@ -4755,7 +4771,7 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // Returns: // Number of blocks visited. // -template +template unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { BitVecTraits traits(fgBBNumMax + 1, this); @@ -4764,11 +4780,11 @@ unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPos unsigned preOrderIndex = 0; unsigned postOrderIndex = 0; - ArrayStack blocks(getAllocator(CMK_DepthFirstSearch)); + ArrayStack blocks(getAllocator(CMK_DepthFirstSearch)); auto dfsFrom = [&](BasicBlock* firstBB) { BitVecOps::AddElemD(&traits, visited, firstBB->bbNum); - blocks.Emplace(this, firstBB); + blocks.Emplace(this, firstBB, useProfile); visitPreorder(firstBB, preOrderIndex++); while (!blocks.Empty()) @@ -4780,7 +4796,7 @@ unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPos { if (BitVecOps::TryAddElemD(&traits, visited, succ->bbNum)) { - blocks.Emplace(this, succ); + blocks.Emplace(this, succ, useProfile); visitPreorder(succ, preOrderIndex++); } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c1ef6f1df18dd..a1622ea1fc6c0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3397,9 +3397,153 @@ bool Compiler::fgReorderBlocks(bool useProfile) } } - // If we will be reordering blocks, ensure the false target of a BBJ_COND block is its next block if (useProfile) { + if (compHndBBtabCount == 0) + { + FlowGraphDfsTree* const dfsTree = fgComputeDfs(); + // unsigned* ordinal = new (this, CMK_Generic) unsigned[dfsTree->GetPostOrderCount()]; + // jitstd::list edges(jitstd::allocator(getAllocator(CMK_FlowEdge))); + + for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) + { + BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + // ordinal[block->bbNum] = dfsTree->GetPostOrderCount() * i; + fgUnlinkBlock(blockToMove); + fgInsertBBafter(block, blockToMove); + } + + assert(fgFirstBB->IsFirst()); + assert(fgLastBB->IsLast()); + return true; + } + + // const auto TryMoveForward = [this, ordinal](FlowEdge* const edge) + // { + // assert(edge != nullptr); + // BasicBlock* fixed = edge->getSourceBlock(); + // BasicBlock* moving = edge->getDestinationBlock(); + + // if (fixed->NextIs(moving)) + // { + // return true; + // } + + // if (moving->IsLast()) + // { + // return false; + // } + + // const unsigned fixedOrdinal = ordinal[fixed->bbNum]; + // const unsigned movingOrdinal = ordinal[moving->bbNum]; + // if (movingOrdinal < fixedOrdinal) + // { + // return false; + // } + + // unsigned ordinalInUse = fixedOrdinal + 1; + // ordinal[moving->bbNum] = ordinalInUse; + // fgUnlinkBlock(moving); + // fgInsertBBafter(fixed, moving); + + // BasicBlock* blockToCheck = moving->Next(); + // while ((blockToCheck != nullptr) && (ordinalInUse == ordinal[blockToCheck->bbNum])) + // { + // ordinalInUse++; + // ordinal[blockToCheck->bbNum] = ordinalInUse; + // blockToCheck = blockToCheck->Next(); + // } + + // return true; + // }; + + // for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) + // { + // for (FlowEdge* const pred : block->PredEdges()) + // { + // edges.remove(pred); + // } + + // if (block->IsFirst() || block->IsLast()) + // { + // continue; + // } + + // unsigned numForwardEdges = 0; + // FlowEdge* forwardEdges[2]; + // for (FlowEdge* const succEdge : block->SuccEdges()) + // { + // if (ordinal[block->bbNum] < ordinal[succEdge->getDestinationBlock()->bbNum]) + // { + // numForwardEdges++; + // forwardEdges[min((unsigned)0, numForwardEdges)] = succEdge; + // } + // } + + // bool wasMoved = false; + // bool considerPending = false; + + // switch (numForwardEdges) + // { + // case 1: + // wasMoved = TryMoveForward(forwardEdges[0]); + // break; + + // case 2: + // { + // considerPending = false; + // const bool firstEdgeLikely = forwardEdges[0]->getLikelihood() > forwardEdges[1]->getLikelihood(); + // wasMoved = firstEdgeLikely ? TryMoveForward(forwardEdges[0]) : TryMoveForward(forwardEdges[1]); + + // if (!wasMoved) + // { + // wasMoved = firstEdgeLikely ? TryMoveForward(forwardEdges[1]) : TryMoveForward(forwardEdges[0]); + // } + + // if (!block->NextIs(forwardEdges[0]->getDestinationBlock())) + // { + // edges.push_back(forwardEdges[0]); + // } + + // if (!block->NextIs(forwardEdges[1]->getDestinationBlock())) + // { + // edges.push_back(forwardEdges[1]); + // } + // break; + // } + + // default: + // // This is an n-ary branch, or we don't have any forward edges. Don't bother. + // continue; + // } + + // if (wasMoved) + // { + // continue; + // } + + // if (!considerPending) + // { + // continue; + // } + + // while (!edges.empty()) + // { + // FlowEdge* const edge = edges.back(); + // edges.pop_back(); + + // assert(ordinal[edge->getSourceBlock()->bbNum] < ordinal[block->bbNum]); + // assert(ordinal[edge->getDestinationBlock()->bbNum] > ordinal[block->bbNum]); + + // if (TryMoveForward(edge)) + // { + // break; + // } + // } + // } + + // We will be reordering blocks, so ensure the false target of a BBJ_COND block is its next block for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) { if (block->KindIs(BBJ_COND) && !block->NextIs(block->GetFalseTarget())) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 6ab0977c55c25..fc2fb86245687 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4070,6 +4070,7 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) // Preorder and postorder numbers are assigned into the BasicBlock structure. // The tree returned contains a postorder of the basic blocks. // +template FlowGraphDfsTree* Compiler::fgComputeDfs() { BasicBlock** postOrder = new (this, CMK_DepthFirstSearch) BasicBlock*[fgBBcount]; @@ -4095,10 +4096,18 @@ FlowGraphDfsTree* Compiler::fgComputeDfs() } }; - unsigned numBlocks = fgRunDfs(visitPreorder, visitPostorder, visitEdge); + + unsigned numBlocks = skipEH ? fgRunDfs(visitPreorder, visitPostorder, visitEdge) + : fgRunDfs(visitPreorder, visitPostorder, visitEdge); return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks, hasCycle); } +// Add explicit instantiations. +template FlowGraphDfsTree* Compiler::fgComputeDfs(); +template FlowGraphDfsTree* Compiler::fgComputeDfs(); +template FlowGraphDfsTree* Compiler::fgComputeDfs(); +template FlowGraphDfsTree* Compiler::fgComputeDfs(); + //------------------------------------------------------------------------ // fgInvalidateDfsTree: Invalidate computed DFS tree and dependent annotations // (like loops, dominators and SSA). From de7a43a0b703113566741a06dbe4d27779eeed05 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 23 Apr 2024 19:38:45 -0400 Subject: [PATCH 02/16] Implement RPO layout for methods with EH --- src/coreclr/jit/compiler.hpp | 11 ++-- src/coreclr/jit/fgopt.cpp | 107 +++++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index e1c67afc56532..3b0c2148e264c 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -721,14 +721,13 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const return BasicBlockVisit::Continue; case BBJ_CALLFINALLY: - if (useProfile && isBBCallFinallyPair()) + if (useProfile) { - return func(Next()); - } - else - { - return func(GetTarget()); + return isBBCallFinallyPair() ? func(Next()) : BasicBlockVisit::Continue; } + + return func(GetTarget()); + case BBJ_CALLFINALLYRET: case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a1622ea1fc6c0..f82ca840fd528 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3399,26 +3399,111 @@ bool Compiler::fgReorderBlocks(bool useProfile) if (useProfile) { - if (compHndBBtabCount == 0) + FlowGraphDfsTree* const dfsTree = fgComputeDfs(); + + for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { - FlowGraphDfsTree* const dfsTree = fgComputeDfs(); - // unsigned* ordinal = new (this, CMK_Generic) unsigned[dfsTree->GetPostOrderCount()]; - // jitstd::list edges(jitstd::allocator(getAllocator(CMK_FlowEdge))); + BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + fgUnlinkBlock(blockToMove); + fgInsertBBafter(block, blockToMove); + } + + if (verbose) + { + fgDispBasicBlocks(); + } + + if (compHndBBtabCount != 0) + { + BasicBlock** const tryRegionExits = new (this, CMK_Generic) BasicBlock*[compHndBBtabCount]{}; + for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) + { + BasicBlock* const next = block->Next(); + + if (block->hasTryIndex()) + { + const unsigned tryIndex = block->getTryIndex(); + + if (!bbIsTryBeg(block)) + { + fgUnlinkBlock(block); + fgInsertBBafter(tryRegionExits[tryIndex], block); + } + + tryRegionExits[tryIndex] = block; + } + + block = next; + } + + if (verbose) + { + fgDispBasicBlocks(); + } for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { BasicBlock* const block = dfsTree->GetPostOrder(i); BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); - // ordinal[block->bbNum] = dfsTree->GetPostOrderCount() * i; - fgUnlinkBlock(blockToMove); - fgInsertBBafter(block, blockToMove); + + if (bbIsTryBeg(blockToMove)) + { + const unsigned tryIndex = blockToMove->getTryIndex(); + + if (block->hasTryIndex() && (block->getTryIndex() < tryIndex)) + { + continue; + } + + fgUnlinkRange(blockToMove, tryRegionExits[tryIndex]); + fgMoveBlocksAfter(blockToMove, tryRegionExits[tryIndex], block); + } + } + + for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) + { + BasicBlock* const tryExit = tryRegionExits[XTnum]; + + if (tryExit == nullptr) + { + continue; + } + + EHblkDsc* const ehDsc = ehGetDsc(XTnum); + const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; + ehDsc->ebdTryLast = tryExit; + + if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) + { + BasicBlock* const enclosingTryExit = tryRegionExits[enclosingTryIndex]; + BasicBlock* const tryEntryPrev = ehDsc->ebdTryBeg->Prev(); + const unsigned predTryIndex = ((tryEntryPrev != nullptr) && tryEntryPrev->hasTryIndex()) ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; + + if ((enclosingTryExit == nullptr) || enclosingTryExit->NextIs(ehDsc->ebdTryBeg)) + { + tryRegionExits[enclosingTryIndex] = tryExit; + } + else if (predTryIndex != enclosingTryIndex) + { + assert(enclosingTryExit != nullptr); + fgUnlinkRange(ehDsc->ebdTryBeg, ehDsc->ebdTryLast); + fgMoveBlocksAfter(ehDsc->ebdTryBeg, ehDsc->ebdTryLast, enclosingTryExit); + tryRegionExits[enclosingTryIndex] = tryExit; + } + } + } + + if (verbose) + { + fgDispBasicBlocks(); } - - assert(fgFirstBB->IsFirst()); - assert(fgLastBB->IsLast()); - return true; } + assert(fgFirstBB->IsFirst()); + assert(fgLastBB->IsLast()); + return true; + // const auto TryMoveForward = [this, ordinal](FlowEdge* const edge) // { // assert(edge != nullptr); From 17fbe7cdf60879f51550cb6d326f39575c80087d Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Apr 2024 00:34:03 -0400 Subject: [PATCH 03/16] Cleanup --- src/coreclr/jit/block.cpp | 12 +- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/compiler.hpp | 18 +- src/coreclr/jit/fgopt.cpp | 395 +++++++++++++----------------- src/coreclr/jit/flowgraph.cpp | 10 +- src/coreclr/jit/jitconfigvalues.h | 3 + 6 files changed, 198 insertions(+), 241 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index cd0175b8a3b3a..b1a9c30d2c8f3 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -137,10 +137,11 @@ void FlowEdge::addLikelihood(weight_t addedLikelihood) // AllSuccessorEnumerator: Construct an instance of the enumerator. // // Arguments: -// comp - Compiler instance -// block - The block whose successors are to be iterated +// comp - Compiler instance +// block - The block whose successors are to be iterated +// useProfile - Unused; this is needed to match RegularSuccessorEnumerator's parameter list // -AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile) +AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile /* = false */) : m_block(block) { m_numSuccs = 0; @@ -173,8 +174,9 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block // RegularSuccessorEnumerator: Construct an instance of the enumerator. // // Arguments: -// comp - Compiler instance -// block - The block whose successors are to be iterated +// comp - Compiler instance +// block - The block whose successors are to be iterated +// useProfile - If true, determines the order of successors visited using profile data // RegularSuccessorEnumerator::RegularSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile) : m_block(block) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 15c3d0740614f..bd9b8cfdd189a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6047,6 +6047,7 @@ class Compiler bool fgComputeCalledCount(weight_t returnWeight); bool fgReorderBlocks(bool useProfile); + void fgDoReversePostOrderLayout(); bool fgFuncletsAreCold(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 3b0c2148e264c..f66d892caa9fa 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -696,14 +696,15 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) // VisitRegularSuccs: Visit regular successors of this block. // // Arguments: -// comp - Compiler instance -// func - Callback +// comp - Compiler instance +// func - Callback +// useProfile - If true, determines the order of successors visited using profile data // // Returns: // Whether or not the visiting was aborted. // template -BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile) +BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile /* = false */) { switch (bbKind) { @@ -723,6 +724,7 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const case BBJ_CALLFINALLY: if (useProfile) { + // Don't visit EH successor if using profile data for block reordering. return isBBCallFinallyPair() ? func(Next()) : BasicBlockVisit::Continue; } @@ -4757,9 +4759,11 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // fgRunDfs: Run DFS over the flow graph. // // Type parameters: -// VisitPreorder - Functor type that takes a BasicBlock* and its preorder number -// VisitPostorder - Functor type that takes a BasicBlock* and its postorder number -// VisitEdge - Functor type that takes two BasicBlock*. +// VisitPreorder - Functor type that takes a BasicBlock* and its preorder number +// VisitPostorder - Functor type that takes a BasicBlock* and its postorder number +// VisitEdge - Functor type that takes two BasicBlock*. +// SuccessorEnumerator - Enumerator type for controlling which successors are visited +// useProfile - If true, determines order of successors visited using profile data // // Parameters: // visitPreorder - Functor to visit block in its preorder @@ -4770,7 +4774,7 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // Returns: // Number of blocks visited. // -template +template unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { BitVecTraits traits(fgBBNumMax + 1, this); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index f82ca840fd528..7f24bff1b00da 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3399,235 +3399,12 @@ bool Compiler::fgReorderBlocks(bool useProfile) if (useProfile) { - FlowGraphDfsTree* const dfsTree = fgComputeDfs(); - - for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) - { - BasicBlock* const block = dfsTree->GetPostOrder(i); - BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); - fgUnlinkBlock(blockToMove); - fgInsertBBafter(block, blockToMove); - } - - if (verbose) - { - fgDispBasicBlocks(); - } - - if (compHndBBtabCount != 0) + if (JitConfig.JitDoReversePostOrderLayout()) { - BasicBlock** const tryRegionExits = new (this, CMK_Generic) BasicBlock*[compHndBBtabCount]{}; - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) - { - BasicBlock* const next = block->Next(); - - if (block->hasTryIndex()) - { - const unsigned tryIndex = block->getTryIndex(); - - if (!bbIsTryBeg(block)) - { - fgUnlinkBlock(block); - fgInsertBBafter(tryRegionExits[tryIndex], block); - } - - tryRegionExits[tryIndex] = block; - } - - block = next; - } - - if (verbose) - { - fgDispBasicBlocks(); - } - - for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) - { - BasicBlock* const block = dfsTree->GetPostOrder(i); - BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); - - if (bbIsTryBeg(blockToMove)) - { - const unsigned tryIndex = blockToMove->getTryIndex(); - - if (block->hasTryIndex() && (block->getTryIndex() < tryIndex)) - { - continue; - } - - fgUnlinkRange(blockToMove, tryRegionExits[tryIndex]); - fgMoveBlocksAfter(blockToMove, tryRegionExits[tryIndex], block); - } - } - - for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) - { - BasicBlock* const tryExit = tryRegionExits[XTnum]; - - if (tryExit == nullptr) - { - continue; - } - - EHblkDsc* const ehDsc = ehGetDsc(XTnum); - const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; - ehDsc->ebdTryLast = tryExit; - - if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - BasicBlock* const enclosingTryExit = tryRegionExits[enclosingTryIndex]; - BasicBlock* const tryEntryPrev = ehDsc->ebdTryBeg->Prev(); - const unsigned predTryIndex = ((tryEntryPrev != nullptr) && tryEntryPrev->hasTryIndex()) ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; - - if ((enclosingTryExit == nullptr) || enclosingTryExit->NextIs(ehDsc->ebdTryBeg)) - { - tryRegionExits[enclosingTryIndex] = tryExit; - } - else if (predTryIndex != enclosingTryIndex) - { - assert(enclosingTryExit != nullptr); - fgUnlinkRange(ehDsc->ebdTryBeg, ehDsc->ebdTryLast); - fgMoveBlocksAfter(ehDsc->ebdTryBeg, ehDsc->ebdTryLast, enclosingTryExit); - tryRegionExits[enclosingTryIndex] = tryExit; - } - } - } - - if (verbose) - { - fgDispBasicBlocks(); - } + fgDoReversePostOrderLayout(); + return true; } - assert(fgFirstBB->IsFirst()); - assert(fgLastBB->IsLast()); - return true; - - // const auto TryMoveForward = [this, ordinal](FlowEdge* const edge) - // { - // assert(edge != nullptr); - // BasicBlock* fixed = edge->getSourceBlock(); - // BasicBlock* moving = edge->getDestinationBlock(); - - // if (fixed->NextIs(moving)) - // { - // return true; - // } - - // if (moving->IsLast()) - // { - // return false; - // } - - // const unsigned fixedOrdinal = ordinal[fixed->bbNum]; - // const unsigned movingOrdinal = ordinal[moving->bbNum]; - // if (movingOrdinal < fixedOrdinal) - // { - // return false; - // } - - // unsigned ordinalInUse = fixedOrdinal + 1; - // ordinal[moving->bbNum] = ordinalInUse; - // fgUnlinkBlock(moving); - // fgInsertBBafter(fixed, moving); - - // BasicBlock* blockToCheck = moving->Next(); - // while ((blockToCheck != nullptr) && (ordinalInUse == ordinal[blockToCheck->bbNum])) - // { - // ordinalInUse++; - // ordinal[blockToCheck->bbNum] = ordinalInUse; - // blockToCheck = blockToCheck->Next(); - // } - - // return true; - // }; - - // for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) - // { - // for (FlowEdge* const pred : block->PredEdges()) - // { - // edges.remove(pred); - // } - - // if (block->IsFirst() || block->IsLast()) - // { - // continue; - // } - - // unsigned numForwardEdges = 0; - // FlowEdge* forwardEdges[2]; - // for (FlowEdge* const succEdge : block->SuccEdges()) - // { - // if (ordinal[block->bbNum] < ordinal[succEdge->getDestinationBlock()->bbNum]) - // { - // numForwardEdges++; - // forwardEdges[min((unsigned)0, numForwardEdges)] = succEdge; - // } - // } - - // bool wasMoved = false; - // bool considerPending = false; - - // switch (numForwardEdges) - // { - // case 1: - // wasMoved = TryMoveForward(forwardEdges[0]); - // break; - - // case 2: - // { - // considerPending = false; - // const bool firstEdgeLikely = forwardEdges[0]->getLikelihood() > forwardEdges[1]->getLikelihood(); - // wasMoved = firstEdgeLikely ? TryMoveForward(forwardEdges[0]) : TryMoveForward(forwardEdges[1]); - - // if (!wasMoved) - // { - // wasMoved = firstEdgeLikely ? TryMoveForward(forwardEdges[1]) : TryMoveForward(forwardEdges[0]); - // } - - // if (!block->NextIs(forwardEdges[0]->getDestinationBlock())) - // { - // edges.push_back(forwardEdges[0]); - // } - - // if (!block->NextIs(forwardEdges[1]->getDestinationBlock())) - // { - // edges.push_back(forwardEdges[1]); - // } - // break; - // } - - // default: - // // This is an n-ary branch, or we don't have any forward edges. Don't bother. - // continue; - // } - - // if (wasMoved) - // { - // continue; - // } - - // if (!considerPending) - // { - // continue; - // } - - // while (!edges.empty()) - // { - // FlowEdge* const edge = edges.back(); - // edges.pop_back(); - - // assert(ordinal[edge->getSourceBlock()->bbNum] < ordinal[block->bbNum]); - // assert(ordinal[edge->getDestinationBlock()->bbNum] > ordinal[block->bbNum]); - - // if (TryMoveForward(edge)) - // { - // break; - // } - // } - // } - // We will be reordering blocks, so ensure the false target of a BBJ_COND block is its next block for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) { @@ -4747,6 +4524,172 @@ bool Compiler::fgReorderBlocks(bool useProfile) #pragma warning(pop) #endif +//----------------------------------------------------------------------------- +// fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO of the method's +// main body (i.e. non-EH) successors. +// +// Notes: +// This will not reorder blocks within handler regions. +// +void Compiler::fgDoReversePostOrderLayout() +{ +#ifdef DEBUG + if (verbose) + { + printf("*************** In fgDoReversePostOrderLayout()\n"); + + printf("\nInitial BasicBlocks"); + fgDispBasicBlocks(verboseTrees); + printf("\n"); + } +#endif // DEBUG + + // Compute DFS of main function body's blocks, using profile data to determine the order successors are visited in + // + FlowGraphDfsTree* const dfsTree = fgComputeDfs(); + + for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) + { + BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + fgUnlinkBlock(blockToMove); + fgInsertBBafter(block, blockToMove); + } + + if (compHndBBtabCount == 0) + { + // No try regions to fix + // + return; + } + + // The RPO-based layout above can make try regions non-contiguous. + // We will fix this by placing each try region's blocks adjacent to one another, + // and then re-inserting each try region based on the RPO. + // + + // First, re-establish contiguousness of try regions + // (tryRegionEnds tracks the last block visited in each try region) + // + BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock*[compHndBBtabCount]{}; + for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) + { + BasicBlock* const next = block->Next(); + + if (block->hasTryIndex()) + { + const unsigned tryIndex = block->getTryIndex(); + + if (tryRegionEnds[tryIndex] != nullptr) + { + fgUnlinkBlock(block); + fgInsertBBafter(tryRegionEnds[tryIndex], block); + } + + tryRegionEnds[tryIndex] = block; + } + + block = next; + } + + // Move each contiguous try region based on the RPO + // + for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) + { + BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + + if (bbIsTryBeg(blockToMove)) + { + const unsigned tryIndex = blockToMove->getTryIndex(); + + // The candidate predecessor block is in a try region + // + if (block->hasTryIndex()) + { + const unsigned predTryIndex = block->getTryIndex(); + + // We can reach blockToMove's try region from block's try region, but they aren't nested regions, + // and we aren't jumping from the end of block's try region, so don't move blockToMove + // + if ((predTryIndex != ehGetDsc(tryIndex)->ebdEnclosingTryIndex) && (block != tryRegionEnds[predTryIndex])) + { + continue; + } + } + + fgUnlinkRange(blockToMove, tryRegionEnds[tryIndex]); + fgMoveBlocksAfter(blockToMove, tryRegionEnds[tryIndex], block); + } + } + + // Update the EH descriptors + // + for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) + { + BasicBlock* const tryExit = tryRegionEnds[XTnum]; + + // We can have multiple EH descriptors map to the same try region, + // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, + // so the duplicate EH descriptors' last block pointers can be null. + // Tolerate this. + // + if (tryExit == nullptr) + { + continue; + } + + // Update the end pointer of this try region to the new last block + EHblkDsc* const ehDsc = ehGetDsc(XTnum); + const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; + ehDsc->ebdTryLast = tryExit; + + // If this try region is nested in another one, we might need to update its enclosing region's end block + // + if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) + { + BasicBlock* const enclosingTryExit = tryRegionEnds[enclosingTryIndex]; + + // If multiple EH descriptors map to the same try region, + // then the enclosing region's last block might be null, so set it here. + // Similarly, if the enclosing region ends right before the nested region begins, + // extend the enclosing region's last block to the end of the nested region. + // + if ((enclosingTryExit == nullptr) || enclosingTryExit->NextIs(ehDsc->ebdTryBeg)) + { + tryRegionEnds[enclosingTryIndex] = tryExit; + continue; + } + + // This try region has a unique enclosing region, + // so this region cannot possibly be at the beginning of the method + // + assert(!ehDsc->ebdTryBeg->IsFirst()); + BasicBlock* const tryEntryPrev = ehDsc->ebdTryBeg->Prev(); + const unsigned predTryIndex = tryEntryPrev->hasTryIndex() ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; + + if (predTryIndex != enclosingTryIndex) + { + // We can visit the end of the enclosing try region before visiting this try region, + // thus placing this region outside of its enclosing region. + // Fix this by moving this region to the end of the enclosing region. + // + assert(enclosingTryExit != nullptr); + fgUnlinkRange(ehDsc->ebdTryBeg, ehDsc->ebdTryLast); + fgMoveBlocksAfter(ehDsc->ebdTryBeg, ehDsc->ebdTryLast, enclosingTryExit); + tryRegionEnds[enclosingTryIndex] = tryExit; + } + } + } + +#ifdef DEBUG + if (expensiveDebugCheckLevel >= 2) + { + fgDebugCheckBBlist(); + } +#endif // DEBUG +} + //------------------------------------------------------------- // fgUpdateFlowGraphPhase: run flow graph optimization as a // phase, with no tail duplication diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index fc2fb86245687..a5db5187addf5 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4043,8 +4043,8 @@ bool FlowGraphDfsTree::Contains(BasicBlock* block) const // block `descendant` // // Arguments: -// ancestor -- block that is possible ancestor -// descendant -- block that is possible descendant +// ancestor - block that is possible ancestor +// descendant - block that is possible descendant // // Returns: // True if `ancestor` is ancestor of `descendant` in the depth first spanning @@ -4063,6 +4063,10 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) //------------------------------------------------------------------------ // fgComputeDfs: Compute a depth-first search tree for the flow graph. // +// Type parameters: +// skipEH - If true, does not iterate EH successors +// useProfile - If true, determines order of successors visited using profile data +// // Returns: // The tree. // @@ -4070,7 +4074,7 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) // Preorder and postorder numbers are assigned into the BasicBlock structure. // The tree returned contains a postorder of the basic blocks. // -template +template FlowGraphDfsTree* Compiler::fgComputeDfs() { BasicBlock** postOrder = new (this, CMK_DepthFirstSearch) BasicBlock*[fgBBcount]; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 700cd33bd6bf2..54aaafc4273dd 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -754,6 +754,9 @@ RELEASE_CONFIG_INTEGER(JitEnablePhysicalPromotion, W("JitEnablePhysicalPromotion // Enable cross-block local assertion prop RELEASE_CONFIG_INTEGER(JitEnableCrossBlockLocalAssertionProp, W("JitEnableCrossBlockLocalAssertionProp"), 1) +// Do greedy RPO-based layout in Compiler::fgReorderBlocks. +RELEASE_CONFIG_INTEGER(JitDoReversePostOrderLayout, W("JitDoReversePostOrderLayout"), 1); + // JitFunctionFile: Name of a file that contains a list of functions. If the currently compiled function is in the // file, certain other JIT config variables will be active. If the currently compiled function is not in the file, // the specific JIT config variables will not be active. From 1084d7ea87f52cf3b655c0ac6a3c3bf4a215dd60 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 24 Apr 2024 00:36:19 -0400 Subject: [PATCH 04/16] Style --- src/coreclr/jit/block.cpp | 14 ++++++++++---- src/coreclr/jit/compiler.hpp | 10 +++++++--- src/coreclr/jit/fgopt.cpp | 24 +++++++++++++----------- src/coreclr/jit/flowgraph.cpp | 8 +++++--- 4 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index b1a9c30d2c8f3..13d9304dc99cf 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -182,7 +182,9 @@ RegularSuccessorEnumerator::RegularSuccessorEnumerator(Compiler* comp, BasicBloc : m_block(block) { m_numSuccs = 0; - block->VisitRegularSuccs(comp, [this](BasicBlock* succ) { + block->VisitRegularSuccs( + comp, + [this](BasicBlock* succ) { if (m_numSuccs < ArrLen(m_successors)) { m_successors[m_numSuccs] = succ; @@ -190,18 +192,22 @@ RegularSuccessorEnumerator::RegularSuccessorEnumerator(Compiler* comp, BasicBloc m_numSuccs++; return BasicBlockVisit::Continue; - }, useProfile); + }, + useProfile); if (m_numSuccs > ArrLen(m_successors)) { m_pSuccessors = new (comp, CMK_BasicBlock) BasicBlock*[m_numSuccs]; unsigned numSuccs = 0; - block->VisitRegularSuccs(comp, [this, &numSuccs](BasicBlock* succ) { + block->VisitRegularSuccs( + comp, + [this, &numSuccs](BasicBlock* succ) { assert(numSuccs < m_numSuccs); m_pSuccessors[numSuccs++] = succ; return BasicBlockVisit::Continue; - }, useProfile); + }, + useProfile); assert(numSuccs == m_numSuccs); } diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index f66d892caa9fa..4040c6e130bec 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -727,9 +727,9 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const // Don't visit EH successor if using profile data for block reordering. return isBBCallFinallyPair() ? func(Next()) : BasicBlockVisit::Continue; } - + return func(GetTarget()); - + case BBJ_CALLFINALLYRET: case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: @@ -4774,7 +4774,11 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // Returns: // Number of blocks visited. // -template +template unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { BitVecTraits traits(fgBBNumMax + 1, this); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 7f24bff1b00da..b36358229d5af 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4550,7 +4550,7 @@ void Compiler::fgDoReversePostOrderLayout() for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { - BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const block = dfsTree->GetPostOrder(i); BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); fgUnlinkBlock(blockToMove); fgInsertBBafter(block, blockToMove); @@ -4571,7 +4571,7 @@ void Compiler::fgDoReversePostOrderLayout() // First, re-establish contiguousness of try regions // (tryRegionEnds tracks the last block visited in each try region) // - BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock*[compHndBBtabCount]{}; + BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) { BasicBlock* const next = block->Next(); @@ -4596,13 +4596,13 @@ void Compiler::fgDoReversePostOrderLayout() // for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { - BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const block = dfsTree->GetPostOrder(i); BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); if (bbIsTryBeg(blockToMove)) { const unsigned tryIndex = blockToMove->getTryIndex(); - + // The candidate predecessor block is in a try region // if (block->hasTryIndex()) @@ -4612,7 +4612,8 @@ void Compiler::fgDoReversePostOrderLayout() // We can reach blockToMove's try region from block's try region, but they aren't nested regions, // and we aren't jumping from the end of block's try region, so don't move blockToMove // - if ((predTryIndex != ehGetDsc(tryIndex)->ebdEnclosingTryIndex) && (block != tryRegionEnds[predTryIndex])) + if ((predTryIndex != ehGetDsc(tryIndex)->ebdEnclosingTryIndex) && + (block != tryRegionEnds[predTryIndex])) { continue; } @@ -4640,9 +4641,9 @@ void Compiler::fgDoReversePostOrderLayout() } // Update the end pointer of this try region to the new last block - EHblkDsc* const ehDsc = ehGetDsc(XTnum); - const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; - ehDsc->ebdTryLast = tryExit; + EHblkDsc* const ehDsc = ehGetDsc(XTnum); + const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; + ehDsc->ebdTryLast = tryExit; // If this try region is nested in another one, we might need to update its enclosing region's end block // @@ -4660,14 +4661,15 @@ void Compiler::fgDoReversePostOrderLayout() tryRegionEnds[enclosingTryIndex] = tryExit; continue; } - + // This try region has a unique enclosing region, // so this region cannot possibly be at the beginning of the method // assert(!ehDsc->ebdTryBeg->IsFirst()); BasicBlock* const tryEntryPrev = ehDsc->ebdTryBeg->Prev(); - const unsigned predTryIndex = tryEntryPrev->hasTryIndex() ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; - + const unsigned predTryIndex = + tryEntryPrev->hasTryIndex() ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; + if (predTryIndex != enclosingTryIndex) { // We can visit the end of the enclosing try region before visiting this try region, diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index a5db5187addf5..b69c68babc092 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4100,9 +4100,11 @@ FlowGraphDfsTree* Compiler::fgComputeDfs() } }; - - unsigned numBlocks = skipEH ? fgRunDfs(visitPreorder, visitPostorder, visitEdge) - : fgRunDfs(visitPreorder, visitPostorder, visitEdge); + unsigned numBlocks = + skipEH ? fgRunDfs(visitPreorder, visitPostorder, visitEdge) + : fgRunDfs(visitPreorder, visitPostorder, visitEdge); return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks, hasCycle); } From d1bf04a909bb2f4b60a3876fa1682e0108d01885 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 26 Apr 2024 15:51:37 -0400 Subject: [PATCH 05/16] Only reorder blocks within same EH region --- src/coreclr/jit/fgopt.cpp | 149 +++++++++++++------------------------- 1 file changed, 52 insertions(+), 97 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index b36358229d5af..472b02f74d411 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3402,6 +3402,14 @@ bool Compiler::fgReorderBlocks(bool useProfile) if (JitConfig.JitDoReversePostOrderLayout()) { fgDoReversePostOrderLayout(); + +#ifdef DEBUG + if (expensiveDebugCheckLevel >= 2) + { + fgDebugCheckBBlist(); + } +#endif // DEBUG + return true; } @@ -4529,7 +4537,9 @@ bool Compiler::fgReorderBlocks(bool useProfile) // main body (i.e. non-EH) successors. // // Notes: -// This will not reorder blocks within handler regions. +// This won't reorder blocks in the funclet section, if there are any. +// In the main method body, this will reorder blocks in the same EH region +// to avoid making any regions non-contiguous. // void Compiler::fgDoReversePostOrderLayout() { @@ -4548,148 +4558,93 @@ void Compiler::fgDoReversePostOrderLayout() // FlowGraphDfsTree* const dfsTree = fgComputeDfs(); - for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) - { - BasicBlock* const block = dfsTree->GetPostOrder(i); - BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); - fgUnlinkBlock(blockToMove); - fgInsertBBafter(block, blockToMove); - } - - if (compHndBBtabCount == 0) - { - // No try regions to fix - // - return; - } - - // The RPO-based layout above can make try regions non-contiguous. - // We will fix this by placing each try region's blocks adjacent to one another, - // and then re-inserting each try region based on the RPO. - // - - // First, re-establish contiguousness of try regions - // (tryRegionEnds tracks the last block visited in each try region) + // Fast path: We don't have any EH regions, so just reorder the blocks // - BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB;) + if (compHndBBtabCount == 0) { - BasicBlock* const next = block->Next(); - - if (block->hasTryIndex()) + for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { - const unsigned tryIndex = block->getTryIndex(); - - if (tryRegionEnds[tryIndex] != nullptr) - { - fgUnlinkBlock(block); - fgInsertBBafter(tryRegionEnds[tryIndex], block); - } - - tryRegionEnds[tryIndex] = block; + BasicBlock* const block = dfsTree->GetPostOrder(i); + BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + fgUnlinkBlock(blockToMove); + fgInsertBBafter(block, blockToMove); } - block = next; + return; } - // Move each contiguous try region based on the RPO + // We have EH regions, so make sure the RPO doesn't make any regions non-contiguous + // by only reordering blocks within the same region // for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { BasicBlock* const block = dfsTree->GetPostOrder(i); BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); - if (bbIsTryBeg(blockToMove)) + if (BasicBlock::sameEHRegion(block, blockToMove)) { - const unsigned tryIndex = blockToMove->getTryIndex(); - - // The candidate predecessor block is in a try region - // - if (block->hasTryIndex()) - { - const unsigned predTryIndex = block->getTryIndex(); + fgUnlinkBlock(blockToMove); + fgInsertBBafter(block, blockToMove); + } + } - // We can reach blockToMove's try region from block's try region, but they aren't nested regions, - // and we aren't jumping from the end of block's try region, so don't move blockToMove - // - if ((predTryIndex != ehGetDsc(tryIndex)->ebdEnclosingTryIndex) && - (block != tryRegionEnds[predTryIndex])) - { - continue; - } - } + // The RPO won't change the entry blocks of any try regions, but reordering can change the last block in a region + // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). + // First, determine the new try region ends. + // + BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - fgUnlinkRange(blockToMove, tryRegionEnds[tryIndex]); - fgMoveBlocksAfter(blockToMove, tryRegionEnds[tryIndex], block); + // If we aren't using EH funclets, fgFirstFuncletBB is nullptr, and we'll traverse the entire blocklist. + // Else if we do have funclets, don't extend the end of a try region to include its funclet blocks. + // + for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = block->Next()) + { + if (block->hasTryIndex()) + { + tryRegionEnds[block->getTryIndex()] = block; } } - // Update the EH descriptors + // Now, update the EH descriptors // - for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) + unsigned XTnum; + EHblkDsc* HBtab; + for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) { - BasicBlock* const tryExit = tryRegionEnds[XTnum]; + BasicBlock* const tryEnd = tryRegionEnds[XTnum]; // We can have multiple EH descriptors map to the same try region, // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, // so the duplicate EH descriptors' last block pointers can be null. // Tolerate this. // - if (tryExit == nullptr) + if (tryEnd == nullptr) { continue; } // Update the end pointer of this try region to the new last block - EHblkDsc* const ehDsc = ehGetDsc(XTnum); - const unsigned enclosingTryIndex = ehDsc->ebdEnclosingTryIndex; - ehDsc->ebdTryLast = tryExit; + // + HBtab->ebdTryLast = tryEnd; + const unsigned enclosingTryIndex = HBtab->ebdEnclosingTryIndex; // If this try region is nested in another one, we might need to update its enclosing region's end block // if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) { - BasicBlock* const enclosingTryExit = tryRegionEnds[enclosingTryIndex]; + BasicBlock* const enclosingTryEnd = tryRegionEnds[enclosingTryIndex]; // If multiple EH descriptors map to the same try region, - // then the enclosing region's last block might be null, so set it here. + // then the enclosing region's last block might be null in the table, so set it here. // Similarly, if the enclosing region ends right before the nested region begins, // extend the enclosing region's last block to the end of the nested region. // - if ((enclosingTryExit == nullptr) || enclosingTryExit->NextIs(ehDsc->ebdTryBeg)) + if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) { - tryRegionEnds[enclosingTryIndex] = tryExit; - continue; - } - - // This try region has a unique enclosing region, - // so this region cannot possibly be at the beginning of the method - // - assert(!ehDsc->ebdTryBeg->IsFirst()); - BasicBlock* const tryEntryPrev = ehDsc->ebdTryBeg->Prev(); - const unsigned predTryIndex = - tryEntryPrev->hasTryIndex() ? tryEntryPrev->getTryIndex() : EHblkDsc::NO_ENCLOSING_INDEX; - - if (predTryIndex != enclosingTryIndex) - { - // We can visit the end of the enclosing try region before visiting this try region, - // thus placing this region outside of its enclosing region. - // Fix this by moving this region to the end of the enclosing region. - // - assert(enclosingTryExit != nullptr); - fgUnlinkRange(ehDsc->ebdTryBeg, ehDsc->ebdTryLast); - fgMoveBlocksAfter(ehDsc->ebdTryBeg, ehDsc->ebdTryLast, enclosingTryExit); - tryRegionEnds[enclosingTryIndex] = tryExit; + tryRegionEnds[enclosingTryIndex] = tryEnd; } } } - -#ifdef DEBUG - if (expensiveDebugCheckLevel >= 2) - { - fgDebugCheckBBlist(); - } -#endif // DEBUG } //------------------------------------------------------------- From 98b4bd923586b97bee5e6313a01157bd1edf4ffa Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 26 Apr 2024 16:24:14 -0400 Subject: [PATCH 06/16] Style --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 101e36e9f0338..604d1151aa1d8 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4611,7 +4611,7 @@ void Compiler::fgDoReversePostOrderLayout() // Now, update the EH descriptors // - unsigned XTnum; + unsigned XTnum; EHblkDsc* HBtab; for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) { From 587b5bf5c73260eec00503b87afd94e3fbda7ddf Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sat, 27 Apr 2024 18:53:56 -0400 Subject: [PATCH 07/16] Use full RPO --- src/coreclr/jit/block.cpp | 49 +--------------- src/coreclr/jit/block.h | 47 +-------------- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/compiler.hpp | 54 +++++++---------- src/coreclr/jit/fgopt.cpp | 107 ++++++++++++++++++++++++++++------ src/coreclr/jit/flowgraph.cpp | 15 ++--- 6 files changed, 123 insertions(+), 153 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 13d9304dc99cf..ee12e099c19f8 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -139,7 +139,7 @@ void FlowEdge::addLikelihood(weight_t addedLikelihood) // Arguments: // comp - Compiler instance // block - The block whose successors are to be iterated -// useProfile - Unused; this is needed to match RegularSuccessorEnumerator's parameter list +// useProfile - If true, determines the order of successors visited using profile data // AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile /* = false */) : m_block(block) @@ -153,7 +153,7 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block m_numSuccs++; return BasicBlockVisit::Continue; - }); + }, useProfile); if (m_numSuccs > ArrLen(m_successors)) { @@ -164,50 +164,7 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block assert(numSuccs < m_numSuccs); m_pSuccessors[numSuccs++] = succ; return BasicBlockVisit::Continue; - }); - - assert(numSuccs == m_numSuccs); - } -} - -//------------------------------------------------------------------------ -// RegularSuccessorEnumerator: Construct an instance of the enumerator. -// -// Arguments: -// comp - Compiler instance -// block - The block whose successors are to be iterated -// useProfile - If true, determines the order of successors visited using profile data -// -RegularSuccessorEnumerator::RegularSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile) - : m_block(block) -{ - m_numSuccs = 0; - block->VisitRegularSuccs( - comp, - [this](BasicBlock* succ) { - if (m_numSuccs < ArrLen(m_successors)) - { - m_successors[m_numSuccs] = succ; - } - - m_numSuccs++; - return BasicBlockVisit::Continue; - }, - useProfile); - - if (m_numSuccs > ArrLen(m_successors)) - { - m_pSuccessors = new (comp, CMK_BasicBlock) BasicBlock*[m_numSuccs]; - - unsigned numSuccs = 0; - block->VisitRegularSuccs( - comp, - [this, &numSuccs](BasicBlock* succ) { - assert(numSuccs < m_numSuccs); - m_pSuccessors[numSuccs++] = succ; - return BasicBlockVisit::Continue; - }, - useProfile); + }, useProfile); assert(numSuccs == m_numSuccs); } diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 302d6e10f1707..1c7d5c92a72f0 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1820,13 +1820,13 @@ struct BasicBlock : private LIR::Range BasicBlockVisit VisitEHEnclosedHandlerSecondPassSuccs(Compiler* comp, TFunc func); template - BasicBlockVisit VisitAllSuccs(Compiler* comp, TFunc func); + BasicBlockVisit VisitAllSuccs(Compiler* comp, TFunc func, const bool useProfile = false); template BasicBlockVisit VisitEHSuccs(Compiler* comp, TFunc func); template - BasicBlockVisit VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile = false); + BasicBlockVisit VisitRegularSuccs(Compiler* comp, TFunc func); bool HasPotentialEHSuccs(Compiler* comp); @@ -2544,49 +2544,6 @@ class AllSuccessorEnumerator } }; -// An enumerator of a block's non-EH successors. Used for RPO traversals that exclude EH regions. -class RegularSuccessorEnumerator -{ - BasicBlock* m_block; - union - { - // We store up to 4 successors inline in the enumerator. For ASP.NET - // and libraries.pmi this is enough in 99.7% of cases. - BasicBlock* m_successors[4]; - BasicBlock** m_pSuccessors; - }; - - unsigned m_numSuccs; - unsigned m_curSucc = UINT_MAX; - -public: - // Constructs an enumerator of `block`'s regular successors. - RegularSuccessorEnumerator(Compiler* comp, BasicBlock* block, const bool useProfile); - - // Gets the block whose successors are enumerated. - BasicBlock* Block() - { - return m_block; - } - - // Returns the next available successor or `nullptr` if there are no more successors. - BasicBlock* NextSuccessor() - { - m_curSucc++; - if (m_curSucc >= m_numSuccs) - { - return nullptr; - } - - if (m_numSuccs <= ArrLen(m_successors)) - { - return m_successors[m_curSucc]; - } - - return m_pSuccessors[m_curSucc]; - } -}; - // Simple dominator tree node that keeps track of a node's first child and next sibling. // The parent is provided by BasicBlock::bbIDom. struct DomTreeNode diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a30becb89afc7..5bab6475b902d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6075,10 +6075,10 @@ class Compiler PhaseStatus fgSetBlockOrder(); bool fgHasCycleWithoutGCSafePoint(); - template + template unsigned fgRunDfs(VisitPreorder assignPreorder, VisitPostorder assignPostorder, VisitEdge visitEdge); - template + template FlowGraphDfsTree* fgComputeDfs(); void fgInvalidateDfsTree(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 4040c6e130bec..befa60bfb7f14 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -623,12 +623,13 @@ BasicBlockVisit BasicBlock::VisitEHSuccs(Compiler* comp, TFunc func) // Arguments: // comp - Compiler instance // func - Callback +// useProfile - If true, determines the order of successors visited using profile data // // Returns: // Whether or not the visiting was aborted. // template -BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) +BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func, const bool useProfile /* = false */) { switch (bbKind) { @@ -662,11 +663,19 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) return VisitEHSuccs(comp, func); case BBJ_COND: - RETURN_ON_ABORT(func(GetFalseTarget())); - - if (!TrueEdgeIs(GetFalseEdge())) + if (TrueEdgeIs(GetFalseEdge())) + { + RETURN_ON_ABORT(func(GetFalseTarget())); + } + else if (useProfile && (GetTrueEdge()->getLikelihood() < GetFalseEdge()->getLikelihood())) { RETURN_ON_ABORT(func(GetTrueTarget())); + RETURN_ON_ABORT(func(GetFalseTarget())); + } + else + { + RETURN_ON_ABORT(func(GetFalseTarget())); + RETURN_ON_ABORT(func(GetTrueTarget())); } return VisitEHSuccs(comp, func); @@ -698,13 +707,12 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) // Arguments: // comp - Compiler instance // func - Callback -// useProfile - If true, determines the order of successors visited using profile data // // Returns: // Whether or not the visiting was aborted. // template -BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const bool useProfile /* = false */) +BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func) { switch (bbKind) { @@ -722,14 +730,6 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const return BasicBlockVisit::Continue; case BBJ_CALLFINALLY: - if (useProfile) - { - // Don't visit EH successor if using profile data for block reordering. - return isBBCallFinallyPair() ? func(Next()) : BasicBlockVisit::Continue; - } - - return func(GetTarget()); - case BBJ_CALLFINALLYRET: case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: @@ -738,18 +738,10 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func, const return func(GetTarget()); case BBJ_COND: - if (TrueEdgeIs(GetFalseEdge())) - { - RETURN_ON_ABORT(func(GetFalseTarget())); - } - else if (useProfile && (GetTrueEdge()->getLikelihood() < GetFalseEdge()->getLikelihood())) - { - RETURN_ON_ABORT(func(GetTrueTarget())); - RETURN_ON_ABORT(func(GetFalseTarget())); - } - else + RETURN_ON_ABORT(func(GetFalseTarget())); + + if (!TrueEdgeIs(GetFalseEdge())) { - RETURN_ON_ABORT(func(GetFalseTarget())); RETURN_ON_ABORT(func(GetTrueTarget())); } @@ -4759,11 +4751,10 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // fgRunDfs: Run DFS over the flow graph. // // Type parameters: -// VisitPreorder - Functor type that takes a BasicBlock* and its preorder number -// VisitPostorder - Functor type that takes a BasicBlock* and its postorder number -// VisitEdge - Functor type that takes two BasicBlock*. -// SuccessorEnumerator - Enumerator type for controlling which successors are visited -// useProfile - If true, determines order of successors visited using profile data +// VisitPreorder - Functor type that takes a BasicBlock* and its preorder number +// VisitPostorder - Functor type that takes a BasicBlock* and its postorder number +// VisitEdge - Functor type that takes two BasicBlock*. +// useProfile - If true, determines order of successors visited using profile data // // Parameters: // visitPreorder - Functor to visit block in its preorder @@ -4777,7 +4768,6 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) template unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { @@ -4787,7 +4777,7 @@ unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPos unsigned preOrderIndex = 0; unsigned postOrderIndex = 0; - ArrayStack blocks(getAllocator(CMK_DepthFirstSearch)); + ArrayStack blocks(getAllocator(CMK_DepthFirstSearch)); auto dfsFrom = [&](BasicBlock* firstBB) { BitVecOps::AddElemD(&traits, visited, firstBB->bbNum); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 604d1151aa1d8..66c0f5b3e3876 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4537,13 +4537,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) #endif //----------------------------------------------------------------------------- -// fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO of the method's -// main body (i.e. non-EH) successors. -// -// Notes: -// This won't reorder blocks in the funclet section, if there are any. -// In the main method body, this will reorder blocks in the same EH region -// to avoid making any regions non-contiguous. +// fgDoReversePostOrderLayout: Reorder blocks using a greedy RPO traversal. // void Compiler::fgDoReversePostOrderLayout() { @@ -4560,7 +4554,7 @@ void Compiler::fgDoReversePostOrderLayout() // Compute DFS of main function body's blocks, using profile data to determine the order successors are visited in // - FlowGraphDfsTree* const dfsTree = fgComputeDfs(); + FlowGraphDfsTree* const dfsTree = fgComputeDfs(); // Fast path: We don't have any EH regions, so just reorder the blocks // @@ -4577,39 +4571,88 @@ void Compiler::fgDoReversePostOrderLayout() return; } - // We have EH regions, so make sure the RPO doesn't make any regions non-contiguous - // by only reordering blocks within the same region + // The RPO will break up call-finally pairs, so save them before re-ordering + // + BlockToBlockMap callFinallyPairs(getAllocator()); + + for (BasicBlock* const block : Blocks()) + { + if (block->isBBCallFinallyPair()) + { + callFinallyPairs.Set(block, block->Next()); + } + } + + // Reorder blocks // for (unsigned i = dfsTree->GetPostOrderCount() - 1; i != 0; i--) { BasicBlock* const block = dfsTree->GetPostOrder(i); BasicBlock* const blockToMove = dfsTree->GetPostOrder(i - 1); + // Only reorder blocks within the same EH region -- we don't want to make them non-contiguous + // if (BasicBlock::sameEHRegion(block, blockToMove)) { + // Don't reorder EH regions with filter handlers -- we want the filter to come first + // + if (block->hasHndIndex() && ehGetDsc(block->getHndIndex())->HasFilter()) + { + continue; + } + fgUnlinkBlock(blockToMove); fgInsertBBafter(block, blockToMove); } } - // The RPO won't change the entry blocks of any try regions, but reordering can change the last block in a region + // Fix up call-finally pairs + // + for (BlockToBlockMap::Node* const iter : BlockToBlockMap::KeyValueIteration(&callFinallyPairs)) + { + BasicBlock* const callFinally = iter->GetKey(); + BasicBlock* const callFinallyRet = iter->GetValue(); + fgUnlinkBlock(callFinallyRet); + fgInsertBBafter(callFinally, callFinallyRet); + } + + // The RPO won't change the entry blocks of any EH regions, but reordering can change the last block in a region // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). - // First, determine the new try region ends. + // First, determine the new EH region ends. // BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; + bool* const tryRegionInMainBody = new (this, CMK_Generic) bool[compHndBBtabCount] {}; + BasicBlock** const hndRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - // If we aren't using EH funclets, fgFirstFuncletBB is nullptr, and we'll traverse the entire blocklist. - // Else if we do have funclets, don't extend the end of a try region to include its funclet blocks. - // - for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = block->Next()) + for (BasicBlock* const block : Blocks(fgFirstBB, fgLastBBInMainFunction())) { if (block->hasTryIndex()) + { + const unsigned tryIndex = block->getTryIndex(); + tryRegionEnds[tryIndex] = block; + tryRegionInMainBody[tryIndex] = true; + } + + if (block->hasHndIndex()) + { + hndRegionEnds[block->getHndIndex()] = block; + } + } + + for (BasicBlock* const block : Blocks(fgFirstFuncletBB)) + { + if (block->hasHndIndex()) + { + hndRegionEnds[block->getHndIndex()] = block; + } + + if (block->hasTryIndex() && !tryRegionInMainBody[block->getTryIndex()]) { tryRegionEnds[block->getTryIndex()] = block; } } - // Now, update the EH descriptors + // Now, update the EH descriptors, starting with the try regions // unsigned XTnum; EHblkDsc* HBtab; @@ -4649,6 +4692,36 @@ void Compiler::fgDoReversePostOrderLayout() } } } + + // Now, do the handler regions + // + for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) + { + BasicBlock* const hndEnd = hndRegionEnds[XTnum]; + assert(hndEnd != nullptr); + + // Update the end pointer of this handler region to the new last block + // + HBtab->ebdHndLast = hndEnd; + const unsigned enclosingHndIndex = HBtab->ebdEnclosingHndIndex; + + // If this handler region is nested in another one, we might need to update its enclosing region's end block + // + if (enclosingHndIndex != EHblkDsc::NO_ENCLOSING_INDEX) + { + BasicBlock* const enclosingHndEnd = hndRegionEnds[enclosingHndIndex]; + assert(enclosingHndEnd != nullptr); + + // If the enclosing region ends right before the nested region begins, + // extend the enclosing region's last block to the end of the nested region. + // + BasicBlock* const hndBeg = HBtab->HasFilter() ? HBtab->ebdFilter : HBtab->ebdHndBeg; + if (enclosingHndEnd->NextIs(hndBeg)) + { + hndRegionEnds[enclosingHndIndex] = hndEnd; + } + } + } } //------------------------------------------------------------- diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index b69c68babc092..c496ddca0acdc 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4064,7 +4064,6 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) // fgComputeDfs: Compute a depth-first search tree for the flow graph. // // Type parameters: -// skipEH - If true, does not iterate EH successors // useProfile - If true, determines order of successors visited using profile data // // Returns: @@ -4074,7 +4073,7 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) // Preorder and postorder numbers are assigned into the BasicBlock structure. // The tree returned contains a postorder of the basic blocks. // -template +template FlowGraphDfsTree* Compiler::fgComputeDfs() { BasicBlock** postOrder = new (this, CMK_DepthFirstSearch) BasicBlock*[fgBBcount]; @@ -4100,19 +4099,13 @@ FlowGraphDfsTree* Compiler::fgComputeDfs() } }; - unsigned numBlocks = - skipEH ? fgRunDfs(visitPreorder, visitPostorder, visitEdge) - : fgRunDfs(visitPreorder, visitPostorder, visitEdge); + unsigned numBlocks = fgRunDfs(visitPreorder, visitPostorder, visitEdge); return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks, hasCycle); } // Add explicit instantiations. -template FlowGraphDfsTree* Compiler::fgComputeDfs(); -template FlowGraphDfsTree* Compiler::fgComputeDfs(); -template FlowGraphDfsTree* Compiler::fgComputeDfs(); -template FlowGraphDfsTree* Compiler::fgComputeDfs(); +template FlowGraphDfsTree* Compiler::fgComputeDfs(); +template FlowGraphDfsTree* Compiler::fgComputeDfs(); //------------------------------------------------------------------------ // fgInvalidateDfsTree: Invalidate computed DFS tree and dependent annotations From 23c6840663f067b58cfd7ab3c2e8d0d8560471b6 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sat, 27 Apr 2024 19:00:51 -0400 Subject: [PATCH 08/16] Style --- src/coreclr/jit/block.cpp | 14 ++++++++++---- src/coreclr/jit/compiler.hpp | 5 +---- src/coreclr/jit/fgopt.cpp | 16 +++++++++------- src/coreclr/jit/flowgraph.cpp | 5 ++++- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index ee12e099c19f8..ecee292264ec8 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -145,7 +145,9 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block : m_block(block) { m_numSuccs = 0; - block->VisitAllSuccs(comp, [this](BasicBlock* succ) { + block->VisitAllSuccs( + comp, + [this](BasicBlock* succ) { if (m_numSuccs < ArrLen(m_successors)) { m_successors[m_numSuccs] = succ; @@ -153,18 +155,22 @@ AllSuccessorEnumerator::AllSuccessorEnumerator(Compiler* comp, BasicBlock* block m_numSuccs++; return BasicBlockVisit::Continue; - }, useProfile); + }, + useProfile); if (m_numSuccs > ArrLen(m_successors)) { m_pSuccessors = new (comp, CMK_BasicBlock) BasicBlock*[m_numSuccs]; unsigned numSuccs = 0; - block->VisitAllSuccs(comp, [this, &numSuccs](BasicBlock* succ) { + block->VisitAllSuccs( + comp, + [this, &numSuccs](BasicBlock* succ) { assert(numSuccs < m_numSuccs); m_pSuccessors[numSuccs++] = succ; return BasicBlockVisit::Continue; - }, useProfile); + }, + useProfile); assert(numSuccs == m_numSuccs); } diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index befa60bfb7f14..d069bb72a3ca4 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4765,10 +4765,7 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // Returns: // Number of blocks visited. // -template +template unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { BitVecTraits traits(fgBBNumMax + 1, this); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 66c0f5b3e3876..e289d47559ba0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4610,7 +4610,7 @@ void Compiler::fgDoReversePostOrderLayout() // for (BlockToBlockMap::Node* const iter : BlockToBlockMap::KeyValueIteration(&callFinallyPairs)) { - BasicBlock* const callFinally = iter->GetKey(); + BasicBlock* const callFinally = iter->GetKey(); BasicBlock* const callFinallyRet = iter->GetValue(); fgUnlinkBlock(callFinallyRet); fgInsertBBafter(callFinally, callFinallyRet); @@ -4620,16 +4620,16 @@ void Compiler::fgDoReversePostOrderLayout() // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). // First, determine the new EH region ends. // - BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - bool* const tryRegionInMainBody = new (this, CMK_Generic) bool[compHndBBtabCount] {}; - BasicBlock** const hndRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; + BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; + bool* const tryRegionInMainBody = new (this, CMK_Generic) bool[compHndBBtabCount]{}; + BasicBlock** const hndRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; for (BasicBlock* const block : Blocks(fgFirstBB, fgLastBBInMainFunction())) { if (block->hasTryIndex()) { - const unsigned tryIndex = block->getTryIndex(); - tryRegionEnds[tryIndex] = block; + const unsigned tryIndex = block->getTryIndex(); + tryRegionEnds[tryIndex] = block; tryRegionInMainBody[tryIndex] = true; } @@ -4697,12 +4697,14 @@ void Compiler::fgDoReversePostOrderLayout() // for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) { + // The end of each handler region should have been visited by iterating the blocklist above + // BasicBlock* const hndEnd = hndRegionEnds[XTnum]; assert(hndEnd != nullptr); // Update the end pointer of this handler region to the new last block // - HBtab->ebdHndLast = hndEnd; + HBtab->ebdHndLast = hndEnd; const unsigned enclosingHndIndex = HBtab->ebdEnclosingHndIndex; // If this handler region is nested in another one, we might need to update its enclosing region's end block diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index c496ddca0acdc..457a3cde82fff 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4099,7 +4099,10 @@ FlowGraphDfsTree* Compiler::fgComputeDfs() } }; - unsigned numBlocks = fgRunDfs(visitPreorder, visitPostorder, visitEdge); + unsigned numBlocks = + fgRunDfs(visitPreorder, + visitPostorder, + visitEdge); return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks, hasCycle); } From ffdf8ad14c904bbda10cbd0a431f721365a5bbb8 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Sat, 27 Apr 2024 19:03:10 -0400 Subject: [PATCH 09/16] Fix comment --- src/coreclr/jit/compiler.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index d069bb72a3ca4..ff6ebf43ebf20 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -705,8 +705,8 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func, const bool // VisitRegularSuccs: Visit regular successors of this block. // // Arguments: -// comp - Compiler instance -// func - Callback +// comp - Compiler instance +// func - Callback // // Returns: // Whether or not the visiting was aborted. From 1d267cdb75de94ac2a85aa84bd7acf62d379818b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 29 Apr 2024 23:33:26 -0400 Subject: [PATCH 10/16] Add fgMoveColdBlocks --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/fgopt.cpp | 131 +++++++++++++++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5bab6475b902d..4671952d9a22f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6055,6 +6055,7 @@ class Compiler bool fgReorderBlocks(bool useProfile); void fgDoReversePostOrderLayout(); + void fgMoveColdBlocks(); bool fgFuncletsAreCold(); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e289d47559ba0..b56c7b6e08226 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3417,6 +3417,7 @@ bool Compiler::fgReorderBlocks(bool useProfile) if (JitConfig.JitDoReversePostOrderLayout()) { fgDoReversePostOrderLayout(); + fgMoveColdBlocks(); #ifdef DEBUG if (expensiveDebugCheckLevel >= 2) @@ -4654,11 +4655,10 @@ void Compiler::fgDoReversePostOrderLayout() // Now, update the EH descriptors, starting with the try regions // - unsigned XTnum; - EHblkDsc* HBtab; - for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) + unsigned XTnum = 0; + for (EHblkDsc* const HBtab : EHClauses(this)) { - BasicBlock* const tryEnd = tryRegionEnds[XTnum]; + BasicBlock* const tryEnd = tryRegionEnds[XTnum++]; // We can have multiple EH descriptors map to the same try region, // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, @@ -4695,11 +4695,12 @@ void Compiler::fgDoReversePostOrderLayout() // Now, do the handler regions // - for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) + XTnum = 0; + for (EHblkDsc* const HBtab : EHClauses(this)) { // The end of each handler region should have been visited by iterating the blocklist above // - BasicBlock* const hndEnd = hndRegionEnds[XTnum]; + BasicBlock* const hndEnd = hndRegionEnds[XTnum++]; assert(hndEnd != nullptr); // Update the end pointer of this handler region to the new last block @@ -4726,6 +4727,124 @@ void Compiler::fgDoReversePostOrderLayout() } } +//----------------------------------------------------------------------------- +// fgMoveColdBlocks: Move rarely-run blocks to the end of their respective regions. +// +// Notes: +// Exception handlers are assumed to be cold, so we won't move blocks within them. +// +void Compiler::fgMoveColdBlocks() +{ +#ifdef DEBUG + if (verbose) + { + printf("*************** In fgMoveColdBlocks()\n"); + + printf("\nInitial BasicBlocks"); + fgDispBasicBlocks(verboseTrees); + printf("\n"); + } +#endif // DEBUG + + // In a single pass, find the end blocks of each region that we can use as insertion points for cold blocks + // + BasicBlock** const lastColdTryBlocks = + (compHndBBtabCount == 0) ? nullptr : new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; + BasicBlock* const lastMainBB = fgLastBBInMainFunction(); + BasicBlock* lastColdMainBlock = nullptr; + + for (BasicBlock* const block : Blocks(fgFirstBB, lastMainBB)) + { + // If a region ends with a call-finally pair, don't split the pair up. + // Instead, set the end of the region to the BBJ_CALLFINALLY block in the pair. + // Also, don't consider blocks in handler regions. + // (If all of some try region X is enclosed in an exception handler, + // lastColdTryBlocks[X] will be null. We will handle this case later.) + // + if (!block->isBBCallFinallyPairTail() && !block->hasHndIndex()) + { + if (block->hasTryIndex()) + { + lastColdTryBlocks[block->getTryIndex()] = block; + } + else + { + lastColdMainBlock = block; + } + } + } + + // Returns true if block was moved to before insertionPoint, false otherwise + // + auto tryMovingBlock = [this](BasicBlock* const block, BasicBlock* const insertionPoint) -> bool { + assert(block != nullptr); + assert(!block->IsFirst()); + + // We don't save insertion points for handler regions, + // so if we're in one, insertionPoint will be null. + // This shouldn't apply in any other case. + // + if (insertionPoint == nullptr) + { + assert(block->hasHndIndex()); + return false; + } + + // If we're in a handler region inside a try region, + // we might have an insertion point saved, + // but we don't bother with moving blocks within handler regions + // + if (block->hasHndIndex()) + { + return false; + } + + // At this point, block and insertionPoint should be in the same try region, + // and should not be in a handler region + // + assert(BasicBlock::sameEHRegion(block, insertionPoint)); + + if (block == insertionPoint) + { + return false; + } + + // Don't move any part of a call-finally pair -- these need to stay together + // + if (block->isBBCallFinallyPair() || block->isBBCallFinallyPairTail()) + { + return false; + } + + // Don't move the entry to a try region + // + if (this->bbIsTryBeg(block)) + { + return false; + } + + this->fgUnlinkBlock(block); + this->fgInsertBBbefore(insertionPoint, block); + return true; + }; + + // Search the main method body for rarely-run blocks to move + // + for (BasicBlock* block = lastMainBB; block != fgFirstBB;) + { + BasicBlock* const prev = block->Prev(); + BasicBlock** const lastColdBlockPtr = + block->hasTryIndex() ? (lastColdTryBlocks + block->getTryIndex()) : &lastColdMainBlock; + + if (block->isRunRarely() && tryMovingBlock(block, *lastColdBlockPtr)) + { + *lastColdBlockPtr = block; + } + + block = prev; + } +} + //------------------------------------------------------------- // fgUpdateFlowGraphPhase: run flow graph optimization as a // phase, with no tail duplication From 9b08c4ee13cff22f39e39aa8ee45d261de729884 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 30 Apr 2024 14:46:01 -0400 Subject: [PATCH 11/16] fgDoReversePostOrderLayout feedback --- src/coreclr/jit/compiler.hpp | 4 ++ src/coreclr/jit/fgopt.cpp | 78 ++++++++++++++++++++++-------------- 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index ff6ebf43ebf20..d8844c2e35afb 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -669,6 +669,10 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func, const bool } else if (useProfile && (GetTrueEdge()->getLikelihood() < GetFalseEdge()->getLikelihood())) { + // When building an RPO-based block layout, we want to visit the unlikely successor first + // so that in the DFS computation, the likely successor will be processed right before this block, + // meaning the RPO-based layout will enable fall-through into the likely successor. + // RETURN_ON_ABORT(func(GetTrueTarget())); RETURN_ON_ABORT(func(GetFalseTarget())); } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index b56c7b6e08226..943a7f141fb88 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3418,14 +3418,6 @@ bool Compiler::fgReorderBlocks(bool useProfile) { fgDoReversePostOrderLayout(); fgMoveColdBlocks(); - -#ifdef DEBUG - if (expensiveDebugCheckLevel >= 2) - { - fgDebugCheckBBlist(); - } -#endif // DEBUG - return true; } @@ -4553,7 +4545,7 @@ void Compiler::fgDoReversePostOrderLayout() } #endif // DEBUG - // Compute DFS of main function body's blocks, using profile data to determine the order successors are visited in + // Compute DFS of all blocks in the method, using profile data to determine the order successors are visited in // FlowGraphDfsTree* const dfsTree = fgComputeDfs(); @@ -4572,15 +4564,41 @@ void Compiler::fgDoReversePostOrderLayout() return; } + // The RPO will scramble the EH regions, requiring us to correct their state. + // To do so, we will need to determine the new end blocks of each region. + // + struct EHLayoutInfo + { + BasicBlock* tryRegionEnd; + BasicBlock* hndRegionEnd; + bool tryRegionInMainBody; + + // Default constructor provided so we can call ArrayStack::Emplace + // + EHLayoutInfo() = default; + }; + + ArrayStack regions(getAllocator(), compHndBBtabCount); + // The RPO will break up call-finally pairs, so save them before re-ordering // BlockToBlockMap callFinallyPairs(getAllocator()); - for (BasicBlock* const block : Blocks()) + for (EHblkDsc* const HBtab : EHClauses(this)) { - if (block->isBBCallFinallyPair()) + // Default-initialize a EHLayoutInfo for each EH clause + regions.Emplace(); + + if (HBtab->HasFinallyHandler()) { - callFinallyPairs.Set(block, block->Next()); + for (BasicBlock* const pred : HBtab->ebdHndBeg->PredBlocks()) + { + assert(pred->KindIs(BBJ_CALLFINALLY)); + if (pred->isBBCallFinallyPair()) + { + callFinallyPairs.Set(pred, pred->Next()); + } + } } } @@ -4621,22 +4639,19 @@ void Compiler::fgDoReversePostOrderLayout() // (for example, by pushing throw blocks unreachable via normal flow to the end of the region). // First, determine the new EH region ends. // - BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - bool* const tryRegionInMainBody = new (this, CMK_Generic) bool[compHndBBtabCount]{}; - BasicBlock** const hndRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; for (BasicBlock* const block : Blocks(fgFirstBB, fgLastBBInMainFunction())) { if (block->hasTryIndex()) { - const unsigned tryIndex = block->getTryIndex(); - tryRegionEnds[tryIndex] = block; - tryRegionInMainBody[tryIndex] = true; + EHLayoutInfo& layoutInfo = regions.TopRef(block->getTryIndex()); + layoutInfo.tryRegionEnd = block; + layoutInfo.tryRegionInMainBody = true; } if (block->hasHndIndex()) { - hndRegionEnds[block->getHndIndex()] = block; + regions.TopRef(block->getHndIndex()).hndRegionEnd = block; } } @@ -4644,12 +4659,17 @@ void Compiler::fgDoReversePostOrderLayout() { if (block->hasHndIndex()) { - hndRegionEnds[block->getHndIndex()] = block; + regions.TopRef(block->getHndIndex()).hndRegionEnd = block; } - if (block->hasTryIndex() && !tryRegionInMainBody[block->getTryIndex()]) + if (block->hasTryIndex()) { - tryRegionEnds[block->getTryIndex()] = block; + EHLayoutInfo& layoutInfo = regions.TopRef(block->getTryIndex()); + + if (!layoutInfo.tryRegionInMainBody) + { + layoutInfo.tryRegionEnd = block; + } } } @@ -4658,7 +4678,7 @@ void Compiler::fgDoReversePostOrderLayout() unsigned XTnum = 0; for (EHblkDsc* const HBtab : EHClauses(this)) { - BasicBlock* const tryEnd = tryRegionEnds[XTnum++]; + BasicBlock* const tryEnd = regions.TopRef(XTnum++).tryRegionEnd; // We can have multiple EH descriptors map to the same try region, // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, @@ -4679,7 +4699,7 @@ void Compiler::fgDoReversePostOrderLayout() // if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) { - BasicBlock* const enclosingTryEnd = tryRegionEnds[enclosingTryIndex]; + BasicBlock* const enclosingTryEnd = regions.TopRef(enclosingTryIndex).tryRegionEnd; // If multiple EH descriptors map to the same try region, // then the enclosing region's last block might be null in the table, so set it here. @@ -4688,7 +4708,7 @@ void Compiler::fgDoReversePostOrderLayout() // if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) { - tryRegionEnds[enclosingTryIndex] = tryEnd; + regions.TopRef(enclosingTryIndex).tryRegionEnd = tryEnd; } } } @@ -4700,7 +4720,7 @@ void Compiler::fgDoReversePostOrderLayout() { // The end of each handler region should have been visited by iterating the blocklist above // - BasicBlock* const hndEnd = hndRegionEnds[XTnum++]; + BasicBlock* const hndEnd = regions.TopRef(XTnum++).hndRegionEnd; assert(hndEnd != nullptr); // Update the end pointer of this handler region to the new last block @@ -4712,7 +4732,7 @@ void Compiler::fgDoReversePostOrderLayout() // if (enclosingHndIndex != EHblkDsc::NO_ENCLOSING_INDEX) { - BasicBlock* const enclosingHndEnd = hndRegionEnds[enclosingHndIndex]; + BasicBlock* const enclosingHndEnd = regions.TopRef(enclosingHndIndex).hndRegionEnd; assert(enclosingHndEnd != nullptr); // If the enclosing region ends right before the nested region begins, @@ -4721,7 +4741,7 @@ void Compiler::fgDoReversePostOrderLayout() BasicBlock* const hndBeg = HBtab->HasFilter() ? HBtab->ebdFilter : HBtab->ebdHndBeg; if (enclosingHndEnd->NextIs(hndBeg)) { - hndRegionEnds[enclosingHndIndex] = hndEnd; + regions.TopRef(enclosingHndIndex).hndRegionEnd = hndEnd; } } } @@ -4759,7 +4779,7 @@ void Compiler::fgMoveColdBlocks() // Instead, set the end of the region to the BBJ_CALLFINALLY block in the pair. // Also, don't consider blocks in handler regions. // (If all of some try region X is enclosed in an exception handler, - // lastColdTryBlocks[X] will be null. We will handle this case later.) + // lastColdTryBlocks[X] will be null. We will check for this case in tryMovingBlock below.) // if (!block->isBBCallFinallyPairTail() && !block->hasHndIndex()) { From 9dfe4dc2d7e605a794801c1b1fb0c94624827403 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 1 May 2024 11:36:37 -0400 Subject: [PATCH 12/16] fgMoveColdBlocks feedback --- src/coreclr/jit/fgopt.cpp | 271 ++++++++++++++++++++++++++++++-------- 1 file changed, 213 insertions(+), 58 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 943a7f141fb88..24c257ab01d1d 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4578,7 +4578,7 @@ void Compiler::fgDoReversePostOrderLayout() EHLayoutInfo() = default; }; - ArrayStack regions(getAllocator(), compHndBBtabCount); + ArrayStack regions(getAllocator(CMK_ArrayStack), compHndBBtabCount); // The RPO will break up call-finally pairs, so save them before re-ordering // @@ -4766,102 +4766,257 @@ void Compiler::fgMoveColdBlocks() } #endif // DEBUG - // In a single pass, find the end blocks of each region that we can use as insertion points for cold blocks - // - BasicBlock** const lastColdTryBlocks = - (compHndBBtabCount == 0) ? nullptr : new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - BasicBlock* const lastMainBB = fgLastBBInMainFunction(); - BasicBlock* lastColdMainBlock = nullptr; - - for (BasicBlock* const block : Blocks(fgFirstBB, lastMainBB)) - { - // If a region ends with a call-finally pair, don't split the pair up. - // Instead, set the end of the region to the BBJ_CALLFINALLY block in the pair. - // Also, don't consider blocks in handler regions. - // (If all of some try region X is enclosed in an exception handler, - // lastColdTryBlocks[X] will be null. We will check for this case in tryMovingBlock below.) + auto moveColdMainBlocks = [this]() { + // Find the last block in the main body that isn't part of an EH region // - if (!block->isBBCallFinallyPairTail() && !block->hasHndIndex()) + BasicBlock* lastMainBB; + for (lastMainBB = this->fgLastBBInMainFunction(); lastMainBB != nullptr; lastMainBB = lastMainBB->Prev()) { - if (block->hasTryIndex()) + if (!lastMainBB->hasTryIndex() && !lastMainBB->hasHndIndex()) { - lastColdTryBlocks[block->getTryIndex()] = block; + break; } - else + } + + // Nothing to do if there are two or fewer non-EH blocks + // + if ((lastMainBB == nullptr) || lastMainBB->IsFirst() || lastMainBB->PrevIs(fgFirstBB)) + { + return; + } + + // Search the main method body for rarely-run blocks to move + // + BasicBlock* prev; + for (BasicBlock* block = lastMainBB->Prev(); block != fgFirstBB; block = prev) + { + prev = block->Prev(); + + // We only want to move cold blocks. + // Also, don't consider blocks in EH regions for now; only move blocks in the main method body. + // Finally, don't move block if it is the beginning of a call-finally pair, + // as we want to keep these pairs contiguous + // (if we encounter the end of a pair below, we'll move the whole pair). + // + if (!block->isRunRarely() || block->hasTryIndex() || block->hasHndIndex() || block->isBBCallFinallyPair()) { - lastColdMainBlock = block; + continue; + } + + this->fgUnlinkBlock(block); + this->fgInsertBBafter(lastMainBB, block); + + // If block is the end of a call-finally pair, prev is the beginning of the pair. + // Move prev to before block to keep the pair contiguous. + // + if (block->KindIs(BBJ_CALLFINALLYRET)) + { + BasicBlock* const callFinally = prev; + prev = prev->Prev(); + assert(callFinally->KindIs(BBJ_CALLFINALLY)); + assert(!callFinally->HasFlag(BBF_RETLESS_CALL)); + this->fgUnlinkBlock(callFinally); + this->fgInsertBBafter(lastMainBB, callFinally); + } + } + + // We have moved all cold main blocks before lastMainBB to after lastMainBB. + // If lastMainBB itself is cold, move it to the end of the method to restore its relative ordering. + // + if (lastMainBB->isRunRarely()) + { + BasicBlock* const newLastMainBB = this->fgLastBBInMainFunction(); + if (lastMainBB != newLastMainBB) + { + BasicBlock* const prev = lastMainBB->Prev(); + this->fgUnlinkBlock(lastMainBB); + this->fgInsertBBafter(newLastMainBB, lastMainBB); + + // Call-finally check + // + if (lastMainBB->KindIs(BBJ_CALLFINALLYRET)) + { + assert(prev->KindIs(BBJ_CALLFINALLY)); + assert(!prev->HasFlag(BBF_RETLESS_CALL)); + assert(prev != newLastMainBB); + this->fgUnlinkBlock(prev); + this->fgInsertBBafter(newLastMainBB, prev); + } } } + }; + + moveColdMainBlocks(); + + // No EH regions + // + if (compHndBBtabCount == 0) + { + return; } - // Returns true if block was moved to before insertionPoint, false otherwise + // We assume exception handlers are cold, so we won't bother moving blocks within them. + // We will move blocks only within try regions. + // First, determine where each try region ends, without considering nested regions. + // We will use these end blocks as insertion points. // - auto tryMovingBlock = [this](BasicBlock* const block, BasicBlock* const insertionPoint) -> bool { - assert(block != nullptr); - assert(!block->IsFirst()); + BasicBlock** const tryRegionEnds = new (this, CMK_Generic) BasicBlock* [compHndBBtabCount] {}; - // We don't save insertion points for handler regions, - // so if we're in one, insertionPoint will be null. - // This shouldn't apply in any other case. - // - if (insertionPoint == nullptr) + for (BasicBlock* const block : Blocks(fgFirstBB, fgLastBBInMainFunction())) + { + if (block->hasTryIndex()) { - assert(block->hasHndIndex()); - return false; + tryRegionEnds[block->getTryIndex()] = block; } + } + + // Search all try regions in the main method body for cold blocks to move + // + BasicBlock* prev; + for (BasicBlock* block = fgLastBBInMainFunction(); block != fgFirstBB; block = prev) + { + prev = block->Prev(); - // If we're in a handler region inside a try region, - // we might have an insertion point saved, - // but we don't bother with moving blocks within handler regions + // Only consider rarely-run blocks in try regions. + // If we have such a block that is also part of an exception handler, don't bother moving it. + // Finally, don't move block if it is the beginning of a call-finally pair, + // as we want to keep these pairs contiguous + // (if we encounter the end of a pair below, we'll move the whole pair). // - if (block->hasHndIndex()) + if (!block->hasTryIndex() || !block->isRunRarely() || block->hasHndIndex() || block->isBBCallFinallyPair()) { - return false; + continue; } - // At this point, block and insertionPoint should be in the same try region, - // and should not be in a handler region + const unsigned tryIndex = block->getTryIndex(); + EHblkDsc* const HBtab = ehGetDsc(tryIndex); + + // Don't move the beginning of a try region. + // Also, if this try region's entry is cold, don't bother moving its blocks. // - assert(BasicBlock::sameEHRegion(block, insertionPoint)); + if ((HBtab->ebdTryBeg == block) || (HBtab->ebdTryBeg->isRunRarely())) + { + continue; + } + BasicBlock* const insertionPoint = tryRegionEnds[tryIndex]; + assert(insertionPoint != nullptr); + + // Don't move the end of this try region + // if (block == insertionPoint) { - return false; + continue; + } + + fgUnlinkBlock(block); + fgInsertBBafter(insertionPoint, block); + + // Keep call-finally pairs contiguous + // + if (block->KindIs(BBJ_CALLFINALLYRET)) + { + BasicBlock* const callFinally = prev; + prev = prev->Prev(); + assert(callFinally->KindIs(BBJ_CALLFINALLY)); + assert(!callFinally->HasFlag(BBF_RETLESS_CALL)); + fgUnlinkBlock(callFinally); + fgInsertBBafter(insertionPoint, callFinally); } + } - // Don't move any part of a call-finally pair -- these need to stay together + // Before updating EH descriptors, find the new try region ends + // + for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) + { + BasicBlock* const tryEnd = tryRegionEnds[XTnum]; + + // This try region isn't in the main method body // - if (block->isBBCallFinallyPair() || block->isBBCallFinallyPairTail()) + if (tryEnd == nullptr) { - return false; + continue; } - // Don't move the entry to a try region + // If we moved cold blocks to the end of this try region, + // search for the new end block // - if (this->bbIsTryBeg(block)) + BasicBlock* newTryEnd = tryEnd; + for (BasicBlock* const block : Blocks(tryEnd, fgLastBBInMainFunction())) { - return false; + if (!BasicBlock::sameTryRegion(tryEnd, block)) + { + break; + } + + newTryEnd = block; } - this->fgUnlinkBlock(block); - this->fgInsertBBbefore(insertionPoint, block); - return true; - }; + // We moved cold blocks to the end of this try region, but the old end block is cold, too. + // Move the old end block to the end of the region to preserve its relative ordering. + // + if ((tryEnd != newTryEnd) && tryEnd->isRunRarely() && !tryEnd->hasHndIndex()) + { + BasicBlock* const prev = tryEnd->Prev(); + fgUnlinkBlock(tryEnd); + fgInsertBBafter(newTryEnd, tryEnd); - // Search the main method body for rarely-run blocks to move + // Keep call-finally pairs contiguous + // + if (tryEnd->KindIs(BBJ_CALLFINALLYRET)) + { + assert(prev->KindIs(BBJ_CALLFINALLY)); + assert(!prev->HasFlag(BBF_RETLESS_CALL)); + fgUnlinkBlock(prev); + fgInsertBBafter(newTryEnd, prev); + } + } + else + { + // Otherwise, just update the try region end + // + tryRegionEnds[XTnum] = newTryEnd; + } + } + + // Now, update EH descriptors // - for (BasicBlock* block = lastMainBB; block != fgFirstBB;) + for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) { - BasicBlock* const prev = block->Prev(); - BasicBlock** const lastColdBlockPtr = - block->hasTryIndex() ? (lastColdTryBlocks + block->getTryIndex()) : &lastColdMainBlock; + BasicBlock* const tryEnd = tryRegionEnds[XTnum]; - if (block->isRunRarely() && tryMovingBlock(block, *lastColdBlockPtr)) + // We can have multiple EH descriptors map to the same try region, + // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, + // so the duplicate EH descriptors' last block pointers can be null. + // Tolerate this. + // + if (tryEnd == nullptr) { - *lastColdBlockPtr = block; + continue; } - block = prev; + // Update the end pointer of this try region to the new last block + // + EHblkDsc* const HBtab = ehGetDsc(XTnum); + HBtab->ebdTryLast = tryEnd; + const unsigned enclosingTryIndex = HBtab->ebdEnclosingTryIndex; + + // If this try region is nested in another one, we might need to update its enclosing region's end block + // + if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) + { + BasicBlock* const enclosingTryEnd = tryRegionEnds[enclosingTryIndex]; + + // If multiple EH descriptors map to the same try region, + // then the enclosing region's last block might be null in the table, so set it here. + // Similarly, if the enclosing region ends right before the nested region begins, + // extend the enclosing region's last block to the end of the nested region. + // + if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) + { + tryRegionEnds[enclosingTryIndex] = tryEnd; + } + } } } From 04cecca100535d888f6af61fb364e9a453d5da27 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 1 May 2024 12:46:24 -0400 Subject: [PATCH 13/16] Reduce code dup --- src/coreclr/jit/compiler.h | 3 ++ src/coreclr/jit/fgopt.cpp | 87 ++++++++++++++++++-------------------- 2 files changed, 44 insertions(+), 46 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4671952d9a22f..21914eebea90a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2793,6 +2793,9 @@ class Compiler EHblkDsc* ehIsBlockHndLast(BasicBlock* block); bool ehIsBlockEHLast(BasicBlock* block); + template + void ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast); + bool ehBlockHasExnFlowDsc(BasicBlock* block); // Return the region index of the most nested EH region this block is in. diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 24c257ab01d1d..f7cd4d1b4609e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4675,47 +4675,19 @@ void Compiler::fgDoReversePostOrderLayout() // Now, update the EH descriptors, starting with the try regions // - unsigned XTnum = 0; - for (EHblkDsc* const HBtab : EHClauses(this)) - { - BasicBlock* const tryEnd = regions.TopRef(XTnum++).tryRegionEnd; - - // We can have multiple EH descriptors map to the same try region, - // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, - // so the duplicate EH descriptors' last block pointers can be null. - // Tolerate this. - // - if (tryEnd == nullptr) - { - continue; - } - - // Update the end pointer of this try region to the new last block - // - HBtab->ebdTryLast = tryEnd; - const unsigned enclosingTryIndex = HBtab->ebdEnclosingTryIndex; + auto getTryLast = [®ions](const unsigned index) -> BasicBlock* { + return regions.TopRef(index).tryRegionEnd; + }; - // If this try region is nested in another one, we might need to update its enclosing region's end block - // - if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) - { - BasicBlock* const enclosingTryEnd = regions.TopRef(enclosingTryIndex).tryRegionEnd; + auto setTryLast = [®ions](const unsigned index, BasicBlock* const block) { + regions.TopRef(index).tryRegionEnd = block; + }; - // If multiple EH descriptors map to the same try region, - // then the enclosing region's last block might be null in the table, so set it here. - // Similarly, if the enclosing region ends right before the nested region begins, - // extend the enclosing region's last block to the end of the nested region. - // - if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) - { - regions.TopRef(enclosingTryIndex).tryRegionEnd = tryEnd; - } - } - } + ehUpdateTryLasts(getTryLast, setTryLast); // Now, do the handler regions // - XTnum = 0; + unsigned XTnum = 0; for (EHblkDsc* const HBtab : EHClauses(this)) { // The end of each handler region should have been visited by iterating the blocklist above @@ -4981,15 +4953,39 @@ void Compiler::fgMoveColdBlocks() // Now, update EH descriptors // - for (unsigned XTnum = 0; XTnum < compHndBBtabCount; XTnum++) + auto getTryLast = [tryRegionEnds](const unsigned index) -> BasicBlock* { + return tryRegionEnds[index]; + }; + + auto setTryLast = [tryRegionEnds](const unsigned index, BasicBlock* const block) { + tryRegionEnds[index] = block; + }; + + ehUpdateTryLasts(getTryLast, setTryLast); +} + +//------------------------------------------------------------- +// ehUpdateTryLasts: Iterates EH descriptors, updating each try region's +// end block as determined by getTryLast. +// +// Type parameters: +// GetTryLast - Functor type that takes an EH index, +// and returns the corresponding region's new try end block +// SetTryLast - Functor type that takes an EH index and a BasicBlock*, +// and updates some internal state tracking the new try end block of each EH region +// +// Parameters: +// getTryLast - Functor to get new try end block for an EH region +// setTryLast - Functor to update the new try end block for an EH region +// +template +void Compiler::ehUpdateTryLasts(GetTryLast getTryLast, SetTryLast setTryLast) +{ + unsigned XTnum = 0; + for (EHblkDsc* const HBtab : EHClauses(this)) { - BasicBlock* const tryEnd = tryRegionEnds[XTnum]; + BasicBlock* const tryEnd = getTryLast(XTnum++); - // We can have multiple EH descriptors map to the same try region, - // but we will only update the try region's last block pointer at the index given by BasicBlock::getTryIndex, - // so the duplicate EH descriptors' last block pointers can be null. - // Tolerate this. - // if (tryEnd == nullptr) { continue; @@ -4997,7 +4993,6 @@ void Compiler::fgMoveColdBlocks() // Update the end pointer of this try region to the new last block // - EHblkDsc* const HBtab = ehGetDsc(XTnum); HBtab->ebdTryLast = tryEnd; const unsigned enclosingTryIndex = HBtab->ebdEnclosingTryIndex; @@ -5005,7 +5000,7 @@ void Compiler::fgMoveColdBlocks() // if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) { - BasicBlock* const enclosingTryEnd = tryRegionEnds[enclosingTryIndex]; + BasicBlock* const enclosingTryEnd = getTryLast(enclosingTryIndex); // If multiple EH descriptors map to the same try region, // then the enclosing region's last block might be null in the table, so set it here. @@ -5014,7 +5009,7 @@ void Compiler::fgMoveColdBlocks() // if ((enclosingTryEnd == nullptr) || enclosingTryEnd->NextIs(HBtab->ebdTryBeg)) { - tryRegionEnds[enclosingTryIndex] = tryEnd; + setTryLast(enclosingTryIndex, tryEnd); } } } From fbea16097d27da715dbe986d794c4dc601916307 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 1 May 2024 22:45:35 -0400 Subject: [PATCH 14/16] Renumber blocks --- src/coreclr/jit/fgopt.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index f7cd4d1b4609e..1ef80294e84d0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -3418,6 +3418,12 @@ bool Compiler::fgReorderBlocks(bool useProfile) { fgDoReversePostOrderLayout(); fgMoveColdBlocks(); + + // Renumber blocks to facilitate LSRA's order of block visitation + // TODO: Consider removing this, and using traversal order in lSRA + // + fgRenumberBlocks(); + return true; } From 2abe1a2c778f1dfd932d8ea007aadfdae6a94e40 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 2 May 2024 11:03:51 -0400 Subject: [PATCH 15/16] Disable by default --- 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 54aaafc4273dd..81345191ce5f8 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -755,7 +755,7 @@ RELEASE_CONFIG_INTEGER(JitEnablePhysicalPromotion, W("JitEnablePhysicalPromotion RELEASE_CONFIG_INTEGER(JitEnableCrossBlockLocalAssertionProp, W("JitEnableCrossBlockLocalAssertionProp"), 1) // Do greedy RPO-based layout in Compiler::fgReorderBlocks. -RELEASE_CONFIG_INTEGER(JitDoReversePostOrderLayout, W("JitDoReversePostOrderLayout"), 1); +RELEASE_CONFIG_INTEGER(JitDoReversePostOrderLayout, W("JitDoReversePostOrderLayout"), 0); // JitFunctionFile: Name of a file that contains a list of functions. If the currently compiled function is in the // file, certain other JIT config variables will be active. If the currently compiled function is not in the file, From d2f5ad632b6e5fd2220f9f79b6a3b0b42facbb8a Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 2 May 2024 14:29:59 -0400 Subject: [PATCH 16/16] Feedback --- src/coreclr/jit/fgopt.cpp | 45 +++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 1ef80294e84d0..88dd717fab693 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4588,7 +4588,21 @@ void Compiler::fgDoReversePostOrderLayout() // The RPO will break up call-finally pairs, so save them before re-ordering // - BlockToBlockMap callFinallyPairs(getAllocator()); + struct CallFinallyPair + { + BasicBlock* callFinally; + BasicBlock* callFinallyRet; + + // Constructor provided so we can call ArrayStack::Emplace + // + CallFinallyPair(BasicBlock* first, BasicBlock* second) + : callFinally(first) + , callFinallyRet(second) + { + } + }; + + ArrayStack callFinallyPairs(getAllocator()); for (EHblkDsc* const HBtab : EHClauses(this)) { @@ -4602,7 +4616,7 @@ void Compiler::fgDoReversePostOrderLayout() assert(pred->KindIs(BBJ_CALLFINALLY)); if (pred->isBBCallFinallyPair()) { - callFinallyPairs.Set(pred, pred->Next()); + callFinallyPairs.Emplace(pred, pred->Next()); } } } @@ -4633,12 +4647,11 @@ void Compiler::fgDoReversePostOrderLayout() // Fix up call-finally pairs // - for (BlockToBlockMap::Node* const iter : BlockToBlockMap::KeyValueIteration(&callFinallyPairs)) + for (int i = 0; i < callFinallyPairs.Height(); i++) { - BasicBlock* const callFinally = iter->GetKey(); - BasicBlock* const callFinallyRet = iter->GetValue(); - fgUnlinkBlock(callFinallyRet); - fgInsertBBafter(callFinally, callFinallyRet); + const CallFinallyPair& pair = callFinallyPairs.BottomRef(i); + fgUnlinkBlock(pair.callFinallyRet); + fgInsertBBafter(pair.callFinally, pair.callFinallyRet); } // The RPO won't change the entry blocks of any EH regions, but reordering can change the last block in a region @@ -4650,14 +4663,14 @@ void Compiler::fgDoReversePostOrderLayout() { if (block->hasTryIndex()) { - EHLayoutInfo& layoutInfo = regions.TopRef(block->getTryIndex()); + EHLayoutInfo& layoutInfo = regions.BottomRef(block->getTryIndex()); layoutInfo.tryRegionEnd = block; layoutInfo.tryRegionInMainBody = true; } if (block->hasHndIndex()) { - regions.TopRef(block->getHndIndex()).hndRegionEnd = block; + regions.BottomRef(block->getHndIndex()).hndRegionEnd = block; } } @@ -4665,12 +4678,12 @@ void Compiler::fgDoReversePostOrderLayout() { if (block->hasHndIndex()) { - regions.TopRef(block->getHndIndex()).hndRegionEnd = block; + regions.BottomRef(block->getHndIndex()).hndRegionEnd = block; } if (block->hasTryIndex()) { - EHLayoutInfo& layoutInfo = regions.TopRef(block->getTryIndex()); + EHLayoutInfo& layoutInfo = regions.BottomRef(block->getTryIndex()); if (!layoutInfo.tryRegionInMainBody) { @@ -4682,11 +4695,11 @@ void Compiler::fgDoReversePostOrderLayout() // Now, update the EH descriptors, starting with the try regions // auto getTryLast = [®ions](const unsigned index) -> BasicBlock* { - return regions.TopRef(index).tryRegionEnd; + return regions.BottomRef(index).tryRegionEnd; }; auto setTryLast = [®ions](const unsigned index, BasicBlock* const block) { - regions.TopRef(index).tryRegionEnd = block; + regions.BottomRef(index).tryRegionEnd = block; }; ehUpdateTryLasts(getTryLast, setTryLast); @@ -4698,7 +4711,7 @@ void Compiler::fgDoReversePostOrderLayout() { // The end of each handler region should have been visited by iterating the blocklist above // - BasicBlock* const hndEnd = regions.TopRef(XTnum++).hndRegionEnd; + BasicBlock* const hndEnd = regions.BottomRef(XTnum++).hndRegionEnd; assert(hndEnd != nullptr); // Update the end pointer of this handler region to the new last block @@ -4710,7 +4723,7 @@ void Compiler::fgDoReversePostOrderLayout() // if (enclosingHndIndex != EHblkDsc::NO_ENCLOSING_INDEX) { - BasicBlock* const enclosingHndEnd = regions.TopRef(enclosingHndIndex).hndRegionEnd; + BasicBlock* const enclosingHndEnd = regions.BottomRef(enclosingHndIndex).hndRegionEnd; assert(enclosingHndEnd != nullptr); // If the enclosing region ends right before the nested region begins, @@ -4719,7 +4732,7 @@ void Compiler::fgDoReversePostOrderLayout() BasicBlock* const hndBeg = HBtab->HasFilter() ? HBtab->ebdFilter : HBtab->ebdHndBeg; if (enclosingHndEnd->NextIs(hndBeg)) { - regions.TopRef(enclosingHndIndex).hndRegionEnd = hndEnd; + regions.BottomRef(enclosingHndIndex).hndRegionEnd = hndEnd; } } }