Improve DOT flow graph dumping (#52329)

1. Do not include EH and Loop regions in the graph for inlinees. The
data required for them is not valid in the inlinee compiler.
2. Do not include Loop regions in phases starting with the rationalizer.
The loop table is not maintained, and decays, but we don't ever mark it
as invalid. This is an arbitrary point after which it seems to be
unmaintained (and can lead to asserts when using it).
3. Add the text "(inlinee)" to the function name in inlinee graph output,
to distinguish it.
4. Fix a bug where the block map was using incorrect block number count
for inlinees.
5. Fix a region insert bug when inserting a parent region after a child
region where they share the end block (but the parent start block is earlier
than the child). This happens in some EH region tables. Added some comments
about all the different forms of region that need to be handled.
6. Add a `Verify` function to validate the constructed region tree.
7. Stop adding removed loops to the output.

Author: BruceForstall

src/coreclr/jit/fgdiagnostic.cpp

@@ -617,9 +617,12 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
 
 #ifdef DEBUG
     const bool createDotFile = JitConfig.JitDumpFgDot() != 0;
-    const bool includeEH     = JitConfig.JitDumpFgEH() != 0;
-    const bool includeLoops  = JitConfig.JitDumpFgLoops() != 0;
-    const bool constrained   = JitConfig.JitDumpFgConstrained() != 0;
+    const bool includeEH     = (JitConfig.JitDumpFgEH() != 0) && !compIsForInlining();
+    // The loop table is not well maintained after the optimization phases, but there is no single point at which
+    // it is declared invalid. For now, refuse to add loop information starting at the rationalize phase, to
+    // avoid asserts.
+    const bool includeLoops = (JitConfig.JitDumpFgLoops() != 0) && !compIsForInlining() && (phase < PHASE_RATIONALIZE);
+    const bool constrained  = JitConfig.JitDumpFgConstrained() != 0;
 #else  // !DEBUG
     const bool createDotFile = true;
     const bool includeEH     = false;
@@ -633,6 +636,8 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
         return false;
     }
 
+    JITDUMP("Dumping flow graph after phase %s\n", PhaseNames[phase]);
+
     bool        validWeights  = fgHaveValidEdgeWeights;
     double      weightDivisor = (double)BasicBlock::getCalledCount(this);
     const char* escapedString;
@@ -654,7 +659,8 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
     if (createDotFile)
     {
         fprintf(fgxFile, "digraph FlowGraph {\n");
-        fprintf(fgxFile, "    graph [label = \"%s\\nafter\\n%s\"];\n", info.compMethodName, PhaseNames[phase]);
+        fprintf(fgxFile, "    graph [label = \"%s%s\\nafter\\n%s\"];\n", info.compMethodName,
+                compIsForInlining() ? "\\n(inlinee)" : "", PhaseNames[phase]);
         fprintf(fgxFile, "    node [shape = \"Box\"];\n");
     }
     else
@@ -712,12 +718,18 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
     // 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 inlinees, the max block number of the inliner is used, so we need to allocate the block map based on
+    // that size, even though it means allocating a block map possibly much bigger than what's required for just
+    // the inlinee blocks.
+
+    unsigned  blkMapSize   = 1 + (compIsForInlining() ? impInlineInfo->InlinerCompiler->fgBBNumMax : fgBBNumMax);
+    unsigned  blockOrdinal = 1;
+    unsigned* blkMap       = new (this, CMK_DebugOnly) unsigned[blkMapSize];
+    memset(blkMap, 0, sizeof(unsigned) * blkMapSize);
     for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
     {
+        assert(block->bbNum < blkMapSize);
         blkMap[block->bbNum] = blockOrdinal++;
     }
 
@@ -1040,7 +1052,8 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                 };
 
             public:
