[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Manman Ren via llvm-commits]

manmanren added inline comments.

================
Comment at: include/llvm/Support/CFGMST.h:36
@@ +35,3 @@
+  // Store all the edges in CFG. It may contains some stale edges
+  // when Removed is set.
+  std::vector<std::unique_ptr<Edge>> AllEdges;
----------------
Can you add some high-level comments for CFGMST? What fields does this class expect on Edge and BBInfo? And what Removed field is used for? Do all members need to be public?

================
Comment at: include/llvm/Support/CFGMST.h:92
@@ +91,3 @@
+    // Add a fake edge to the entry.
+    addEdge(0, BB, BFI->getEntryFreq());
+
----------------
Use nullptr instead of 0?

================
Comment at: include/llvm/Support/CFGMST.h:181
@@ +180,3 @@
+
+  // Add edge to AllEdges with with Weight.
+  const std::unique_ptr<Edge> &addEdge(const BasicBlock *Src,
----------------
typo

================
Comment at: include/llvm/Support/CFGMST.h:187
@@ +186,3 @@
+      BBInfos.insert(std::make_pair(
+          Src, std::move(std::unique_ptr<BBInfo>(new BBInfo(Index)))));
+    }
----------------
make_unique?

================
Comment at: include/llvm/Support/CFGMST.h:195
@@ +194,3 @@
+    AllEdges.push_back(
+        std::move(std::unique_ptr<Edge>(new Edge(Src, Dest, W))));
+    return AllEdges[AllEdges.size() - 1];
----------------
emplace_back maybe?

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:194
@@ -181,1 +193,3 @@
 
+// Do PGO IR instrumentation generate or use operation as optiton
+// specified.
----------------
typo: option


http://reviews.llvm.org/D12781