JIT: Revise DFS RPO computation

src/coreclr/jit/block.cpp

@@ -1508,6 +1508,9 @@ BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind)
 
     block->bbNatLoopNum = BasicBlock::NOT_IN_LOOP;
 
+    block->bbPreorderNum  = 0;
+    block->bbPostorderNum = 0;
+
     return block;
 }
 

src/coreclr/jit/block.h

@@ -1080,7 +1080,8 @@ struct BasicBlock : private LIR::Range
         void* bbSparseProbeList; // Used early on by fgInstrument
     };
 
-    unsigned bbPostOrderNum; // the block's post order number in the graph.
+    unsigned bbPreorderNum;  // the block's  preorder number in the graph (1...fgMaxBBNum]
+    unsigned bbPostorderNum; // the block's postorder number in the graph (1...fgMaxBBNum]
 
     IL_OFFSET bbCodeOffs;    // IL offset of the beginning of the block
     IL_OFFSET bbCodeOffsEnd; // IL offset past the end of the block. Thus, the [bbCodeOffs..bbCodeOffsEnd)

src/coreclr/jit/compiler.h

@@ -4378,11 +4378,10 @@ class Compiler
     unsigned                     fgBBcountAtCodegen; // # of BBs in the method at the start of codegen
     jitstd::vector<BasicBlock*>* fgBBOrder;          // ordered vector of BBs
 #endif
-    unsigned     fgBBNumMin;       // The min bbNum that has been assigned to basic blocks
-    unsigned     fgBBNumMax;       // The max bbNum that has been assigned to basic blocks
-    unsigned     fgDomBBcount;     // # of BBs for which we have dominator and reachability information
-    BasicBlock** fgBBInvPostOrder; // The flow graph stored in an array sorted in topological order, needed to compute
-                                   // dominance. Indexed by block number. Size: fgBBNumMax + 1.
+    unsigned     fgBBNumMin;           // The min bbNum that has been assigned to basic blocks
+    unsigned     fgBBNumMax;           // The max bbNum that has been assigned to basic blocks
+    unsigned     fgDomBBcount;         // # of BBs for which we have dominator and reachability information
+    BasicBlock** fgBBReversePostorder; // Blocks in reverse postorder
 
     // After the dominance tree is computed, we cache a DFS preorder number and DFS postorder number to compute
     // dominance queries in O(1). fgDomTreePreOrder and fgDomTreePostOrder are arrays giving the block's preorder and
@@ -5193,10 +5192,11 @@ class Compiler
 
     BasicBlock* fgIntersectDom(BasicBlock* a, BasicBlock* b); // Intersect two immediate dominator sets.
 
-    void fgDfsInvPostOrder(); // In order to compute dominance using fgIntersectDom, the flow graph nodes must be
-                              // processed in topological sort, this function takes care of that.
-
-    void fgDfsInvPostOrderHelper(BasicBlock* block, BlockSet& visited, unsigned* count);
+    void fgDfsReversePostorder();
+    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.

src/coreclr/jit/fgdiagnostic.cpp

@@ -1814,7 +1814,7 @@ void Compiler::fgDispDoms()
 
     for (unsigned i = 1; i <= fgBBNumMax; ++i)
     {
-        BasicBlock* current = fgBBInvPostOrder[i];
+        BasicBlock* current = fgBBReversePostorder[i];
         printf(FMT_BB ":  ", current->bbNum);
         while (current != current->bbIDom)
         {

src/coreclr/jit/fgopt.cpp

@@ -745,16 +745,23 @@ bool Compiler::fgRemoveDeadBlocks()
 }
 
 //-------------------------------------------------------------
-// fgDfsInvPostOrder: Helper function for computing dominance information.
+// fgDfsReversePostorder: Depth first search to establish block
+//   preorder and reverse postorder numbers, plus a reverse postorder for blocks.
 //
-// In order to be able to compute dominance, we need to first get a DFS reverse post order sort on the basic flow
-// graph for the dominance algorithm to operate correctly. The reason why we need the DFS sort is because we will
-// build the dominance sets using the partial order induced by the DFS sorting.  With this precondition not
-// holding true, the algorithm doesn't work properly.
+// Notes:
+//   Assumes caller has computed the fgEnterBlkSet.
+//
+//   Assumes caller has allocated the fgBBReversePostorder array.
+//   It will be filled in with blocks in reverse post order.
+//
+//   This algorithm only pays attention to the actual blocks. It ignores any imaginary entry block.
 //
-void Compiler::fgDfsInvPostOrder()
+void Compiler::fgDfsReversePostorder()
 {
-    // NOTE: This algorithm only pays attention to the actual blocks. It ignores the imaginary entry block.
+    // 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);
+    assert(fgBBReversePostorder != nullptr);
 
     // visited   :  Once we run the DFS post order sort recursive algorithm, we mark the nodes we visited to avoid
     //              backtracking.
@@ -765,28 +772,26 @@ void Compiler::fgDfsInvPostOrder()
     // mark in this step.
     BlockSet_ValRet_T startNodes = fgDomFindStartNodes();
 
-    // 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);
-
     BlockSetOps::UnionD(this, startNodes, fgEnterBlks);
     assert(BlockSetOps::IsMember(this, startNodes, fgFirstBB->bbNum));
 
     // Call the flowgraph DFS traversal helper.
-    unsigned postIndex = 1;
+    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) &&
             !BlockSetOps::IsMember(this, visited, block->bbNum))
         {
-            fgDfsInvPostOrderHelper(block, visited, &postIndex);
+            fgDfsReversePostorderHelper(block, visited, preorderIndex, postorderIndex);
         }
     }
 
     // After the DFS reverse postorder is completed, we must have visited all the basic blocks.
