[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Rong Xu via llvm-commits]

xur marked 44 inline comments as done.
xur added a comment.

Thank David and Sean for the detailed comments -- they are really helpful.
I integrated the review to the patch. Could you please take a look. Thanks!


================
Comment at: include/llvm/Support/CFGMST.h:94
@@ +93,3 @@
+
+    // Special handling for single BB functions.
+    if (succ_empty(BB)) {
----------------
davidxl wrote:
> Add more comments explaining here why need two duplicate fake edges.
This is a mistaken introduced in the lat minute formatting. Fixed.

================
Comment at: include/llvm/Support/CFGMST.h:104
@@ +103,3 @@
+      uint64_t Weight;
+      if (int successors = TI->getNumSuccessors()) {
+        for (int i = 0; i != successors; ++i) {
----------------
davidxl wrote:
> Redundant guard.
why Redundant? numsuccessors can be 0 (and these is else branch)

================
Comment at: include/llvm/Support/CFGMST.h:146
@@ +145,3 @@
+          if (unionGroups(Ei->SrcBB, Ei->DestBB))
+            Ei->InMST = true;
+        }
----------------
davidxl wrote:
> Should this be marked unconditionally here? Otherwise, we can simply boost the weight for such edges even higher without special treatment here?
The reason to split two passes is for the case of using profile count value as the weight, in which the values can vary drastically. 

================
Comment at: include/llvm/Support/CFGMST.h:185
@@ +184,3 @@
+  const std::unique_ptr<Edge> &addEdge(const BasicBlock *Src,
+                                       const BasicBlock *Dest, unsigned W = 2) {
+    if (BBInfos.find(Src) == BBInfos.end()) {
----------------
davidxl wrote:
> Use a const var for default weight.
removed the default weight.

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:161
@@ +160,3 @@
+  // Return the information string of this object.
+  virtual const std::string infoString() const {
+    return "Index=" + std::to_string(Index);
----------------
silvas wrote:
> Do you actually need dynamic dispatch?
it returns different information strings. I can get rid of it. But I don't think it really matters -- it only used in debug printing.

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:524
@@ +523,3 @@
+  // This IR instrumentation handles it differently from Clang-based
+  // instrumentation.
+  ProgramMaxCount = PGOReader->getMaximumFunctionCount();
----------------
silvas wrote:
> In what way? And why does it matter to know that here?
you are right. This comment is unnecessary and does not belong here. Deleted.

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:631
@@ +630,3 @@
+      EdgeCounts[s] = 0;
+    uint64_t MaxCount;
+    for (unsigned s = 0; s < Size; s++) {
----------------
silvas wrote:
> Is this ever initialized?
good catch. fixed.

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:638
@@ +637,3 @@
+        continue;
+      unsigned SuccNum = GetSuccessorNumber(const_cast<BasicBlock *>(SrcBB),
+                                            const_cast<BasicBlock *>(DestBB));
----------------
silvas wrote:
> Can you fix GetSuccessorNumber to take a `const BasicBlock *`? (hopefully it is a pretty obvious fix and there is no need for pre-commit review).
> Then you can get rid of all these const_cast's.
Will do this later.


http://reviews.llvm.org/D12781