[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Manman Ren via llvm-commits]

manmanren added a subscriber: manmanren.
manmanren added a comment.

Thanks for working on this!

Mostly nitpicking. I haven't really looked at the implementation yet.

Manman


================
Comment at: include/llvm/InitializePasses.h:123
@@ -122,1 +122,3 @@
+void initializeAddressSanitizerPass(PassRegistry &);
+void initializeAddressSanitizerModulePass(PassRegistry &);
 void initializeMemorySanitizerPass(PassRegistry&);
----------------
Nit: it is a little strange to change some of the formatting.

================
Comment at: include/llvm/Support/CFGMST.h:35
@@ +34,3 @@
+
+  // Store all the edges in CFG. It may contains some stale edges
+  // when Removed is set.
----------------
Nit: It may contain

================
Comment at: include/llvm/Support/CFGMST.h:49
@@ +48,3 @@
+
+  // find the root group of the G and compress the path from G to the root.
+  BBInfo *findAndCompressGroup(BBInfo *G) {
----------------
Nit: Find

================
Comment at: include/llvm/Support/CFGMST.h:165
@@ +164,3 @@
+    for (auto &BI : BBInfos) {
+      const BasicBlock *BB = BI.first;
+      DEBUG(dbgs() << "  BB: "
----------------
This will cause "unused variable" warning when building clang without assertion.

================
Comment at: include/llvm/Support/CFGMST.h:173
@@ +172,3 @@
+                 << " (*: Instrument, C: CriticalEdge, -: Removed)\n");
+    uint32_t Count = 0;
+    for (auto &EI : AllEdges) {
----------------
Same here.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:247
@@ -221,3 +246,3 @@
     addExtensionsToPM(EP_Peephole, MPM);
-    MPM.add(createCFGSimplificationPass());   // Clean up after IPCP & DAE
+    MPM.add(createCFGSimplificationPass()); // Clean up after IPCP & DAE
   }
----------------
Can you commit this separately if it is necessary?

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:140
@@ +139,3 @@
+  // Return the information string of an edge.
+  virtual const StringRef infoString() {
+    std::string Str = (Removed ? "-" : " ");
----------------
Should you add const here "infoString() const {"?

================
Comment at: lib/Transforms/Instrumentation/PGOIRInstr.cpp:145
@@ +144,3 @@
+    Str += "  W=" + std::to_string(Weight);
+    return StringRef(Str);
+  }
----------------
This seems wrong. StringRef does not own the string data.


http://reviews.llvm.org/D12781