Don't recompute preds lists during loop cloning (#51757)

Instead, add and modify the appropriate preds when the mechanical
cloning is performed. This will preserve existing profile data
on the edges.

Contributes to #49030

No x86/x64 SPMI asm diffs.

Author: BruceForstall

src/coreclr/jit/compiler.h

@@ -5268,7 +5268,7 @@ class Compiler
 
     // When the flow graph changes, we need to update the block numbers, predecessor lists, reachability sets, and
     // dominators.
-    void fgUpdateChangedFlowGraph(bool computeDoms = true);
+    void fgUpdateChangedFlowGraph(const bool computePreds = true, const bool computeDoms = true);
 
 public:
     // Compute the predecessors of the blocks in the control flow graph.
@@ -6534,8 +6534,9 @@ class Compiler
     void optUpdateLoopHead(unsigned loopInd, BasicBlock* from, BasicBlock* to);
 
     // Updates the successors of "blk": if "blk2" is a successor of "blk", and there is a mapping for "blk2->blk3" in
-    // "redirectMap", change "blk" so that "blk3" is this successor. Note that the predecessor lists are not updated.
-    void optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap);
+    // "redirectMap", change "blk" so that "blk3" is this successor. If `addPreds` is true, add predecessor edges
+    // for the block based on its new target, whether redirected or not.
+    void optRedirectBlock(BasicBlock* blk, BlockToBlockMap* redirectMap, const bool addPreds = false);
 
     // Marks the containsCall information to "lnum" and any parent loops.
     void AddContainsCallAllContainingLoops(unsigned lnum);

src/coreclr/jit/fgdiagnostic.cpp

@@ -707,11 +707,24 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
         fprintf(fgxFile, ">");
     }
 
+    // In some cases, we want to change the display based on whether an edge is lexically backwards, forwards,
+    // or lexical successor. Also, for the region tree, using the lexical order is useful for determining where
+    // to insert in the tree, to determine nesting. We'd like to use the bbNum to do this. However, we don't
+    // want to renumber the blocks. So, create a mapping of bbNum to ordinal, and compare block order by
+    // comparing the mapped ordinals instead.
+
+    unsigned  blockOrdinal = 0;
+    unsigned* blkMap       = new (this, CMK_DebugOnly) unsigned[fgBBNumMax + 1];
+    memset(blkMap, 0, sizeof(unsigned) * (fgBBNumMax + 1));
+    for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
+    {
+        blkMap[block->bbNum] = blockOrdinal++;
+    }
+
     static const char* kindImage[] = {"EHFINALLYRET", "EHFILTERRET", "EHCATCHRET",  "THROW", "RETURN", "NONE",
                                       "ALWAYS",       "LEAVE",       "CALLFINALLY", "COND",  "SWITCH"};
 
     BasicBlock* block;
-    unsigned    blockOrdinal;
     for (block = fgFirstBB, blockOrdinal = 1; block != nullptr; block = block->bbNext, blockOrdinal++)
     {
         if (createDotFile)
@@ -840,13 +853,13 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
 
                     const char* sep = "";
 
-                    if (bSource->bbNum > bTarget->bbNum)
+                    if (blkMap[bSource->bbNum] > blkMap[bTarget->bbNum])
                     {
                         // Lexical backedge
                         fprintf(fgxFile, " [color=green");
                         sep = ", ";
                     }
-                    else if ((bSource->bbNum + 1) == bTarget->bbNum)
+                    else if ((blkMap[bSource->bbNum] + 1) == blkMap[bTarget->bbNum])
                     {
                         // Lexical successor
                         fprintf(fgxFile, " [color=blue, weight=20");
@@ -953,12 +966,12 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
             {
                 BasicBlock* const bTarget = bSource->GetSucc(i);
                 fprintf(fgxFile, "    " FMT_BB " -> " FMT_BB, bSource->bbNum, bTarget->bbNum);
-                if (bSource->bbNum > bTarget->bbNum)
+                if (blkMap[bSource->bbNum] > blkMap[bTarget->bbNum])
                 {
                     // Lexical backedge
                     fprintf(fgxFile, " [color=green]\n");
                 }
-                else if ((bSource->bbNum + 1) == bTarget->bbNum)
+                else if ((blkMap[bSource->bbNum] + 1) == blkMap[bTarget->bbNum])
                 {
                     // Lexical successor
                     fprintf(fgxFile, " [color=blue]\n");
@@ -1027,24 +1040,11 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                 };
 
             public:
-                RegionGraph(Compiler* comp) : m_comp(comp), m_rgnRoot(nullptr)
+                RegionGraph(Compiler* comp, unsigned* blkMap) : m_comp(comp), m_rgnRoot(nullptr), m_blkMap(blkMap)
                 {
                     // Create a root region that encompasses the whole function.
-                    // We don't want to renumber the blocks, but it's useful to have a sequential number
-                    // representing the lexical block order so we know where to insert a block range
-                    // in the region tree. To do this, create a mapping of bbNum to ordinal, and compare
-                    // block order by comparing the mapped ordinals.
-
                     m_rgnRoot =
                         new (m_comp, CMK_DebugOnly) Region(RegionType::Root, "Root", comp->fgFirstBB, comp->fgLastBB);
-
-                    unsigned bbOrdinal = 0;
-                    m_blkMap           = new (m_comp, CMK_DebugOnly) unsigned[comp->fgBBNumMax + 1];
-                    memset(m_blkMap, 0, sizeof(unsigned) * (comp->fgBBNumMax + 1));
-                    for (BasicBlock* block = comp->fgFirstBB; block != nullptr; block = block->bbNext)
-                    {
-                        m_blkMap[block->bbNum] = bbOrdinal++;
-                    }
                 }
 
                 //------------------------------------------------------------------------
@@ -1353,7 +1353,7 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
 
             // Define the region graph object. We'll add regions to this, then output the graph.
 
-            RegionGraph rgnGraph(this);
+            RegionGraph rgnGraph(this, blkMap);
 
             // Add the EH regions to the region graph. An EH region consists of a region for the
             // `try`, a region for the handler, and, for filter/filter-handlers, a region for the
@@ -2071,7 +2071,7 @@ class BBPredsChecker
     bool CheckEhTryDsc(BasicBlock* block, BasicBlock* blockPred, EHblkDsc* ehTryDsc);
     bool CheckEhHndDsc(BasicBlock* block, BasicBlock* blockPred, EHblkDsc* ehHndlDsc);
     bool CheckJump(BasicBlock* blockPred, BasicBlock* block);
-    bool CheckEHFinalyRet(BasicBlock* blockPred, BasicBlock* block);
+    bool CheckEHFinallyRet(BasicBlock* blockPred, BasicBlock* block);
 
 private:
     Compiler* comp;
@@ -2130,7 +2130,7 @@ unsigned BBPredsChecker::CheckBBPreds(BasicBlock* block, unsigned curTraversalSt
         assert(CheckJump(blockPred, block));
     }
 
-    // Make sure preds are in increasting BBnum order
+    // Make sure preds are in increasing BBnum order
     //
     assert(block->checkPredListOrder());
 
@@ -2232,7 +2232,7 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block)
             return true;
 
         case BBJ_EHFINALLYRET:
-            assert(CheckEHFinalyRet(blockPred, block));
+            assert(CheckEHFinallyRet(blockPred, block));
             return true;
 
         case BBJ_THROW:
@@ -2265,9 +2265,8 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block)
     return false;
 }
 
