<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 16, 2015 at 3:54 PM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">xur marked 17 inline comments as done.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Instrumentation/CFGMST.h:110<br>
@@ +109,3 @@<br>
+          BasicBlock *TargetBB = TI->getSuccessor(i);<br>
+          if ((Critical = isCriticalEdge(TI, i)))<br>
+            Weight = BPI->getEdgeProbability(&*BB, TargetBB)<br>
----------------<br>
</span><span class="">silvas wrote:<br>
> Move the declaration for `bool Critical = false;` down one line and simplify to `bool Critical = isCriticalEdge(TI, i);`<br>
</span>reorganized the code, also handles overflow.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:151<br>
@@ +150,3 @@<br>
+  const BasicBlock *DestBB;<br>
+  unsigned Weight;<br>
+  bool InMST;<br>
----------------<br>
</span><span class="">silvas wrote:<br>
> Other places seem to use uint64_t for weight. Why `unsigned` here?<br>
</span>changed to uint64_t.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:439<br>
@@ +438,3 @@<br>
+                            std::to_string(FunctionHash);<br>
+      MST.dumpEdges(Message);<br>
+    }<br>
----------------<br>
</span><span class="">silvas wrote:<br>
> Avoid having debug code be non-conditional.<br>
</span>Now guarded by DEBUG.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:460<br>
@@ +459,3 @@<br>
+      // Add new edge of SrcBB->InstrBB.<br>
+      PGOUseEdge &NewEdge = MST.addEdge(SrcBB, InstrBB, 0);<br>
+      NewEdge.setEdgeCount(CountValue);<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> 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.<br>
</span>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.<br></blockquote><div><br></div><div>In general it is still not safe -- even with size recompute and using index is not good enough. Suppose there is this code in the loop  T& elm = my_vector[i]; -- a push_back may do a reallocation and invalidate the reference and any use of elm after the push_back in the same loop iteration will be invalid. Since you have changed to use PGOUseEdge* directly instead of reference to unique_ptr, the code as it is now is safe. </div><div><br></div><div>A safer approach is to copy AllEdges into a different vector and use range based loop on it -- but it may cost a little more compile time. Another approach is to put removed Edges and InstrBB into a pending list and do their count setting outside the loop.  Neither approach is ideal, but it protects you from future breakage ..</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="http://reviews.llvm.org/D12781" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12781</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>