[PATCH] D40425: Extending CFGPrinter and CallPrinter with Heat Colors

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: include/llvm/Analysis/CFGPrinter.h:80
+
+  const Function *getF() { return this->F; }
+
----------------
getF -> getFunction


================
Comment at: include/llvm/Analysis/CFGPrinter.h:158
+    if (OutStr[0] == '\n')
+      OutStr.erase(OutStr.begin());
 
----------------
Can you commit these format changes separately as NFC?


================
Comment at: include/llvm/Analysis/CFGPrinter.h:255
+      for (unsigned i = 0; i < TI->getNumSuccessors(); i++) {
+        total += CFGInfo->getFreq(TI->getSuccessor(i));
+      }
----------------
total may overflow ..


================
Comment at: include/llvm/Analysis/CFGPrinter.h:272
+      //formating value to percentage
+      Attrs = formatv("label=\"{0:P}\"", val);
+    }
----------------
to get edge frequency, what you should do is to: get the edge probability information using BPI, and then multiply it with the source BB's frequency.

For edges, the branch probability data is also useful to be displayed.


================
Comment at: include/llvm/Analysis/CFGPrinter.h:279
+    std::string attrs = "";
+    if (CFGInfo->showHeatColors()) {
+      uint64_t freq = CFGInfo->getFreq(Node);
----------------
early return if not?


================
Comment at: lib/Analysis/CFGPrinter.cpp:26
 
-namespace {
-  struct CFGViewerLegacyPass : public FunctionPass {
-    static char ID; // Pass identifcation, replacement for typeid
-    CFGViewerLegacyPass() : FunctionPass(ID) {
-      initializeCFGViewerLegacyPassPass(*PassRegistry::getPassRegistry());
-    }
+static cl::opt<bool> ShowHeatColors("cfg-heat-colors", cl::init(false),
+                                    cl::Hidden,
----------------
IMO, it is better to make it 'true' by default.


================
Comment at: lib/Analysis/CFGPrinter.cpp:82
+    bool UseHeuristic = true;
+    uint64_t MaxFreq = getMaxFreq(F, BFI, UseHeuristic);
+    viewHeatCFGToDotFile(F, BFI, MaxFreq, UseHeuristic, /*isSimple=*/false);
----------------
use function level max freq can be misleading.  The function itself can be totally cold according to the profile summary. With profile data, it is better to use profile summary data: There are three ranges : hot, medium (between hot and cold), and cold.  


Repository:
  rL LLVM

https://reviews.llvm.org/D40425