[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Xinliang David Li via llvm-commits]

On Mon, Nov 16, 2015 at 3:54 PM, Rong Xu <xur at google.com> wrote:

> 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.
>

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.

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 ..

David


>
>
> http://reviews.llvm.org/D12781
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151116/9f156713/attachment.html>