-                RegionGraph(Compiler* comp, unsigned* blkMap) : m_comp(comp), m_rgnRoot(nullptr), m_blkMap(blkMap)
+                RegionGraph(Compiler* comp, unsigned* blkMap, unsigned blkMapSize)
+                    : m_comp(comp), m_rgnRoot(nullptr), m_blkMap(blkMap), m_blkMapSize(blkMapSize)
                 {
                     // Create a root region that encompasses the whole function.
                     m_rgnRoot =
@@ -1087,6 +1100,24 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                         unsigned childStartOrdinal = m_blkMap[child->m_bbStart->bbNum];
                         unsigned childEndOrdinal   = m_blkMap[child->m_bbEnd->bbNum];
 
+                        // Consider the following cases, where each "x" is a block in the range:
+                        //    xxxxxxx      // current 'child' range; we're comparing against this
+                        //    xxxxxxx      // (1) same range; could be considered child or parent
+                        //  xxxxxxxxx      // (2) parent range, shares last block
+                        //    xxxxxxxxx    // (3) parent range, shares first block
+                        //  xxxxxxxxxxx    // (4) fully overlapping parent range
+                        // xx              // (5) non-overlapping preceding sibling range
+                        //            xx   // (6) non-overlapping following sibling range
+                        //      xxx        // (7) child range
+                        //    xxx          // (8) child range, shares same start block
+                        //    x            // (9) single-block child range, shares same start block
+                        //        xxx      // (10) child range, shares same end block
+                        //          x      // (11) single-block child range, shares same end block
+                        //  xxxxxxx        // illegal: overlapping ranges
+                        //  xxx            // illegal: overlapping ranges (shared child start block and new end block)
+                        //      xxxxxxx    // illegal: overlapping ranges
+                        //          xxx    // illegal: overlapping ranges (shared child end block and new start block)
+
                         // Assert the child is properly nested within the parent.
                         // Note that if regions have the same start and end, you can't tell which is nested within the
                         // other, though it shouldn't matter.
@@ -1095,21 +1126,19 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                         assert(childEndOrdinal <= curEndOrdinal);
 
                         // Should the new region be before this child?
+                        // Case (5).
                         if (newEndOrdinal < childStartOrdinal)
                         {
                             // Insert before this child.
                             newRgn->m_rgnNext = child;
                             *lastChildPtr     = newRgn;
                             break;
                         }
-                        else if (newEndOrdinal <= childEndOrdinal)
+                        else if ((newStartOrdinal >= childStartOrdinal) && (newEndOrdinal <= childEndOrdinal))
                         {
                             // Insert as a child of this child.
-                            // Need to recurse to walk the child's children list to see where
-                            // it belongs.
-
-                            // It better be properly nested.
-                            assert(newStartOrdinal >= childStartOrdinal);
+                            // Need to recurse to walk the child's children list to see where it belongs.
+                            // Case (1), (7), (8), (9), (10), (11).
 
                             curStartOrdinal = m_blkMap[child->m_bbStart->bbNum];
                             curEndOrdinal   = m_blkMap[child->m_bbEnd->bbNum];
@@ -1122,6 +1151,8 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                         else if (newStartOrdinal <= childStartOrdinal)
                         {
                             // The new region is a parent of one or more of the existing children.
+                            // Case (2), (3), (4).
+
                             // Find all the children it encompasses.
                             Region** lastEndChildPtr = &child->m_rgnNext;
                             Region*  endChild        = child->m_rgnNext;
@@ -1154,6 +1185,7 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                         }
 
                         // Else, look for next child.
+                        // Case (6).
 
                         lastChildPtr = &child->m_rgnNext;
                         child        = child->m_rgnNext;
@@ -1216,17 +1248,82 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                 //------------------------------------------------------------------------
                 // Dump: dump the entire region graph
                 //
-                // Arguments:
-                //    stmt  - the statement to dump;
-                //    bbNum - the basic block number to dump.
-                //
                 void Dump()
                 {
                     printf("Region graph:\n");
                     DumpRegionNode(m_rgnRoot, 0);
                     printf("\n");
                 }
 
+                //------------------------------------------------------------------------
+                // VerifyNode: verify the region graph rooted at `rgn`.
+                //
+                // Arguments:
+                //    rgn  - the node (and its children) to check.
+                //
+                void Verify(Region* rgn)
+                {
+                    // The region needs to be a non-overlapping parent to all its children.
+                    // The children need to be non-overlapping, and in increasing order.
+
+                    unsigned rgnStartOrdinal = m_blkMap[rgn->m_bbStart->bbNum];
+                    unsigned rgnEndOrdinal   = m_blkMap[rgn->m_bbEnd->bbNum];
+                    assert(rgnStartOrdinal <= rgnEndOrdinal);
+
+                    Region* child     = rgn->m_rgnChild;
+                    Region* lastChild = nullptr;
+                    if (child != nullptr)
+                    {
+                        unsigned childStartOrdinal = m_blkMap[child->m_bbStart->bbNum];
+                        unsigned childEndOrdinal   = m_blkMap[child->m_bbEnd->bbNum];
+                        assert(childStartOrdinal <= childEndOrdinal);
+                        assert(rgnStartOrdinal <= childStartOrdinal);
+
+                        while (true)
+                        {
+                            Verify(child);
+
+                            lastChild                      = child;
+                            unsigned lastChildStartOrdinal = childStartOrdinal;
+                            unsigned lastChildEndOrdinal   = childEndOrdinal;
+
+                            child = child->m_rgnNext;
+                            if (child == nullptr)
+                            {
+                                break;
+                            }
+
+                            childStartOrdinal = m_blkMap[child->m_bbStart->bbNum];
+                            childEndOrdinal   = m_blkMap[child->m_bbEnd->bbNum];
+                            assert(childStartOrdinal <= childEndOrdinal);
+
+                            // The children can't overlap; they can't share any blocks.
+                            assert(lastChildEndOrdinal < childStartOrdinal);
+                        }
+
+                        // The parent region must fully include the last child.
+                        assert(childEndOrdinal <= rgnEndOrdinal);
+                    }
+                }
+
+                //------------------------------------------------------------------------
+                // Verify: verify the region graph satisfies proper nesting, and other legality rules.
+                //
+                void Verify()
+                {
+                    assert(m_comp != nullptr);
+                    assert(m_blkMap != nullptr);
+                    for (unsigned i = 0; i < m_blkMapSize; i++)
+                    {
+                        assert(m_blkMap[i] < m_blkMapSize);
+                    }
+
+                    // The root region has no siblings.
+                    assert(m_rgnRoot != nullptr);
+                    assert(m_rgnRoot->m_rgnNext == nullptr);
+                    Verify(m_rgnRoot);
+                }
+
 #endif // DEBUG
 
                 //------------------------------------------------------------------------
@@ -1349,11 +1446,12 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                 Compiler* m_comp;
                 Region*   m_rgnRoot;
                 unsigned* m_blkMap;
+                unsigned  m_blkMapSize;
             };
 
             // Define the region graph object. We'll add regions to this, then output the graph.
 
-            RegionGraph rgnGraph(this, blkMap);
+            RegionGraph rgnGraph(this, blkMap, blkMapSize);
 
             // 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
@@ -1405,13 +1503,18 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
                 for (unsigned loopNum = 0; loopNum < optLoopCount; loopNum++)
                 {
                     const LoopDsc& loop = optLoopTable[loopNum];
+                    if (loop.lpFlags & LPFLG_REMOVED)
+                    {
+                        continue;
+                    }
                     sprintf_s(name, sizeof(name), FMT_LP, loopNum);
                     rgnGraph.Insert(name, RegionGraph::RegionType::Loop, loop.lpFirst, loop.lpBottom);
                 }
             }
 
             // All the regions have been added. Now, output them.
             DBEXEC(verbose, rgnGraph.Dump());
+            INDEBUG(rgnGraph.Verify());
             rgnGraph.Output(fgxFile);
         }
     }