[PATCH] D77172: Heat Coloring (3/3): Adding Heat Functionality to CallPrinter

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/Analysis/HeatUtils.h:34
+uint64_t
+getNumOfCalls(CallSite &callsite,
+              function_ref<BlockFrequencyInfo *(Function &)> LookupBFI);
----------------
Use CallBase instead of CallSite


================
Comment at: llvm/lib/Analysis/CallPrinter.cpp:34
+static cl::opt<bool>
+    EstimateEdgeWeight("callgraph-weights", cl::init(false), cl::Hidden,
+                       cl::desc("Show edges labeled with weights"));
----------------
ShowEdgeWeight and callgraph-show-weights ?


================
Comment at: llvm/lib/Analysis/CallPrinter.cpp:66
+      uint64_t localMaxFreq = 0;
+      for (auto pairF = M->getFunctionList().begin(); pairF != M->getFunctionList().end(); ++pairF) {
+        localMaxFreq += getNumOfCalls(*pairF, F, LookupBFI);
----------------
This is O(N^2) to the number of functions.  For a C++ module, this can be quite large.


================
Comment at: llvm/lib/Analysis/CallPrinter.cpp:71
+        MaxFreq = localMaxFreq;
+      Freq[&F] = localMaxFreq;
+    }
----------------
why max but not sum?


================
Comment at: llvm/lib/Analysis/CallPrinter.cpp:108
+        for (auto CI = Node->begin(), CE = Node->end(); CI != CE; CI++) {
+          if (!Visited.count(CI->second->getFunction()))
+            Visited.insert(CI->second->getFunction());
----------------
can directly use insert method and check its return value


================
Comment at: llvm/lib/Analysis/CallPrinter.cpp:209
+        if (CS.getCaller() == Caller) {
+          if (VisitedCallSites.count(U))
+            continue;
----------------
what is this check for?


================
Comment at: llvm/lib/Analysis/HeatUtils.cpp:46
+  BasicBlock *BB = CS.getInstruction()->getParent();
+  return LookupBFI(*CS.getCaller())->getBlockFreq(BB).getFrequency();
+}
----------------
As I mentioned in one of the previous reviews, the blockfrequency is relative to the parent function (because of the final scaling step used in computing the integer frequency value which depends on the min and max value) and it will be meaningless to compare block frequency of callsites in different functions. 


================
Comment at: llvm/lib/Analysis/HeatUtils.cpp:57
+      if (CS.getCaller() == (&callerFunction)) {
+          counter += 5;
+      }
----------------
where does the '5' come from?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77172/new/

https://reviews.llvm.org/D77172