JIT: Add post-phase validation of DFS tree

src/coreclr/jit/compiler.cpp

@@ -4946,6 +4946,12 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
                 //
                 DoPhase(this, PHASE_OPTIMIZE_BRANCHES, &Compiler::optRedundantBranches);
             }
+            else
+            {
+                // DFS tree is always invalid after this point.
+                //
+                fgInvalidateDfsTree();
+            }
 
             if (doCse)
             {
@@ -4954,17 +4960,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
                 DoPhase(this, PHASE_OPTIMIZE_VALNUM_CSES, &Compiler::optOptimizeCSEs);
             }
 
-            // Assertion prop can do arbitrary statement remorphing, which
-            // can clone code and disrupt our simpleminded SSA accounting.
-            //
-            // So, disable the ssa checks.
-            //
-            if (fgSsaValid)
-            {
-                JITDUMP("Marking SSA as invalid before assertion prop\n");
-                fgSsaValid = false;
-            }
-
             if (doAssertionProp)
             {
                 // Assertion propagation

src/coreclr/jit/compiler.h

@@ -6049,7 +6049,11 @@ class Compiler
     PhaseStatus fgSetBlockOrder();
     bool fgHasCycleWithoutGCSafePoint();
 
+    template<typename TFuncAssignPreorder, typename TFuncAssignPostorder>
+    unsigned fgRunDfs(TFuncAssignPreorder assignPreorder, TFuncAssignPostorder assignPostorder);
+
     FlowGraphDfsTree* fgComputeDfs();
+    void fgInvalidateDfsTree();
 
     void fgRemoveReturnBlock(BasicBlock* block);
 
@@ -6115,6 +6119,8 @@ class Compiler
     bool fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks checks);
     bool fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks checks);
 
+    void fgDebugCheckDfsTree();
+
 #endif // DEBUG
 
     static bool fgProfileWeightsEqual(weight_t weight1, weight_t weight2, weight_t epsilon = 0.01);

src/coreclr/jit/compiler.hpp

@@ -4915,6 +4915,82 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason)
     return whyNot == nullptr;
 }
 
+//------------------------------------------------------------------------
+// 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
+//
+// Parameters:
+//   visitPreorder  - Functor to visit block in its preorder
+//   visitPostorder - Functor to visit block in its postorder
+//
+// Returns:
+//   Number of blocks visited.
+//
+template <typename VisitPreorder, typename VisitPostorder>
+unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPostorder)
+{
+    BitVecTraits traits(fgBBNumMax + 1, this);
+    BitVec       visited(BitVecOps::MakeEmpty(&traits));
+
+    unsigned preOrderIndex  = 0;
+    unsigned postOrderIndex = 0;
+
+    ArrayStack<AllSuccessorEnumerator> blocks(getAllocator(CMK_DepthFirstSearch));
+
+    auto dfsFrom = [&](BasicBlock* firstBB) {
+
+        BitVecOps::AddElemD(&traits, visited, firstBB->bbNum);
+        blocks.Emplace(this, firstBB);
+        visitPreorder(firstBB, preOrderIndex++);
+
+        while (!blocks.Empty())
+        {
+            BasicBlock* block = blocks.TopRef().Block();
+            BasicBlock* succ  = blocks.TopRef().NextSuccessor();
+
+            if (succ != nullptr)
+            {
+                if (BitVecOps::TryAddElemD(&traits, visited, succ->bbNum))
+                {
+                    blocks.Emplace(this, succ);
+                    visitPreorder(succ, preOrderIndex++);
+                }
+            }
+            else
+            {
+                blocks.Pop();
+                visitPostorder(block, postOrderIndex++);
+            }
+        }
+
+    };
+
+    dfsFrom(fgFirstBB);
+
+    if ((fgEntryBB != nullptr) && !BitVecOps::IsMember(&traits, visited, fgEntryBB->bbNum))
+    {
+        // OSR methods will early on create flow that looks like it goes to the
+        // patchpoint, but during morph we may transform to something that
+        // requires the original entry (fgEntryBB).
+        assert(opts.IsOSR());
+        assert((fgEntryBB->bbRefs == 1) && (fgEntryBB->bbPreds == nullptr));
+        dfsFrom(fgEntryBB);
+    }
+
+    if ((genReturnBB != nullptr) && !BitVecOps::IsMember(&traits, visited, genReturnBB->bbNum) && !fgGlobalMorphDone)
+    {
+        // We introduce the merged return BB before morph and will redirect
+        // other returns to it as part of morph; keep it reachable.
+        dfsFrom(genReturnBB);
+    }
+
+    assert(preOrderIndex == postOrderIndex);
+    return preOrderIndex;
+}
+
 //------------------------------------------------------------------------------
 // FlowGraphNaturalLoop::VisitLoopBlocksReversePostOrder: Visit all of the
 // loop's blocks in reverse post order.