-    noway_assert(postIndex == fgBBcount + 1);
+    noway_assert(preorderIndex == fgBBcount + 1);
+    noway_assert(postorderIndex == fgBBcount + 1);
     noway_assert(fgBBNumMax == fgBBcount);
 
 #ifdef DEBUG
@@ -795,7 +800,7 @@ void Compiler::fgDfsInvPostOrder()
         printf("\nAfter doing a post order traversal of the BB graph, this is the ordering:\n");
         for (unsigned i = 1; i <= fgBBNumMax; ++i)
         {
-            printf("%02u -> " FMT_BB "\n", i, fgBBInvPostOrder[i]->bbNum);
+            printf("%02u -> " FMT_BB "\n", i, fgBBReversePostorder[i]->bbNum);
         }
         printf("\n");
     }
@@ -841,18 +846,22 @@ BlockSet_ValRet_T Compiler::fgDomFindStartNodes()
 }
 
 //------------------------------------------------------------------------
-// fgDfsInvPostOrderHelper: Helper to assign post-order numbers to blocks.
+// fgDfsReversevPostorderHelper: Helper to assign post-order numbers to blocks.
 //
 // Arguments:
 //    block   - The starting entry block
 //    visited - The set of visited blocks
-//    count   - Pointer to the Dfs counter
+//    preorderIndex - preorder visit counter
+//    postorderIndex - postorder visit counter
 //
 // Notes:
 //    Compute a non-recursive DFS traversal of the flow graph using an
-//    evaluation stack to assign post-order numbers.
+//    evaluation stack to assign pre and post-order numbers.
 //
