Improve flowgraph xml/dot dumping (#51082)

* Improve flowgraph xml/dot dumping

1. Decouple it from JitDump
2. Add some documentation, including a useful link to https://sketchviz.com/
3. Fix DUMP_FLOWGRAPHS Release build
4. Fix some xml dumping issues
5. Replace custom function pattern parsing with CONFIG_METHODSET ".contains" function
6. Make dot format the default (instead of xml)

Fixes #43712

* Code review feedback: use `getCalledCount`

src/coreclr/jit/block.h

@@ -537,7 +537,7 @@ struct BasicBlock : private LIR::Range
     weight_t bbWeight; // The dynamic execution weight of this block
 
     // getCalledCount -- get the value used to normalize weights for this method
-    weight_t getCalledCount(Compiler* comp);
+    static weight_t getCalledCount(Compiler* comp);
 
     // getBBWeight -- get the normalized weight of this block
     weight_t getBBWeight(Compiler* comp);

src/coreclr/jit/compiler.cpp

@@ -1830,7 +1830,7 @@ void Compiler::compInit(ArenaAllocator*       pAlloc,
     bRangeAllowStress = false;
 #endif
 
-#if defined(DEBUG) || defined(LATE_DISASM)
+#if defined(DEBUG) || defined(LATE_DISASM) || DUMP_FLOWGRAPHS
     // Initialize the method name and related info, as it is used early in determining whether to
     // apply stress modes, and which ones to apply.
     // Note that even allocating memory can invoke the stress mechanism, so ensure that both
@@ -1852,7 +1852,7 @@ void Compiler::compInit(ArenaAllocator*       pAlloc,
 
     info.compFullName  = eeGetMethodFullName(methodHnd);
     info.compPerfScore = 0.0;
-#endif // defined(DEBUG) || defined(LATE_DISASM)
+#endif // defined(DEBUG) || defined(LATE_DISASM) || DUMP_FLOWGRAPHS
 
 #if defined(DEBUG) || defined(INLINE_DATA)
     info.compMethodHashPrivate = 0;

src/coreclr/jit/compiler.h

@@ -5506,7 +5506,6 @@ public:
     const char* fgProcessEscapes(const char* nameIn, escapeMapping_t* map);
     FILE* fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, LPCWSTR type);
     bool fgDumpFlowGraph(Phases phase);
-
 #endif // DUMP_FLOWGRAPHS
 
 #ifdef DEBUG
@@ -9445,12 +9444,12 @@ public:
         BOOL hasCircularClassConstraints;
         BOOL hasCircularMethodConstraints;
 
-#if defined(DEBUG) || defined(LATE_DISASM)
+#if defined(DEBUG) || defined(LATE_DISASM) || DUMP_FLOWGRAPHS
         const char* compMethodName;
         const char* compClassName;
         const char* compFullName;
         double      compPerfScore;
-#endif // defined(DEBUG) || defined(LATE_DISASM)
+#endif // defined(DEBUG) || defined(LATE_DISASM) || DUMP_FLOWGRAPHS
 
 #if defined(DEBUG) || defined(INLINE_DATA)
         // Method hash is logcally const, but computed

src/coreclr/jit/fgdiagnostic.cpp

@@ -342,6 +342,20 @@ static void fprintfDouble(FILE* fgxFile, double value)
 //    phase       - A phase identifier to indicate which phase is associated with the dump
 //    type        - A (wide) string indicating the type of dump, "dot" or "xml"
 //
+// Notes:
+// The filename to use to write the data comes from the COMPlus_JitDumpFgFile or COMPlus_NgenDumpFgFile
+// configuration. If unset, use "default". The "type" argument is used as a filename extension,
+// e.g., "default.dot".
+//
+// There are several "special" filenames recognized:
+// "profiled" -- only create graphs for methods with profile info, one file per method.
+// "hot" -- only create graphs for the hot region, one file per method.
+// "cold" -- only create graphs for the cold region, one file per method.
+// "jit" -- only create graphs for JITing, one file per method.
+// "all" -- create graphs for all regions, one file per method.
+// "stdout" -- output to stdout, not a file.
+// "stderr" -- output to stderr, not a file.
+//
 // Return Value:
 //    Opens a file to which a flowgraph can be dumped, whose name is based on the current
 //    config vales.
@@ -349,44 +363,43 @@ static void fprintfDouble(FILE* fgxFile, double value)
 FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, LPCWSTR type)
 {
     FILE*       fgxFile;
-    LPCWSTR     pattern  = nullptr;
-    LPCWSTR     filename = nullptr;
-    LPCWSTR     pathname = nullptr;
+    LPCWSTR     phasePattern = W("*"); // default (used in Release) is dump all phases
+    bool        dumpFunction = true;   // default (used in Release) is always dump
+    LPCWSTR     filename     = nullptr;
+    LPCWSTR     pathname     = nullptr;
     const char* escapedString;
     bool        createDuplicateFgxFiles = true;
 
+    if (fgBBcount <= 1)
+    {
+        return nullptr;
+    }
+
 #ifdef DEBUG
     if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT))
     {
-        pattern  = JitConfig.NgenDumpFg();
+        dumpFunction =
+            JitConfig.NgenDumpFg().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args);
         filename = JitConfig.NgenDumpFgFile();
         pathname = JitConfig.NgenDumpFgDir();
     }
     else
     {
-        pattern  = JitConfig.JitDumpFg();
+        dumpFunction =
+            JitConfig.JitDumpFg().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args);
         filename = JitConfig.JitDumpFgFile();
         pathname = JitConfig.JitDumpFgDir();
     }