src/coreclr/jit/fgdiagnostic.cpp

@@ -4664,43 +4664,17 @@ void Compiler::fgDebugCheckSsa()
     assert(fgSsaPassesCompleted > 0);
     assert(fgDomsComputed);
 
-    // This class visits the flow graph the same way the SSA builder does.
-    // In particular it may skip over blocks that SSA did not rename.
-    //
-    class SsaCheckDomTreeVisitor : public NewDomTreeVisitor<SsaCheckDomTreeVisitor>
-    {
-        SsaCheckVisitor& m_checkVisitor;
-
-    public:
-        SsaCheckDomTreeVisitor(Compiler* compiler, SsaCheckVisitor& checkVisitor)
-            : NewDomTreeVisitor(compiler), m_checkVisitor(checkVisitor)
-        {
-        }
-
-        void PreOrderVisit(BasicBlock* block)
-        {
-            m_checkVisitor.SetBlock(block);
-
-            for (Statement* const stmt : block->Statements())
-            {
-                m_checkVisitor.WalkTree(stmt->GetRootNodePointer(), nullptr);
-            }
-        }
-    };
-
     // Visit the blocks that SSA initially renamed
     //
-    SsaCheckVisitor        scv(this);
-    SsaCheckDomTreeVisitor visitor(this, scv);
-    visitor.WalkTree(fgSsaDomTree);
-
-    // Also visit any blocks added after SSA was built
-    //
-    for (BasicBlock* const block : Blocks())
+    SsaCheckVisitor scv(this);
+    for (unsigned i = 0; i < m_dfsTree->GetPostOrderCount(); i++)
     {
-        if (block->bbNum > fgDomBBcount)
+        BasicBlock* block = m_dfsTree->GetPostOrder()[i];
+        scv.SetBlock(block);
+
+        for (Statement* const stmt : block->Statements())
         {
-            visitor.PreOrderVisit(block);
+            scv.WalkTree(stmt->GetRootNodePointer(), nullptr);
         }
     }
 
@@ -5136,5 +5110,20 @@ void Compiler::fgDebugCheckLoopTable()
     assert(preHeaderCount == 0);
 }
 
+//------------------------------------------------------------------------------
+// fgDebugCheckDfsTree: Checks that the DFS tree matches the current flow graph.
+//
+void Compiler::fgDebugCheckDfsTree()
+{
+    unsigned count =
+        fgRunDfs([](BasicBlock* block, unsigned preorderNum) { assert(block->bbPreorderNum == preorderNum); },
+                 [=](BasicBlock* block, unsigned postorderNum) {
+                     assert(block->bbNewPostorderNum == postorderNum);
+                     assert(m_dfsTree->GetPostOrder()[postorderNum] == block);
+                 });
+
+    assert(m_dfsTree->GetPostOrderCount() == count);
+}
+
 /*****************************************************************************/
 #endif // DEBUG

src/coreclr/jit/fgopt.cpp

@@ -6438,7 +6438,32 @@ PhaseStatus Compiler::fgDfsBlocksAndRemove()
         }
 #endif
 
-        fgRemoveUnreachableBlocks([=](BasicBlock* block) { return !m_dfsTree->Contains(block); });
+        // The DFS we run is not precise around call-finally, so
+        // `fgRemoveUnreachableBlocks` can expose newly unreachable blocks
+        // that we did not uncover during the DFS. If we did remove any
+        // call-finally blocks then iterate to closure. This is a very rare
+        // case.
+        while (true)
+        {
+            bool anyCallFinallyPairs = false;
+            fgRemoveUnreachableBlocks([=, &anyCallFinallyPairs](BasicBlock* block) {
+                if (!m_dfsTree->Contains(block))
+                {
+                    anyCallFinallyPairs |= block->isBBCallAlwaysPair();
+                    return true;
+                }
+
+                return false;
+            });
+
+            if (!anyCallFinallyPairs)
+            {
+                break;
+            }
+
+            m_dfsTree = fgComputeDfs();
+        }
+
         status = PhaseStatus::MODIFIED_EVERYTHING;
     }
 

src/coreclr/jit/flowgraph.cpp

