[PATCH] D12781: PGO IR-level instrumentation infrastructure

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:200
@@ +199,3 @@
+  // Give an edge, find the BB that will be instrumented.
+  // Return nullptr if no BB to be instrumented.
+  BasicBlock *getInstrBB(Edge *E);
----------------
if there is no BB ..

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:408
@@ +407,3 @@
+  // one unknown edge.
+  void setUnknownCountEdge(DirectEdges &Edges, uint64_t Value);
+
----------------
It is better to just call it 'setEdgeCount' with same documentation -- or make it just 

getUnknownCountEdge() and let the client to set the edge directly.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:449
@@ +448,3 @@
+  for (unsigned i = 0, j = 0, E = MST.AllEdges.size(); i < E; i++) {
+    const std::unique_ptr<PGOUseEdge> &Ei = MST.AllEdges[i];
+    BasicBlock *InstrBB = getInstrBB(Ei.get());
----------------
silvas wrote:
> Avoid `const std::unique_ptr<PGOUseEdge> &`.
Sean's comment here is not addressed -- use raw PGOUseEdge&

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:460
@@ +459,3 @@
+      // Add new edge of SrcBB->InstrBB.
+      PGOUseEdge &NewEdge = MST.addEdge(SrcBB, InstrBB, 0);
+      NewEdge.setEdgeCount(CountValue);
----------------
In general it is not safe to update the container (AllEdges) while iterating -- it may invalidate the iterator or element reference saved before.  The code should create a worklist of edges before the count setting -- the worklist will then be 'frozen' (It is safe to update worklist, but there is no need to push new split edges into the list) and AllEdges can be safely updated. 

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:496
@@ +495,3 @@
+  std::vector<uint64_t> CountFromProfile;
+  if (std::error_code EC = PGOReader->getFunctionCounts(FuncName, FunctionHash,
+                                                        CountFromProfile)) {
----------------
Use the new getInstrProfRecord method -- eventually we will need to read value profile data too.


http://reviews.llvm.org/D12781