-#endif // DEBUG
 
-    if (fgBBcount <= 1)
-    {
-        return nullptr;
-    }
-
-    if (pattern == nullptr)
-    {
-        return nullptr;
-    }
+    phasePattern = JitConfig.JitDumpFgPhase();
+#endif // DEBUG
 
-    if (wcslen(pattern) == 0)
+    if (!dumpFunction)
     {
         return nullptr;
     }
 
-    LPCWSTR phasePattern = JitConfig.JitDumpFgPhase();
-    LPCWSTR phaseName    = PhaseShortNames[phase];
+    LPCWSTR phaseName = PhaseShortNames[phase];
     if (phasePattern == nullptr)
     {
         if (phase != PHASE_DETERMINE_FIRST_COLD_BLOCK)
@@ -402,84 +415,6 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, LPCWSTR typ
         }
     }
 
-    if (*pattern != W('*'))
-    {
-        bool hasColon = (wcschr(pattern, W(':')) != nullptr);
-
-        if (hasColon)
-        {
-            const char* className = info.compClassName;
-            if (*pattern == W('*'))
-            {
-                pattern++;
-            }
-            else
-            {
-                while ((*pattern != W(':')) && (*pattern != W('*')))
-                {
-                    if (*pattern != *className)
-                    {
-                        return nullptr;
-                    }
-
-                    pattern++;
-                    className++;
-                }
-                if (*pattern == W('*'))
-                {
-                    pattern++;
-                }
-                else
-                {
-                    if (*className != 0)
-                    {
-                        return nullptr;
-                    }
-                }
-            }
-            if (*pattern != W(':'))
-            {
-                return nullptr;
-            }
-
-            pattern++;
-        }
-
-        const char* methodName = info.compMethodName;
-        if (*pattern == W('*'))
-        {
-            pattern++;
-        }
-        else
-        {
-            while ((*pattern != 0) && (*pattern != W('*')))
-            {
-                if (*pattern != *methodName)
-                {
-                    return nullptr;
-                }
-
-                pattern++;
-                methodName++;
-            }
-            if (*pattern == W('*'))
-            {
-                pattern++;
-            }
-            else
-            {
-                if (*methodName != 0)
-                {
-                    return nullptr;
-                }
-            }
-        }
-        if (*pattern != 0)
-        {
-            return nullptr;
-        }
-    }
-
     if (filename == nullptr)
     {
         filename = W("default");
@@ -645,13 +580,15 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, LPCWSTR typ
 //    The xml dumps are the historical mechanism for dumping the flowgraph.
 //    The dot format can be viewed by:
 //    - Graphviz (http://www.graphviz.org/)
-//      - The command "C:\Program Files (x86)\Graphviz2.38\bin\dot.exe" -Tsvg -oFoo.svg -Kdot Foo.dot
-//        will produce a Foo.svg file that can be opened with any svg-capable browser (e.g. IE).
+//      - The command:
+//           "C:\Program Files (x86)\Graphviz2.38\bin\dot.exe" -Tsvg -oFoo.svg -Kdot Foo.dot
+//        will produce a Foo.svg file that can be opened with any svg-capable browser.
+//    - https://sketchviz.com/
 //    - http://rise4fun.com/Agl/
 //      - Cut and paste the graph from your .dot file, replacing the digraph on the page, and then click the play
 //        button.
 //      - It will show a rotating '/' and then render the graph in the browser.
-//    MSAGL has also been open-sourced to https://github.com/Microsoft/automatic-graph-layout.git.
+//    MSAGL has also been open-sourced to https://github.com/Microsoft/automatic-graph-layout.
 //
 //    Here are the config values that control it:
 //      COMPlus_JitDumpFg       A string (ala the COMPlus_JitDump string) indicating what methods to dump flowgraphs
@@ -659,30 +596,33 @@ FILE* Compiler::fgOpenFlowGraphFile(bool* wbDontClose, Phases phase, LPCWSTR typ
 //      COMPlus_JitDumpFgDir    A path to a directory into which the flowgraphs will be dumped.
 //      COMPlus_JitDumpFgFile   The filename to use. The default is "default.[xml|dot]".
 //                              Note that the new graphs will be appended to this file if it already exists.
+//      COMPlus_NgenDumpFg      Same as COMPlus_JitDumpFg, but for ngen compiles.
+//      COMPlus_NgenDumpFgDir   Same as COMPlus_JitDumpFgDir, but for ngen compiles.
+//      COMPlus_NgenDumpFgFile  Same as COMPlus_JitDumpFgFile, but for ngen compiles.
 //      COMPlus_JitDumpFgPhase  Phase(s) after which to dump the flowgraph.
 //                              Set to the short name of a phase to see the flowgraph after that phase.
 //                              Leave unset to dump after COLD-BLK (determine first cold block) or set to * for all
 //                              phases.
-//      COMPlus_JitDumpFgDot    Set to non-zero to emit Dot instead of Xml Flowgraph dump. (Default is xml format.)
+//      COMPlus_JitDumpFgDot    0 for xml format, non-zero for dot format. (Default is dot format.)
 
 bool Compiler::fgDumpFlowGraph(Phases phase)
 {
     bool result        = false;
     bool dontClose     = false;
-    bool createDotFile = false;
-    if (JitConfig.JitDumpFgDot())
-    {
-        createDotFile = true;
-    }
+    bool createDotFile = true;
 
-    FILE* fgxFile = fgOpenFlowGraphFile(&dontClose, phase, createDotFile ? W("dot") : W("fgx"));
+#ifdef DEBUG
+    createDotFile = JitConfig.JitDumpFgDot() != 0;
+#endif // DEBUG
 
+    FILE* fgxFile = fgOpenFlowGraphFile(&dontClose, phase, createDotFile ? W("dot") : W("fgx"));
     if (fgxFile == nullptr)
     {
         return false;
     }
+
     bool        validWeights  = fgHaveValidEdgeWeights;
-    double      weightDivisor = (double)fgCalledCount;
+    double      weightDivisor = (double)BasicBlock::getCalledCount(this);
     const char* escapedString;
     const char* regionString = "NONE";
 
@@ -824,14 +764,18 @@ bool Compiler::fgDumpFlowGraph(Phases phase)
             }
 
             const char* rootTreeOpName = "n/a";
-            if (block->lastNode() != nullptr)
+            if (block->IsLIR() || (block->lastStmt() != nullptr))
             {
-                rootTreeOpName = GenTree::OpName(block->lastNode()->OperGet());
+                if (block->lastNode() != nullptr)
+                {
+                    rootTreeOpName = GenTree::OpName(block->lastNode()->OperGet());
+                }
             }
 
             fprintf(fgxFile, "\n            weight=");
             fprintfDouble(fgxFile, ((double)block->bbWeight) / weightDivisor);
-            fprintf(fgxFile, "\n            codeEstimate=\"%d\"", fgGetCodeEstimate(block));
+            // fgGetCodeEstimate() will assert if the costs have not yet been initialized.
+            // fprintf(fgxFile, "\n            codeEstimate=\"%d\"", fgGetCodeEstimate(block));
             fprintf(fgxFile, "\n            startOffset=\"%d\"", block->bbCodeOffs);
             fprintf(fgxFile, "\n            rootTreeOp=\"%s\"", rootTreeOpName);
             fprintf(fgxFile, "\n            endOffset=\"%d\"", block->bbCodeOffsEnd);

src/coreclr/jit/gentree.cpp

@@ -161,7 +161,7 @@ static void printIndent(IndentStack* indentStack)
 
 #endif
 
-#if defined(DEBUG) || NODEBASH_STATS || MEASURE_NODE_SIZE || COUNT_AST_OPERS
+#if defined(DEBUG) || NODEBASH_STATS || MEASURE_NODE_SIZE || COUNT_AST_OPERS || DUMP_FLOWGRAPHS
 
 static const char* opNames[] = {
 #define GTNODE(en, st, cm, ok) #en,

src/coreclr/jit/gentree.h

@@ -1844,7 +1844,7 @@ public:
 
 //---------------------------------------------------------------------
 
-#if defined(DEBUG) || NODEBASH_STATS || MEASURE_NODE_SIZE || COUNT_AST_OPERS
+#if defined(DEBUG) || NODEBASH_STATS || MEASURE_NODE_SIZE || COUNT_AST_OPERS || DUMP_FLOWGRAPHS
     static const char* OpName(genTreeOps op);
 #endif
 

src/coreclr/jit/jitconfigvalues.h

@@ -9,10 +9,11 @@
 #define OPT_CONFIG // Enable optimization level configuration.
 #endif
 
+#if defined(DEBUG)
+
 ///
 /// JIT
 ///
-#if defined(DEBUG)
 CONFIG_INTEGER(AltJitLimit, W("AltJitLimit"), 0)               // Max number of functions to use altjit for (decimal)
 CONFIG_INTEGER(AltJitSkipOnAssert, W("AltJitSkipOnAssert"), 0) // If AltJit hits an assert, fall back to the fallback
                                                                // JIT. Useful in conjunction with
@@ -64,9 +65,8 @@ CONFIG_INTEGER(JitAlignLoopAdaptive,
 
 CONFIG_INTEGER(JitDirectAlloc, W("JitDirectAlloc"), 0)
 CONFIG_INTEGER(JitDoubleAlign, W("JitDoubleAlign"), 1)
-CONFIG_INTEGER(JitDumpASCII, W("JitDumpASCII"), 1)         // Uses only ASCII characters in tree dumps
-CONFIG_INTEGER(JitDumpFgDot, W("JitDumpFgDot"), 0)         // Set to non-zero to emit Dot instead of Xml Flowgraph dump
-CONFIG_INTEGER(JitDumpTerseLsra, W("JitDumpTerseLsra"), 1) // Produce terse dump output for LSRA
+CONFIG_INTEGER(JitDumpASCII, W("JitDumpASCII"), 1)               // Uses only ASCII characters in tree dumps
+CONFIG_INTEGER(JitDumpTerseLsra, W("JitDumpTerseLsra"), 1)       // Produce terse dump output for LSRA
 CONFIG_INTEGER(JitDumpToDebugger, W("JitDumpToDebugger"), 0)     // Output JitDump output to the debugger
 CONFIG_INTEGER(JitDumpVerboseSsa, W("JitDumpVerboseSsa"), 0)     // Produce especially verbose dump output for SSA
 CONFIG_INTEGER(JitDumpVerboseTrees, W("JitDumpVerboseTrees"), 0) // Enable more verbose tree dumps
@@ -195,13 +195,14 @@ CONFIG_METHODSET(NgenUnwindDump, W("NgenUnwindDump")) // Dump the unwind codes f
 ///
 /// JIT
 ///
-CONFIG_STRING(JitDumpFg, W("JitDumpFg"))           // Dumps Xml/Dot Flowgraph for specified method
+CONFIG_METHODSET(JitDumpFg, W("JitDumpFg"))        // Dumps Xml/Dot Flowgraph for specified method
 CONFIG_STRING(JitDumpFgDir, W("JitDumpFgDir"))     // Directory for Xml/Dot flowgraph dump(s)
-CONFIG_STRING(JitDumpFgFile, W("JitDumpFgFile"))   // Filename for Xml/Dot flowgraph dump(s)
+CONFIG_STRING(JitDumpFgFile, W("JitDumpFgFile"))   // Filename for Xml/Dot flowgraph dump(s) (default: "default")
 CONFIG_STRING(JitDumpFgPhase, W("JitDumpFgPhase")) // Phase-based Xml/Dot flowgraph support. Set to the short name of a
                                                    // phase to see the flowgraph after that phase. Leave unset to dump
                                                    // after COLD-BLK (determine first cold block) or set to * for all
                                                    // phases
+CONFIG_INTEGER(JitDumpFgDot, W("JitDumpFgDot"), 1) // 0 == dump XML format; non-zero == dump DOT format
 CONFIG_STRING(JitLateDisasmTo, W("JITLateDisasmTo"))
 CONFIG_STRING(JitRange, W("JitRange"))
 CONFIG_STRING(JitStressModeNames, W("JitStressModeNames")) // Internal Jit stress mode: stress using the given set of
@@ -213,15 +214,16 @@ CONFIG_STRING(JitStressRange, W("JitStressRange"))               // Internal Jit
 ///
 /// NGEN
 ///
-CONFIG_STRING(NgenDumpFg, W("NgenDumpFg"))         // Ngen Xml Flowgraph support
-CONFIG_STRING(NgenDumpFgDir, W("NgenDumpFgDir"))   // Ngen Xml Flowgraph support
-CONFIG_STRING(NgenDumpFgFile, W("NgenDumpFgFile")) // Ngen Xml Flowgraph support
+CONFIG_METHODSET(NgenDumpFg, W("NgenDumpFg"))      // Ngen Xml/Dot flowgraph dump support
+CONFIG_STRING(NgenDumpFgDir, W("NgenDumpFgDir"))   // Ngen Xml/Dot flowgraph dump support
+CONFIG_STRING(NgenDumpFgFile, W("NgenDumpFgFile")) // Ngen Xml/Dot flowgraph dump support
 ///
 /// JIT Hardware Intrinsics
 ///
 CONFIG_INTEGER(EnableIncompleteISAClass, W("EnableIncompleteISAClass"), 0) // Enable testing not-yet-implemented
                                                                            // intrinsic classes
-#endif                                                                     // defined(DEBUG)
+
+#endif // defined(DEBUG)
 
 #if FEATURE_LOOP_ALIGN
 CONFIG_INTEGER(JitAlignLoops, W("JitAlignLoops"), 1) // If set, align inner loops

src/coreclr/jit/lclvars.cpp

@@ -3239,6 +3239,7 @@ unsigned Compiler::lvaLclExactSize(unsigned varNum)
 //  if we don't have profile data then getCalledCount will return BB_UNITY_WEIGHT (100)
 //  otherwise it returns the number of times that profile data says the method was called.
 //
+// static
 BasicBlock::weight_t BasicBlock::getCalledCount(Compiler* comp)
 {
     // when we don't have profile data then fgCalledCount will be BB_UNITY_WEIGHT (100)

src/coreclr/jit/optimizer.cpp

@@ -4611,6 +4611,7 @@ PhaseStatus Compiler::optFindLoops()
 
         // Check if any of the loops need alignment
 
+        JITDUMP("\n");
         optIdentifyLoopsForAlignment();
 
 #if COUNT_LOOPS

src/coreclr/jit/phase.cpp

@@ -197,10 +197,6 @@ void Phase::PostPhase(PhaseStatus status)
             printf("Trees after %s\n", m_name);
             comp->fgDispBasicBlocks(true);
         }
-
-#if DUMP_FLOWGRAPHS
-        comp->fgDumpFlowGraph(m_phase);
-#endif // DUMP_FLOWGRAPHS
     }
 
     if (doPostPhase)
@@ -230,5 +226,9 @@ void Phase::PostPhase(PhaseStatus status)
 
 #endif // DEBUG
 
+#if DUMP_FLOWGRAPHS
+    comp->fgDumpFlowGraph(m_phase);
+#endif // DUMP_FLOWGRAPHS
+
     comp->EndPhase(m_phase);
 }