-bool BBPredsChecker::CheckEHFinalyRet(BasicBlock* blockPred, BasicBlock* block)
+bool BBPredsChecker::CheckEHFinallyRet(BasicBlock* blockPred, BasicBlock* block)
 {
-
     // If the current block is a successor to a BBJ_EHFINALLYRET (return from finally),
     // then the lexically previous block should be a call to the same finally.
     // Verify all of that.

src/coreclr/jit/fgflow.cpp

@@ -686,7 +686,7 @@ void Compiler::fgRemovePreds()
 //
 void Compiler::fgComputePreds()
 {
-    noway_assert(fgFirstBB);
+    noway_assert(fgFirstBB != nullptr);
 
     BasicBlock* block;
 
@@ -697,6 +697,14 @@ void Compiler::fgComputePreds()
         fgDispBasicBlocks();
         printf("\n");
     }
+
+    // Check that the block numbers are increasing order.
+    unsigned lastBBnum = fgFirstBB->bbNum;
+    for (BasicBlock* block = fgFirstBB->bbNext; block != nullptr; block = block->bbNext)
+    {
+        assert(lastBBnum < block->bbNum);
+        lastBBnum = block->bbNum;
+    }
 #endif // DEBUG
 
     // Reset everything pred related

src/coreclr/jit/fgopt.cpp

@@ -174,15 +174,18 @@ bool Compiler::fgReachable(BasicBlock* b1, BasicBlock* b2)
 // Arguments:
 //    computeDoms -- `true` if we should recompute dominators
 //
-void Compiler::fgUpdateChangedFlowGraph(bool computeDoms)
+void Compiler::fgUpdateChangedFlowGraph(const bool computePreds, const bool computeDoms)
 {
     // We need to clear this so we don't hit an assert calling fgRenumberBlocks().
     fgDomsComputed = false;
 
     JITDUMP("\nRenumbering the basic blocks for fgUpdateChangeFlowGraph\n");
     fgRenumberBlocks();
 
-    fgComputePreds();
+    if (computePreds) // This condition is only here until all phases don't require it.
+    {
+        fgComputePreds();
+    }
     fgComputeEnterBlocksSet();
     fgComputeReachabilitySets();
     if (computeDoms)

src/coreclr/jit/flowgraph.cpp

@@ -190,8 +190,9 @@ PhaseStatus Compiler::fgInsertGCPolls()
     {
         noway_assert(opts.OptimizationEnabled());
         fgReorderBlocks();
-        constexpr bool computeDoms = false;
-        fgUpdateChangedFlowGraph(computeDoms);
+        constexpr bool computePreds = true;
+        constexpr bool computeDoms  = false;
+        fgUpdateChangedFlowGraph(computePreds, computeDoms);
     }
 #ifdef DEBUG
     if (verbose)

src/coreclr/jit/jithashtable.h

@@ -471,7 +471,7 @@ class JitHashTable
 
 public:
     //------------------------------------------------------------------------
-    // CheckGrowth: Replace the bucket table with a larger one and copy all nodes
+    // Reallocate: Replace the bucket table with a larger one and copy all nodes
     // from the existing bucket table.
     //
     // Notes: