[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Xinliang David Li via llvm-commits]

On Mon, Nov 16, 2015 at 8:47 PM, Sean Silva <chisophugis at gmail.com> wrote:

> silvas added a comment.
>
> Some more small comments.
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/CFGMST.h:93
> @@ +92,3 @@
> +
> +#define CRITICAL_EDGE_MULTIPLIER 1000
> +
> ----------------
> Use a real variable.
>
> ================
> Comment at: lib/Transforms/Instrumentation/CFGMST.h:191
> @@ +190,3 @@
> +      // Newly inserted, update the real info.
> +      Iter->second = std::move(std::unique_ptr<BBInfo>(new
> BBInfo(Index)));
> +    AllEdges.emplace_back(new Edge(Src, Dest, W));
> ----------------
> llvm::make_unique
>
> ================
> Comment at: lib/Transforms/Instrumentation/CFGMST.h:193
> @@ +192,3 @@
> +    AllEdges.emplace_back(new Edge(Src, Dest, W));
> +    return *(AllEdges.back().get());
> +  }
> ----------------
> `return *AllEdges.back()`
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:31
> @@ +30,3 @@
> +// spanning tree (MST) of the CFG. All edges that not in the MST will be
> +// instrumented. The MST implementation is in Class CFGMST.
> +//
> ----------------
> Give intuition for why it is edges *not in* the MST rather than edges *in*
> the MST? Edges with high counts should have high weight and therefore *not*
> be in the MST (which tries for minimum weight); we don't want to instrument
> edges with high counts, and they are *not* in the MST, so why would we
> place counters there?
>
>
The minimal spanning tree here is actually maximum weight tree -- on-tree
edges have higher frequencies. The number of edges on-tree is way higher
than the number of off-tree edges. Instrumenting on-tree edges also
introduces redundant profile update -- for instance it is common that all
incoming and all outgoing edges of one BB are on-tree.   Besides, it is
intuitively easier to force non-instrumentable edges on tree.

David



> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:148
> @@ +147,3 @@
> +/// Implements a Minimum Spanning Tree (MST) based instrumentation for PGO
> +/// in the function level.
> +//
> ----------------
> Please cite the Knuth paper. Also explain what we actually do with the MST
> (and give intuition for why that makes sense (it's fairly simple, should
> not need a long explanation)).
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:293
> @@ +292,3 @@
> +  // This member stores the shared information with class PGOUseFunc.
> +  FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo;
> +
> ----------------
> Make these two variable just local variables of `instrumentOnFunc` free
> function.
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:306
> @@ +305,3 @@
> +// Critical edges will be split.
> +void PGOGenFunc::instrumentCFG() {
> +  unsigned NumCounters = 0;
> ----------------
> This is only called in one place, inline the code into `instrumentOnFunc`.
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:473
> @@ +472,3 @@
> +    } else
> +      Ei->setEdgeCount(CountValue);
> +
> ----------------
> Can you use continue to reduce indentation for the "large" side of the
> `if`? (
> http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
> )
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:513
> @@ +512,3 @@
> +  }
> +  std::vector<uint64_t> CountFromProfile = Result.get().Counts;
> +
> ----------------
> Do you mean to do a copy here? Probably `std::vector<uint64_t> &` is
> intended.
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:518
> @@ +517,3 @@
> +  uint64_t ValueSum = 0;
> +  for (unsigned i = 0, E = CountFromProfile.size(); i < E; i++) {
> +    DEBUG(dbgs() << "  " << i << ": " << CountFromProfile[i] << "\n");
> ----------------
> `e` instead of `E`, as is common in LLVM.
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:543
> @@ +542,3 @@
> +    BasicBlock *DestBB = const_cast<BasicBlock *>(Ei->DestBB);
> +    UseBBInfo &SrcInfo = getBBInfo(SrcBB);
> +    UseBBInfo &DestInfo = getBBInfo(DestBB);
> ----------------
> `getBBInfo(Ei->SrcBB)`. You shouldn't need a cast here (if you do, then
> please fix whatever code is requiring this to cast away constness).
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:566
> @@ +565,3 @@
> +      BasicBlock *BB = &BBB;
> +      UseBBInfo &Count = getBBInfo(BB);
> +      if (!Count.CountValid) {
> ----------------
> Just make the iteration variable `BB` and use `getBBInfo(&BB)`, like you
> do below in the loop `// Assert every BB has a valid counter.`.
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:633
> @@ +632,3 @@
> +        continue;
> +      unsigned SuccNum = GetSuccessorNumber(const_cast<BasicBlock
> *>(SrcBB),
> +                                            const_cast<BasicBlock
> *>(DestBB));
> ----------------
> Remove the const_cast (I think this will require making GetSuccessorNumber
> take `const BasicBlock *`, which you can do as a separate patch (no need
> for pre-commit review))
>
> ================
> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:652
> @@ +651,3 @@
> +
> +void static instrumentOnFunc(PGOGenFunc &Func) { Func.instrumentCFG(); }
> +
> ----------------
> Do you mean `instrumentOneFunc`? `instrumentOnFunc` doesn't make sense
> (weird use of "on").
>
>
> http://reviews.llvm.org/D12781
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151116/cd57a192/attachment.html>