-void Compiler::fgDfsInvPostOrderHelper(BasicBlock* block, BlockSet& visited, unsigned* count)
+void Compiler::fgDfsReversePostorderHelper(BasicBlock* block,
+                                           BlockSet&   visited,
+                                           unsigned&   preorderIndex,
+                                           unsigned&   postorderIndex)
 {
     // Assume we haven't visited this node yet (callers ensure this).
     assert(!BlockSetOps::IsMember(this, visited, block->bbNum));
@@ -881,6 +890,8 @@ void Compiler::fgDfsInvPostOrderHelper(BasicBlock* block, BlockSet& visited, uns
             // are guaranteed to only process it after all of its successors
             // pre and post actions are processed.
             stack.Push(DfsBlockEntry(DSS_Post, currentBlock));
+            currentBlock->bbPreorderNum = preorderIndex;
+            preorderIndex++;
 
             for (BasicBlock* const succ : currentBlock->Succs(this))
             {
@@ -903,12 +914,15 @@ void Compiler::fgDfsInvPostOrderHelper(BasicBlock* block, BlockSet& visited, uns
             // actions applied.
 
             assert(current.dfsStackState == DSS_Post);
+            currentBlock->bbPostorderNum = postorderIndex;
 
-            unsigned invCount = fgBBcount - *count + 1;
-            assert(1 <= invCount && invCount <= fgBBNumMax);
-            fgBBInvPostOrder[invCount]   = currentBlock;
-            currentBlock->bbPostOrderNum = invCount;
-            ++(*count);
+            // Compute the index of this in the reverse postorder and
+            // update the reverse postorder accordingly.
+            //
+            assert(postorderIndex <= fgBBcount);
+            unsigned reversePostorderIndex              = fgBBcount - postorderIndex + 1;
+            fgBBReversePostorder[reversePostorderIndex] = currentBlock;
+            postorderIndex++;
         }
     }
 }
@@ -945,10 +959,10 @@ void Compiler::fgComputeDoms()
 
     BlockSet processedBlks(BlockSetOps::MakeEmpty(this));
 
-    fgBBInvPostOrder = new (this, CMK_DominatorMemory) BasicBlock*[fgBBNumMax + 1]{};
+    fgBBReversePostorder = new (this, CMK_DominatorMemory) BasicBlock*[fgBBNumMax + 1]{};
 
-    fgDfsInvPostOrder();
-    noway_assert(fgBBInvPostOrder[0] == nullptr);
+    fgDfsReversePostorder();
+    noway_assert(fgBBReversePostorder[0] == nullptr);
 
     // flRoot and bbRoot represent an imaginary unique entry point in the flow graph.
     // All the orphaned EH blocks and fgFirstBB will temporarily have its predecessors list
@@ -961,12 +975,12 @@ void Compiler::fgComputeDoms()
     bbRoot.bbPreds        = nullptr;
     bbRoot.bbNum          = 0;
     bbRoot.bbIDom         = &bbRoot;
-    bbRoot.bbPostOrderNum = 0;
+    bbRoot.bbPostorderNum = fgBBNumMax + 1;
     bbRoot.bbFlags        = BBF_EMPTY;
 
     FlowEdge flRoot(&bbRoot, nullptr);
 
-    fgBBInvPostOrder[0] = &bbRoot;
+    fgBBReversePostorder[0] = &bbRoot;
 
     // Mark both bbRoot and fgFirstBB processed
     BlockSetOps::AddElemD(this, processedBlks, 0); // bbRoot    == block #0
@@ -1020,7 +1034,7 @@ void Compiler::fgComputeDoms()
         {
             FlowEdge*   first   = nullptr;
             BasicBlock* newidom = nullptr;
-            block               = fgBBInvPostOrder[i];
+            block               = fgBBReversePostorder[i];
 
             // If we have a block that has bbRoot as its bbIDom
             // it means we flag it as processed and as an entry block so
@@ -1279,11 +1293,11 @@ BasicBlock* Compiler::fgIntersectDom(BasicBlock* a, BasicBlock* b)
     BasicBlock* finger2 = b;
     while (finger1 != finger2)
     {
-        while (finger1->bbPostOrderNum > finger2->bbPostOrderNum)
+        while (finger1->bbPostorderNum < finger2->bbPostorderNum)
         {
             finger1 = finger1->bbIDom;
         }
-        while (finger2->bbPostOrderNum > finger1->bbPostOrderNum)
+        while (finger2->bbPostorderNum < finger1->bbPostorderNum)
         {
             finger2 = finger2->bbIDom;
         }
@@ -6596,7 +6610,7 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks()
     {
         for (unsigned i = 1; i <= fgBBNumMax; ++i)
         {
-            BasicBlock* block = fgBBInvPostOrder[i];
+            BasicBlock* block = fgBBReversePostorder[i];
             if (BlockSetOps::IsMember(this, fgEnterBlks, block->bbNum))
             {
                 if (fgFirstBB != block) // skip the normal entry.

src/coreclr/jit/ssabuilder.cpp

@@ -31,15 +31,15 @@ static inline BasicBlock* IntersectDom(BasicBlock* finger1, BasicBlock* finger2)
         {
             return nullptr;
         }
-        while (finger1 != nullptr && finger1->bbPostOrderNum < finger2->bbPostOrderNum)
+        while (finger1 != nullptr && finger1->bbPostorderNum < finger2->bbPostorderNum)
         {
             finger1 = finger1->bbIDom;
         }
         if (finger1 == nullptr)
         {
             return nullptr;
         }
-        while (finger2 != nullptr && finger2->bbPostOrderNum < finger1->bbPostOrderNum)
+        while (finger2 != nullptr && finger2->bbPostorderNum < finger1->bbPostorderNum)
         {
             finger2 = finger2->bbIDom;
         }
@@ -209,7 +209,7 @@ int SsaBuilder::TopologicalSort(BasicBlock** postOrder, int count)
 
             DBG_SSA_JITDUMP("[SsaBuilder::TopologicalSort] postOrder[%d] = " FMT_BB "\n", postIndex, block->bbNum);
             postOrder[postIndex]  = block;
-            block->bbPostOrderNum = postIndex;
+            block->bbPostorderNum = postIndex;
             postIndex += 1;
         }
     }
@@ -1560,7 +1560,7 @@ void SsaBuilder::Build()
     for (BasicBlock* const block : m_pCompiler->Blocks())
     {
         block->bbIDom         = nullptr;
-        block->bbPostOrderNum = 0;
+        block->bbPostorderNum = 0;
     }
 
     // Topologically sort the graph.