[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Sean Silva via llvm-commits]

silvas added a comment.

Some more small comments.


================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:93
@@ +92,3 @@
+
+#define CRITICAL_EDGE_MULTIPLIER 1000
+
----------------
Use a real variable.

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:191
@@ +190,3 @@
+      // Newly inserted, update the real info.
+      Iter->second = std::move(std::unique_ptr<BBInfo>(new BBInfo(Index)));
+    AllEdges.emplace_back(new Edge(Src, Dest, W));
----------------
llvm::make_unique

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:193
@@ +192,3 @@
+    AllEdges.emplace_back(new Edge(Src, Dest, W));
+    return *(AllEdges.back().get());
+  }
----------------
`return *AllEdges.back()`

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:31
@@ +30,3 @@
+// spanning tree (MST) of the CFG. All edges that not in the MST will be
+// instrumented. The MST implementation is in Class CFGMST.
+//
----------------
Give intuition for why it is edges *not in* the MST rather than edges *in* the MST? Edges with high counts should have high weight and therefore *not* be in the MST (which tries for minimum weight); we don't want to instrument edges with high counts, and they are *not* in the MST, so why would we place counters there?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:148
@@ +147,3 @@
+/// Implements a Minimum Spanning Tree (MST) based instrumentation for PGO
+/// in the function level.
+//
----------------
Please cite the Knuth paper. Also explain what we actually do with the MST (and give intuition for why that makes sense (it's fairly simple, should not need a long explanation)).

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:293
@@ +292,3 @@
+  // This member stores the shared information with class PGOUseFunc.
+  FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo;
+
----------------
Make these two variable just local variables of `instrumentOnFunc` free function.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:306
@@ +305,3 @@
+// Critical edges will be split.
+void PGOGenFunc::instrumentCFG() {
+  unsigned NumCounters = 0;
----------------
This is only called in one place, inline the code into `instrumentOnFunc`.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:473
@@ +472,3 @@
+    } else
+      Ei->setEdgeCount(CountValue);
+
----------------
Can you use continue to reduce indentation for the "large" side of the `if`? ( http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code )


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:513
@@ +512,3 @@
+  }
+  std::vector<uint64_t> CountFromProfile = Result.get().Counts;
+
----------------
Do you mean to do a copy here? Probably `std::vector<uint64_t> &` is intended.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:518
@@ +517,3 @@
+  uint64_t ValueSum = 0;
+  for (unsigned i = 0, E = CountFromProfile.size(); i < E; i++) {
+    DEBUG(dbgs() << "  " << i << ": " << CountFromProfile[i] << "\n");
----------------
`e` instead of `E`, as is common in LLVM.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:543
@@ +542,3 @@
+    BasicBlock *DestBB = const_cast<BasicBlock *>(Ei->DestBB);
+    UseBBInfo &SrcInfo = getBBInfo(SrcBB);
+    UseBBInfo &DestInfo = getBBInfo(DestBB);
----------------
`getBBInfo(Ei->SrcBB)`. You shouldn't need a cast here (if you do, then please fix whatever code is requiring this to cast away constness).

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:566
@@ +565,3 @@
+      BasicBlock *BB = &BBB;
+      UseBBInfo &Count = getBBInfo(BB);
+      if (!Count.CountValid) {
----------------
Just make the iteration variable `BB` and use `getBBInfo(&BB)`, like you do below in the loop `// Assert every BB has a valid counter.`.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:633
@@ +632,3 @@
+        continue;
+      unsigned SuccNum = GetSuccessorNumber(const_cast<BasicBlock *>(SrcBB),
+                                            const_cast<BasicBlock *>(DestBB));
----------------
Remove the const_cast (I think this will require making GetSuccessorNumber take `const BasicBlock *`, which you can do as a separate patch (no need for pre-commit review))

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:652
@@ +651,3 @@
+
+void static instrumentOnFunc(PGOGenFunc &Func) { Func.instrumentCFG(); }
+
----------------
Do you mean `instrumentOneFunc`? `instrumentOnFunc` doesn't make sense (weird use of "on").


http://reviews.llvm.org/D12781