From dacc0353ecaaece2fce0b6d2e40bcb80735b7734 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 7 Nov 2023 15:38:30 -0800 Subject: [PATCH 1/5] JIT: stop computing dom start nodes for morph RPO We don't need quite this broad of a start node set, as the DFS will find unreachable blocks on its own. Also lay the groundwork for removing unreachable blocks, by tracking the postorder number of the last reachable block. Contributes to #93246. --- src/coreclr/jit/compiler.h | 5 +-- src/coreclr/jit/fgopt.cpp | 68 ++++++++++++++++++++++++-------------- src/coreclr/jit/morph.cpp | 20 +++++++++-- 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 77d2e0d35f383..b380752d2e59c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5330,13 +5330,14 @@ class Compiler BasicBlock* fgIntersectDom(BasicBlock* a, BasicBlock* b); // Intersect two immediate dominator sets. void fgDfsReversePostorder(); + unsigned fgDfsReversePostorderCore(BlockSet_ValArg_T startBlocks); void fgDfsReversePostorderHelper(BasicBlock* block, BlockSet& visited, unsigned& preorderIndex, unsigned& reversePostorderIndex); - BlockSet_ValRet_T fgDomFindStartNodes(); // Computes which basic blocks don't have incoming edges in the flow graph. - // Returns this as a set. + BlockSet_ValRet_T fgDomFindStartBlocks(); // Computes which basic blocks don't have incoming edges in the flow graph. + // Returns this as a set. INDEBUG(void fgDispDomTree(DomTreeNode* domTree);) // Helper that prints out the Dominator Tree in debug builds. diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 3efeaf4411d5a..b94f2ffc948d5 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -763,54 +763,70 @@ bool Compiler::fgRemoveDeadBlocks() //------------------------------------------------------------- // fgDfsReversePostorder: Depth first search to establish block -// preorder and reverse postorder numbers, plus a reverse postorder for blocks. +// preorder and reverse postorder numbers, plus a reverse postorder for blocks, +// using all entry blocks and blocks without preds as start lblocks. // // Notes: -// Assumes caller has computed the fgEnterBlks set. -// // Each block's `bbPreorderNum` and `bbPostorderNum` is set. -// // The `fgBBReversePostorder` array is filled in with the `BasicBlock*` in reverse post-order. -// // This algorithm only pays attention to the actual blocks. It ignores any imaginary entry block. // void Compiler::fgDfsReversePostorder() { - assert(fgBBcount == fgBBNumMax); - assert(BasicBlockBitSetTraits::GetSize(this) == fgBBNumMax + 1); - // Make sure fgEnterBlks are still there in startNodes, even if they participate in a loop (i.e., there is // an incoming edge into the block). assert(fgEnterBlksSetValid); - fgBBReversePostorder = new (this, CMK_DominatorMemory) BasicBlock*[fgBBNumMax + 1]{}; - - // visited : Once we run the DFS post order sort recursive algorithm, we mark the nodes we visited to avoid - // backtracking. - BlockSet visited(BlockSetOps::MakeEmpty(this)); - // We begin by figuring out which basic blocks don't have incoming edges and mark them as - // start nodes. Later on we run the recursive algorithm for each node that we + // start blocks. Later on we run the recursive algorithm for each node that we // mark in this step. - BlockSet_ValRet_T startNodes = fgDomFindStartNodes(); + BlockSet_ValRet_T startBlocks = fgDomFindStartBlocks(); + BlockSetOps::UnionD(this, startBlocks, fgEnterBlks); + assert(BlockSetOps::IsMember(this, startBlocks, fgFirstBB->bbNum)); - BlockSetOps::UnionD(this, startNodes, fgEnterBlks); - assert(BlockSetOps::IsMember(this, startNodes, fgFirstBB->bbNum)); + fgDfsReversePostorderCore(startBlocks); +} + +//------------------------------------------------------------- +// fgDfsReversePostorderCore: Depth first search to establish block +// preorder and reverse postorder numbers, plus a reverse postorder for blocks, +// using a specified set of start blocks. +// +// Arguments: +// startBlocks: set of blocks to consider as DFS roots +// +// Returns: +// postorder number of last block reachable from the root set. Any block +// with a postorder number higher than this was not reachable from the root set. +// +// Notes: +// Each block's `bbPreorderNum` and `bbPostorderNum` is set. +// The `fgBBReversePostorder` array is filled in with the `BasicBlock*` in reverse post-order. +// This algorithm only pays attention to the actual blocks. It ignores any imaginary entry block. +// +unsigned Compiler::fgDfsReversePostorderCore(BlockSet_ValArg_T startBlocks) +{ + assert(fgBBcount == fgBBNumMax); + assert(BasicBlockBitSetTraits::GetSize(this) == fgBBNumMax + 1); + fgBBReversePostorder = new (this, CMK_DominatorMemory) BasicBlock*[fgBBNumMax + 1]{}; + BlockSet visited(BlockSetOps::MakeEmpty(this)); // Call the flowgraph DFS traversal helper. unsigned preorderIndex = 1; unsigned postorderIndex = 1; for (BasicBlock* const block : Blocks()) { - // If the block has no predecessors, and we haven't already visited it (because it's in fgEnterBlks but also - // reachable from the first block), go ahead and traverse starting from this block. - if (BlockSetOps::IsMember(this, startNodes, block->bbNum) && + // Spawn a DFS from each unvisited start block. + // + if (BlockSetOps::IsMember(this, startBlocks, block->bbNum) && !BlockSetOps::IsMember(this, visited, block->bbNum)) { fgDfsReversePostorderHelper(block, visited, preorderIndex, postorderIndex); } } + const unsigned lastReachablePostorderNumber = postorderIndex; + // If there are still unvisited blocks (say isolated cycles), visit them too. // if (preorderIndex != fgBBcount + 1) @@ -841,18 +857,20 @@ void Compiler::fgDfsReversePostorder() printf("\n"); } #endif // DEBUG + + return lastReachablePostorderNumber; } //------------------------------------------------------------- -// fgDomFindStartNodes: Helper for dominance computation to find the start nodes block set. +// fgDomFindStartBlocks: Helper for dominance computation to find the start block set. // -// The start nodes is a set that represents which basic blocks in the flow graph don't have incoming edges. +// The start block set represents basic blocks in the flow graph that do not have incoming edges. // We begin assuming everything is a start block and remove any block that is a successor of another. // // Returns: -// Block set of start nodes. +// Block set describing the start blocks. // -BlockSet_ValRet_T Compiler::fgDomFindStartNodes() +BlockSet_ValRet_T Compiler::fgDomFindStartBlocks() { BlockSet startNodes(BlockSetOps::MakeFull(this)); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index ec70cb0025efa..a4196612a7c98 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13871,7 +13871,7 @@ PhaseStatus Compiler::fgMorphBlocks() fgRenumberBlocks(); EnsureBasicBlockEpoch(); fgComputeEnterBlocksSet(); - fgDfsReversePostorder(); + const unsigned lastReachablePostorderNumber = fgDfsReversePostorderCore(fgEnterBlks); // Disallow general creation of new blocks or edges as it // would invalidate RPO. @@ -13894,8 +13894,22 @@ PhaseStatus Compiler::fgMorphBlocks() fgFirstBB->Next()->bbFlags |= BBF_CAN_ADD_PRED; } - unsigned const bbNumMax = fgBBNumMax; - for (unsigned i = 1; i <= bbNumMax; i++) + // Remember this so we can sanity check that no new blocks will get created. + // + INDEBUG(unsigned const bbNumMax = fgBBNumMax;); + + // Todo: verify enter block set is sufficient to allow actual removal here. + // Likely need to add genReturnBB, fgEntryBB (for OSR), and perhaps more. + // + if (lastReachablePostorderNumber < fgBBNumMax) + { + JITDUMP("Method has %u unreachable blocks, consider removing them\n", + fgBBNumMax - lastReachablePostorderNumber); + } + + // Morph the blocks in RPO. + // + for (unsigned i = 1; i <= fgBBNumMax; i++) { BasicBlock* const block = fgBBReversePostorder[i]; fgMorphBlock(block); From 3f9a5b3c3e7961776d73cdfd74dc5f603d24ef49 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Nov 2023 13:57:12 -0800 Subject: [PATCH 2/5] be more ambitious --- src/coreclr/jit/compiler.h | 6 +- src/coreclr/jit/fgopt.cpp | 138 ++++++++++--------------- src/coreclr/jit/fgprofilesynthesis.cpp | 1 - src/coreclr/jit/morph.cpp | 17 +-- 4 files changed, 57 insertions(+), 105 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b380752d2e59c..42a0cdc1f807c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5329,16 +5329,12 @@ class Compiler BasicBlock* fgIntersectDom(BasicBlock* a, BasicBlock* b); // Intersect two immediate dominator sets. - void fgDfsReversePostorder(); - unsigned fgDfsReversePostorderCore(BlockSet_ValArg_T startBlocks); + unsigned fgDfsReversePostorder(); void fgDfsReversePostorderHelper(BasicBlock* block, BlockSet& visited, unsigned& preorderIndex, unsigned& reversePostorderIndex); - BlockSet_ValRet_T fgDomFindStartBlocks(); // Computes which basic blocks don't have incoming edges in the flow graph. - // Returns this as a set. - INDEBUG(void fgDispDomTree(DomTreeNode* domTree);) // Helper that prints out the Dominator Tree in debug builds. DomTreeNode* fgBuildDomTree(); // Once we compute all the immediate dominator sets for each node in the flow graph diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index b94f2ffc948d5..db06c562715e1 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -764,74 +764,80 @@ bool Compiler::fgRemoveDeadBlocks() //------------------------------------------------------------- // fgDfsReversePostorder: Depth first search to establish block // preorder and reverse postorder numbers, plus a reverse postorder for blocks, -// using all entry blocks and blocks without preds as start lblocks. +// using all entry blocks and EH handler blocks as start blocks. // // Notes: // Each block's `bbPreorderNum` and `bbPostorderNum` is set. // The `fgBBReversePostorder` array is filled in with the `BasicBlock*` in reverse post-order. // This algorithm only pays attention to the actual blocks. It ignores any imaginary entry block. // -void Compiler::fgDfsReversePostorder() -{ - // Make sure fgEnterBlks are still there in startNodes, even if they participate in a loop (i.e., there is - // an incoming edge into the block). - assert(fgEnterBlksSetValid); - - // We begin by figuring out which basic blocks don't have incoming edges and mark them as - // start blocks. Later on we run the recursive algorithm for each node that we - // mark in this step. - BlockSet_ValRet_T startBlocks = fgDomFindStartBlocks(); - BlockSetOps::UnionD(this, startBlocks, fgEnterBlks); - assert(BlockSetOps::IsMember(this, startBlocks, fgFirstBB->bbNum)); - - fgDfsReversePostorderCore(startBlocks); -} - -//------------------------------------------------------------- -// fgDfsReversePostorderCore: Depth first search to establish block -// preorder and reverse postorder numbers, plus a reverse postorder for blocks, -// using a specified set of start blocks. -// -// Arguments: -// startBlocks: set of blocks to consider as DFS roots +// Unreachable blocks will have higher pre and post order numbers than reachable blocks. +// Hence they will appear at lower indices in the fgBBReversePostorder array. // -// Returns: -// postorder number of last block reachable from the root set. Any block -// with a postorder number higher than this was not reachable from the root set. -// -// Notes: -// Each block's `bbPreorderNum` and `bbPostorderNum` is set. -// The `fgBBReversePostorder` array is filled in with the `BasicBlock*` in reverse post-order. -// This algorithm only pays attention to the actual blocks. It ignores any imaginary entry block. -// -unsigned Compiler::fgDfsReversePostorderCore(BlockSet_ValArg_T startBlocks) +unsigned Compiler::fgDfsReversePostorder() { assert(fgBBcount == fgBBNumMax); assert(BasicBlockBitSetTraits::GetSize(this) == fgBBNumMax + 1); fgBBReversePostorder = new (this, CMK_DominatorMemory) BasicBlock*[fgBBNumMax + 1]{}; BlockSet visited(BlockSetOps::MakeEmpty(this)); - // Call the flowgraph DFS traversal helper. unsigned preorderIndex = 1; unsigned postorderIndex = 1; - for (BasicBlock* const block : Blocks()) + + // Walk from our primary root. + // + fgDfsReversePostorderHelper(fgFirstBB, visited, preorderIndex, postorderIndex); + + // If we didn't end up visiting everything, try the EH roots. + // + if (preorderIndex != fgBBcount + 1) { - // Spawn a DFS from each unvisited start block. - // - if (BlockSetOps::IsMember(this, startBlocks, block->bbNum) && - !BlockSetOps::IsMember(this, visited, block->bbNum)) + for (EHblkDsc* const HBtab : EHClauses(this)) { - fgDfsReversePostorderHelper(block, visited, preorderIndex, postorderIndex); + if (HBtab->HasFilter()) + { + BasicBlock* const filterBlock = HBtab->ebdFilter; + if (!BlockSetOps::IsMember(this, visited, filterBlock->bbNum)) + { + fgDfsReversePostorderHelper(filterBlock, visited, preorderIndex, postorderIndex); + } + } + + BasicBlock* const handlerBlock = HBtab->ebdHndBeg; + if (!BlockSetOps::IsMember(this, visited, handlerBlock->bbNum)) + { + fgDfsReversePostorderHelper(handlerBlock, visited, preorderIndex, postorderIndex); + } + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + // For ARM code, prevent creating retless calls by visiting the call finally successors + // + if (HBtab->HasFinallyHandler()) + { + for (BasicBlock* const finallyPredBlock : handlerBlock->Preds()) + { + assert(finallyPredBlock->KindIs(BBJ_CALLFINALLY)); + assert(finallyPredBlock->isBBCallAlwaysPair()); + + if (!BlockSetOps::IsMember(this, visited, finallyPredBlock->bbNum)) + { + fgDfsReversePostorderHelper(finallyPredBlock, visited, preorderIndex, postorderIndex); + } + } + } +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } } - const unsigned lastReachablePostorderNumber = postorderIndex; + // That's everything reachable from the roots. + // + const unsigned highestReachablePostorderNumber = postorderIndex - 1; - // If there are still unvisited blocks (say isolated cycles), visit them too. + // If we still didn't end up visiting everything, visit what remains. // - if (preorderIndex != fgBBcount + 1) + if (highestReachablePostorderNumber != fgBBcount) { - JITDUMP("DFS: flow graph has some isolated cycles, doing extra traversals\n"); + JITDUMP("DFS: there are %u unreachable blocks\n", fgBBcount - highestReachablePostorderNumber); for (BasicBlock* const block : Blocks()) { if (!BlockSetOps::IsMember(this, visited, block->bbNum)) @@ -858,49 +864,11 @@ unsigned Compiler::fgDfsReversePostorderCore(BlockSet_ValArg_T startBlocks) } #endif // DEBUG - return lastReachablePostorderNumber; -} - -//------------------------------------------------------------- -// fgDomFindStartBlocks: Helper for dominance computation to find the start block set. -// -// The start block set represents basic blocks in the flow graph that do not have incoming edges. -// We begin assuming everything is a start block and remove any block that is a successor of another. -// -// Returns: -// Block set describing the start blocks. -// -BlockSet_ValRet_T Compiler::fgDomFindStartBlocks() -{ - BlockSet startNodes(BlockSetOps::MakeFull(this)); - - for (BasicBlock* const block : Blocks()) - { - for (BasicBlock* const succ : block->Succs(this)) - { - BlockSetOps::RemoveElemD(this, startNodes, succ->bbNum); - } - } - -#ifdef DEBUG - if (verbose) - { - printf("\nDominator computation start blocks (those blocks with no incoming edges):\n"); - BlockSetOps::Iter iter(this, startNodes); - unsigned bbNum = 0; - while (iter.NextElem(&bbNum)) - { - printf(FMT_BB " ", bbNum); - } - printf("\n"); - } -#endif // DEBUG - - return startNodes; + return highestReachablePostorderNumber; } //------------------------------------------------------------------------ -// fgDfsReversePostorderHelper: Helper to assign post-order numbers to blocks. +// fgDfsReversePostorderHelper: Helper to assign post-order numbers to blocks // // Arguments: // block - The starting entry block diff --git a/src/coreclr/jit/fgprofilesynthesis.cpp b/src/coreclr/jit/fgprofilesynthesis.cpp index fbea28ed4107c..c9204c1e971e6 100644 --- a/src/coreclr/jit/fgprofilesynthesis.cpp +++ b/src/coreclr/jit/fgprofilesynthesis.cpp @@ -790,7 +790,6 @@ void ProfileSynthesis::RandomizeLikelihoods() void ProfileSynthesis::BuildReversePostorder() { m_comp->EnsureBasicBlockEpoch(); - m_comp->fgComputeEnterBlocksSet(); m_comp->fgDfsReversePostorder(); // Build map from bbNum to Block*. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a4196612a7c98..5505c1ae5ce21 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13869,9 +13869,7 @@ PhaseStatus Compiler::fgMorphBlocks() // We are optimizing. Process in RPO. // fgRenumberBlocks(); - EnsureBasicBlockEpoch(); - fgComputeEnterBlocksSet(); - const unsigned lastReachablePostorderNumber = fgDfsReversePostorderCore(fgEnterBlks); + fgDfsReversePostorder(); // Disallow general creation of new blocks or edges as it // would invalidate RPO. @@ -13896,20 +13894,11 @@ PhaseStatus Compiler::fgMorphBlocks() // Remember this so we can sanity check that no new blocks will get created. // - INDEBUG(unsigned const bbNumMax = fgBBNumMax;); - - // Todo: verify enter block set is sufficient to allow actual removal here. - // Likely need to add genReturnBB, fgEntryBB (for OSR), and perhaps more. - // - if (lastReachablePostorderNumber < fgBBNumMax) - { - JITDUMP("Method has %u unreachable blocks, consider removing them\n", - fgBBNumMax - lastReachablePostorderNumber); - } + unsigned const bbNumMax = fgBBNumMax; // Morph the blocks in RPO. // - for (unsigned i = 1; i <= fgBBNumMax; i++) + for (unsigned i = 1; i <= bbNumMax; i++) { BasicBlock* const block = fgBBReversePostorder[i]; fgMorphBlock(block); From a81c85c8767862c685ab096da18fea0480c5565b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Nov 2023 14:48:12 -0800 Subject: [PATCH 3/5] fix linux build --- 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 db06c562715e1..e1c913aab1c50 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -814,7 +814,7 @@ unsigned Compiler::fgDfsReversePostorder() // if (HBtab->HasFinallyHandler()) { - for (BasicBlock* const finallyPredBlock : handlerBlock->Preds()) + for (BasicBlock* const finallyPredBlock : handlerBlock->PredBlocks()) { assert(finallyPredBlock->KindIs(BBJ_CALLFINALLY)); assert(finallyPredBlock->isBBCallAlwaysPair()); From dd01432354eadcf2dcfbd5e7ba667d442e00513f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 8 Nov 2023 16:12:12 -0800 Subject: [PATCH 4/5] fix arm no retless finally case --- src/coreclr/jit/fgopt.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e1c913aab1c50..c1109c60942b8 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -819,9 +819,11 @@ unsigned Compiler::fgDfsReversePostorder() assert(finallyPredBlock->KindIs(BBJ_CALLFINALLY)); assert(finallyPredBlock->isBBCallAlwaysPair()); - if (!BlockSetOps::IsMember(this, visited, finallyPredBlock->bbNum)) + BasicBlock* const pairTailBlock = finallyPredBlock->Next(); + + if (!BlockSetOps::IsMember(this, visited, pairTailBlock->bbNum)) { - fgDfsReversePostorderHelper(finallyPredBlock, visited, preorderIndex, postorderIndex); + fgDfsReversePostorderHelper(pairTailBlock, visited, preorderIndex, postorderIndex); } } } From 607ed17c467d917bd67863476da5bbda89613a22 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 9 Nov 2023 09:16:21 -0800 Subject: [PATCH 5/5] some finally entries are also loop headers --- src/coreclr/jit/fgopt.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c1109c60942b8..6f04bdbcb4ed6 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -816,9 +816,13 @@ unsigned Compiler::fgDfsReversePostorder() { for (BasicBlock* const finallyPredBlock : handlerBlock->PredBlocks()) { - assert(finallyPredBlock->KindIs(BBJ_CALLFINALLY)); + // In some rare cases the finally is a loop entry. + // + if (!finallyPredBlock->KindIs(BBJ_CALLFINALLY)) + { + continue; + } assert(finallyPredBlock->isBBCallAlwaysPair()); - BasicBlock* const pairTailBlock = finallyPredBlock->Next(); if (!BlockSetOps::IsMember(this, visited, pairTailBlock->bbNum))