[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Sean Silva via llvm-commits]

On Mon, Nov 16, 2015 at 9:34 PM, Xinliang David Li <davidxl at google.com>
wrote:

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

Thanks. Let's get that in a comment.

-- Sean Silva


>
> 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/159e23d5/attachment.html>