[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Rong Xu via llvm-commits]

xur marked 17 inline comments as done.

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:110
@@ +109,3 @@
+          BasicBlock *TargetBB = TI->getSuccessor(i);
+          if ((Critical = isCriticalEdge(TI, i)))
+            Weight = BPI->getEdgeProbability(&*BB, TargetBB)
----------------
silvas wrote:
> Move the declaration for `bool Critical = false;` down one line and simplify to `bool Critical = isCriticalEdge(TI, i);`
reorganized the code, also handles overflow.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:151
@@ +150,3 @@
+  const BasicBlock *DestBB;
+  unsigned Weight;
+  bool InMST;
----------------
silvas wrote:
> Other places seem to use uint64_t for weight. Why `unsigned` here?
changed to uint64_t.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:439
@@ +438,3 @@
+                            std::to_string(FunctionHash);
+      MST.dumpEdges(Message);
+    }
----------------
silvas wrote:
> Avoid having debug code be non-conditional.
Now guarded by DEBUG.

================
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);
----------------
davidxl wrote:
> 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. 
I see your point. But this is exactly the reason I did not use range-based loop like all others in the file. I get the vector size in the loop initialization and use the index to reference the vector elements in the body. So adding element to the vector will not be a problem. I could use a worklist, but the effect should be the same.


http://reviews.llvm.org/D12781