From 2df0cc4d2e87efb444271cd31061ccef308167cf Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Dec 2023 23:17:00 +0100 Subject: [PATCH 1/3] JIT: Generalize check for cycles when finding loops optFindNewLoops tries to leave a breadcrumb for loop alignment about whether it needs to bother with identifying loops. It does so by checking for whether any natural loops were found, but also whether any irreducible loops were found (i.e. any cycle). The idea is that optimizations that run between finding loops and loop alignment can easily remove edges that make irreducible loops into natural loops. However, the check for "any cycle" was not precise enough; it made the assumption that if we find no improper loop headers, this means there is no cycle. But that is not true, specifically because we only consider blocks to be loop headers when their backedges are non-exceptional. Odd loop structures can exist that iterate through required exception throws, like in the case of #96145. In that case we have a cycle through the handler, which ends up being a natural loop after some optimizations run. To fix the situation we generalize `fgComputeDfs` to identify when the flow graph has a cycle, which is the fully general property we want for the breadcrumb. As a bonus we can skip identifying loops if we know the flow graph has no cycles. Fix #96145 --- src/coreclr/jit/compiler.h | 22 +++++++-------- src/coreclr/jit/compiler.hpp | 9 +++++-- src/coreclr/jit/fgdiagnostic.cpp | 3 ++- src/coreclr/jit/flowgraph.cpp | 46 +++++++++++++++++++++++--------- src/coreclr/jit/optimizer.cpp | 12 ++++++--- 5 files changed, 62 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 82a714b303da95..05a5711de5c4cf 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1969,12 +1969,14 @@ class FlowGraphDfsTree // that all predecessors are visited before successors whenever possible. BasicBlock** m_postOrder; unsigned m_postOrderCount; + bool m_hasCycle; public: - FlowGraphDfsTree(Compiler* comp, BasicBlock** postOrder, unsigned postOrderCount) + FlowGraphDfsTree(Compiler* comp, BasicBlock** postOrder, unsigned postOrderCount, bool hasCycle) : m_comp(comp) , m_postOrder(postOrder) , m_postOrderCount(postOrderCount) + , m_hasCycle(hasCycle) { } @@ -2004,6 +2006,11 @@ class FlowGraphDfsTree return BitVecTraits(m_postOrderCount, m_comp); } + bool HasCycle() const + { + return m_hasCycle; + } + bool Contains(BasicBlock* block) const; bool IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) const; }; @@ -2246,10 +2253,6 @@ class FlowGraphNaturalLoops // Collection of loops that were found. jitstd::vector m_loops; - // Whether or not we saw any non-natural loop cycles, also known as - // irreducible loops. - unsigned m_improperLoopHeaders = 0; - FlowGraphNaturalLoops(const FlowGraphDfsTree* dfs); static bool FindNaturalLoopBlocks(FlowGraphNaturalLoop* loop, ArrayStack& worklist); @@ -2264,11 +2267,6 @@ class FlowGraphNaturalLoops return m_loops.size(); } - bool HaveNonNaturalLoopCycles() - { - return m_improperLoopHeaders > 0; - } - FlowGraphNaturalLoop* GetLoopByIndex(unsigned index); FlowGraphNaturalLoop* GetLoopByHeader(BasicBlock* header); @@ -6076,8 +6074,8 @@ class Compiler PhaseStatus fgSetBlockOrder(); bool fgHasCycleWithoutGCSafePoint(); - template - unsigned fgRunDfs(TFuncAssignPreorder assignPreorder, TFuncAssignPostorder assignPostorder); + template + unsigned fgRunDfs(VisitPreorder assignPreorder, VisitPostorder assignPostorder, VisitEdge visitEdge); FlowGraphDfsTree* fgComputeDfs(); void fgInvalidateDfsTree(); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 68b1894fe0537e..4acf9046caaa9e 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4913,16 +4913,19 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) // 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*. // // Parameters: // visitPreorder - Functor to visit block in its preorder // visitPostorder - Functor to visit block in its postorder +// visitEdge - Functor to visit an edge. Called after visitPreorder (if +// this is the first time the successor is seen). // // Returns: // Number of blocks visited. // -template -unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder) +template +unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder, VisitEdge visitEdge) { BitVecTraits traits(fgBBNumMax + 1, this); BitVec visited(BitVecOps::MakeEmpty(&traits)); @@ -4950,6 +4953,8 @@ unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPos blocks.Emplace(this, succ); visitPreorder(succ, preOrderIndex++); } + + visitEdge(block, succ); } else { diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 9f9e5bb4785e24..f8fc9d506e680e 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -5210,7 +5210,8 @@ void Compiler::fgDebugCheckDfsTree() [=](BasicBlock* block, unsigned postorderNum) { assert(block->bbNewPostorderNum == postorderNum); assert(m_dfsTree->GetPostOrder(postorderNum) == block); - }); + }, + [](BasicBlock* block, BasicBlock* succ) {}); assert(m_dfsTree->GetPostOrderCount() == count); } diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index aebae003262303..4e07976631e943 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4079,15 +4079,30 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant) FlowGraphDfsTree* Compiler::fgComputeDfs() { BasicBlock** postOrder = new (this, CMK_DepthFirstSearch) BasicBlock*[fgBBcount]; + bool hasCycle = false; - unsigned numBlocks = fgRunDfs([](BasicBlock* block, unsigned preorderNum) { block->bbPreorderNum = preorderNum; }, - [=](BasicBlock* block, unsigned postorderNum) { - block->bbNewPostorderNum = postorderNum; - assert(postorderNum < fgBBcount); - postOrder[postorderNum] = block; - }); + auto visitPreorder = [](BasicBlock* block, unsigned preorderNum) { + block->bbPreorderNum = preorderNum; + block->bbNewPostorderNum = UINT_MAX; + }; + + auto visitPostorder = [=](BasicBlock* block, unsigned postorderNum) { + block->bbNewPostorderNum = postorderNum; + assert(postorderNum < fgBBcount); + postOrder[postorderNum] = block; + }; - return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks); + auto visitEdge = [&hasCycle](BasicBlock* block, BasicBlock* succ) { + // Check if block -> succ is a backedge, in which case the flow + // graph has a cycle. + if ((succ->bbPreorderNum <= block->bbPreorderNum) && (succ->bbNewPostorderNum == UINT_MAX)) + { + hasCycle = true; + } + }; + + unsigned numBlocks = fgRunDfs(visitPreorder, visitPostorder, visitEdge); + return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks, hasCycle); } //------------------------------------------------------------------------ @@ -4388,13 +4403,20 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr { unsigned rpoNum = dfsTree->GetPostOrderCount() - i; BasicBlock* const block = dfsTree->GetPostOrder(i - 1); - JITDUMP("%02u -> " FMT_BB "[%u, %u]\n", rpoNum + 1, block->bbNum, block->bbPreorderNum + 1, - block->bbNewPostorderNum + 1); + JITDUMP("%02u -> " FMT_BB "[%u, %u]\n", rpoNum, block->bbNum, block->bbPreorderNum, block->bbNewPostorderNum); } + + unsigned improperLoopHeaders = 0; #endif FlowGraphNaturalLoops* loops = new (comp, CMK_Loops) FlowGraphNaturalLoops(dfsTree); + if (!dfsTree->HasCycle()) + { + JITDUMP("DFS tree has no cycles; skipping identification of natural loops\n"); + return loops; + } + ArrayStack worklist(comp->getAllocator(CMK_Loops)); for (unsigned i = dfsTree->GetPostOrderCount(); i != 0; i--) @@ -4439,7 +4461,7 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr if (!FindNaturalLoopBlocks(loop, worklist)) { - loops->m_improperLoopHeaders++; + INDEBUG(improperLoopHeaders++); continue; } @@ -4545,9 +4567,9 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr JITDUMP("\nFound %zu loops\n", loops->m_loops.size()); } - if (loops->m_improperLoopHeaders > 0) + if (improperLoopHeaders > 0) { - JITDUMP("Rejected %u loop headers\n", loops->m_improperLoopHeaders); + JITDUMP("Rejected %u loop headers\n", improperLoopHeaders); } #endif diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 30218fd0da0cda..46f926a93ec54f 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5498,9 +5498,15 @@ void Compiler::optFindNewLoops() m_newToOldLoop = m_loops->NumLoops() == 0 ? nullptr : (new (this, CMK_Loops) LoopDsc*[m_loops->NumLoops()]{}); m_oldToNewLoop = new (this, CMK_Loops) FlowGraphNaturalLoop*[BasicBlock::MAX_LOOP_NUM]{}; - // Unnatural loops can quickly become natural if we manage to remove some - // edges, so be conservative here. - fgMightHaveNaturalLoops = (m_loops->NumLoops() > 0) || m_loops->HaveNonNaturalLoopCycles(); + // Leave a bread crumb for future phases like loop alignment about whether + // looking for loops makes sense. We generally do not expect phases to + // introduce new cycles/loops in the flow graph; if they do, they should + // set this to true themselves. + // We use more general cycles over "m_loops->NumLoops() > 0" here because + // future optimizations can easily cause general cycles to become natural + // loops by removing edges. + fgMightHaveNaturalLoops = m_dfsTree->HasCycle(); + assert(fgMightHaveNaturalLoops || (m_loops->NumLoops() == 0)); for (BasicBlock* block : Blocks()) { From 568af94732d2959d2bdbbcd86bfd65b05867dcc5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Dec 2023 23:41:07 +0100 Subject: [PATCH 2/3] Add comment --- src/coreclr/jit/compiler.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 05a5711de5c4cf..ad5d5e3e10751d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1969,6 +1969,8 @@ class FlowGraphDfsTree // that all predecessors are visited before successors whenever possible. BasicBlock** m_postOrder; unsigned m_postOrderCount; + + // Whether the DFS that produced the tree found any backedges. bool m_hasCycle; public: From 78a17a527d7c24d488b902a17441a927f5c56f29 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Dec 2023 23:44:54 +0100 Subject: [PATCH 3/3] Nit --- src/coreclr/jit/flowgraph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 4e07976631e943..912f0b91cf22ad 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4413,7 +4413,7 @@ FlowGraphNaturalLoops* FlowGraphNaturalLoops::Find(const FlowGraphDfsTree* dfsTr if (!dfsTree->HasCycle()) { - JITDUMP("DFS tree has no cycles; skipping identification of natural loops\n"); + JITDUMP("Flow graph has no cycles; skipping identification of natural loops\n"); return loops; }