@@ -4053,64 +4053,29 @@ bool FlowGraphDfsTree::IsAncestor(BasicBlock* ancestor, BasicBlock* descendant)
 FlowGraphDfsTree* Compiler::fgComputeDfs()
 {
     BasicBlock** postOrder = new (this, CMK_DepthFirstSearch) BasicBlock*[fgBBcount];
-    BitVecTraits traits(fgBBNumMax + 1, this);
 
-    BitVec visited(BitVecOps::MakeEmpty(&traits));
+    unsigned numBlocks = fgRunDfs([](BasicBlock* block, unsigned preorderNum) { block->bbPreorderNum = preorderNum; },
+                                  [=](BasicBlock* block, unsigned postorderNum) {
+                                      block->bbNewPostorderNum = postorderNum;
+                                      assert(postorderNum < fgBBcount);
+                                      postOrder[postorderNum] = block;
+                                  });
 
-    unsigned preOrderIndex  = 0;
-    unsigned postOrderIndex = 0;
-
-    ArrayStack<AllSuccessorEnumerator> blocks(getAllocator(CMK_DepthFirstSearch));
-
-    auto dfsFrom = [&, postOrder](BasicBlock* firstBB) {
-
-        BitVecOps::AddElemD(&traits, visited, firstBB->bbNum);
-        blocks.Emplace(this, firstBB);
-        firstBB->bbPreorderNum = preOrderIndex++;
-
-        while (!blocks.Empty())
-        {
-            BasicBlock* block = blocks.TopRef().Block();
-            BasicBlock* succ  = blocks.TopRef().NextSuccessor();
-
-            if (succ != nullptr)
-            {
-                if (BitVecOps::TryAddElemD(&traits, visited, succ->bbNum))
-                {
-                    blocks.Emplace(this, succ);
-                    succ->bbPreorderNum = preOrderIndex++;
-                }
-            }
-            else
-            {
-                blocks.Pop();
-                postOrder[postOrderIndex] = block;
-                block->bbNewPostorderNum  = postOrderIndex++;
-            }
-        }
-
-    };
-
-    dfsFrom(fgFirstBB);
-
-    if ((fgEntryBB != nullptr) && !BitVecOps::IsMember(&traits, visited, fgEntryBB->bbNum))
-    {
-        // OSR methods will early on create flow that looks like it goes to the
-        // patchpoint, but during morph we may transform to something that
-        // requires the original entry (fgEntryBB).
-        assert(opts.IsOSR());
-        assert((fgEntryBB->bbRefs == 1) && (fgEntryBB->bbPreds == nullptr));
-        dfsFrom(fgEntryBB);
-    }
-
-    if ((genReturnBB != nullptr) && !BitVecOps::IsMember(&traits, visited, genReturnBB->bbNum) && !fgGlobalMorphDone)
-    {
-        // We introduce the merged return BB before morph and will redirect
-        // other returns to it as part of morph; keep it reachable.
-        dfsFrom(genReturnBB);
-    }
+    return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, numBlocks);
+}
 
-    return new (this, CMK_DepthFirstSearch) FlowGraphDfsTree(this, postOrder, postOrderIndex);
+//------------------------------------------------------------------------
+// fgInvalidateDfsTree: Invalidate computed DFS tree and dependent annotations
+// (like loops, dominators and SSA).
+//
+void Compiler::fgInvalidateDfsTree()
+{
+    m_dfsTree      = nullptr;
+    m_loops        = nullptr;
+    fgSsaDomTree   = nullptr;
+    m_newToOldLoop = nullptr;
+    m_oldToNewLoop = nullptr;
+    fgSsaValid     = false;
 }
 
 //------------------------------------------------------------------------
@@ -5425,6 +5390,7 @@ FlowGraphDominatorTree* FlowGraphDominatorTree::Build(const FlowGraphDfsTree* df
     {
         BasicBlock* block  = postOrder[i];
         BasicBlock* parent = block->bbIDom;
+        assert(parent != nullptr);
         assert(dfsTree->Contains(block) && dfsTree->Contains(parent));
 
         domTree[i].nextSibling                        = domTree[parent->bbNewPostorderNum].firstChild;

src/coreclr/jit/morph.cpp

@@ -14102,10 +14102,10 @@ PhaseStatus Compiler::fgMorphBlocks()
 
     // We are done with the global morphing phase
     //
+    fgInvalidateDfsTree();
     fgGlobalMorph     = false;
     fgGlobalMorphDone = true;
     compCurBB         = nullptr;
-    m_dfsTree         = nullptr;
 
 #ifdef DEBUG
     if (optLocalAssertionProp)

src/coreclr/jit/phase.cpp

@@ -170,6 +170,11 @@ void Phase::PostPhase(PhaseStatus status)
         {
             comp->fgDebugCheckLinkedLocals();
         }
+
+        if (comp->m_dfsTree != nullptr)
+        {
+            comp->fgDebugCheckDfsTree();
+        }
     }
 #endif // DEBUG
 }

src/coreclr/jit/redundantbranchopts.cpp

@@ -104,6 +104,9 @@ PhaseStatus Compiler::optRedundantBranches()
     }
 #endif // DEBUG
 
+    // DFS tree is always considered invalid after RBO.
+    fgInvalidateDfsTree();
+
     return visitor.madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
 }