<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 16, 2015 at 9:34 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Mon, Nov 16, 2015 at 8:47 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">silvas added a comment.<br>
<br>
Some more small comments.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/CFGMST.h:93<br>
@@ +92,3 @@<br>
+<br>
+#define CRITICAL_EDGE_MULTIPLIER 1000<br>
+<br>
----------------<br>
Use a real variable.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/CFGMST.h:191<br>
@@ +190,3 @@<br>
+ // Newly inserted, update the real info.<br>
+ Iter->second = std::move(std::unique_ptr<BBInfo>(new BBInfo(Index)));<br>
<span>+ AllEdges.emplace_back(new Edge(Src, Dest, W));<br>
</span>----------------<br>
llvm::make_unique<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/CFGMST.h:193<br>
@@ +192,3 @@<br>
<span>+ AllEdges.emplace_back(new Edge(Src, Dest, W));<br>
</span>+ return *(AllEdges.back().get());<br>
+ }<br>
----------------<br>
`return *AllEdges.back()`<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:31<br>
@@ +30,3 @@<br>
+// spanning tree (MST) of the CFG. All edges that not in the MST will be<br>
+// instrumented. The MST implementation is in Class CFGMST.<br>
+//<br>
----------------<br>
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?<br>
<br></blockquote><div><br></div></div></div><div>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.</div></div></div></div></blockquote><div><br></div><div>Thanks. Let's get that in a comment.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><font color="#888888"><div><br></div><div>David</div></font></span><div><div class="h5"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:148<br>
@@ +147,3 @@<br>
+/// Implements a Minimum Spanning Tree (MST) based instrumentation for PGO<br>
+/// in the function level.<br>
+//<br>
----------------<br>
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)).<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:293<br>
@@ +292,3 @@<br>
+ // This member stores the shared information with class PGOUseFunc.<br>
+ FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo;<br>
+<br>
----------------<br>
Make these two variable just local variables of `instrumentOnFunc` free function.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:306<br>
@@ +305,3 @@<br>
+// Critical edges will be split.<br>
+void PGOGenFunc::instrumentCFG() {<br>
+ unsigned NumCounters = 0;<br>
----------------<br>
This is only called in one place, inline the code into `instrumentOnFunc`.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:473<br>
@@ +472,3 @@<br>
+ } else<br>
+ Ei->setEdgeCount(CountValue);<br>
+<br>
----------------<br>
Can you use continue to reduce indentation for the "large" side of the `if`? ( <a href="http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code" rel="noreferrer" target="_blank">http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code</a> )<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:513<br>
@@ +512,3 @@<br>
+ }<br>
+ std::vector<uint64_t> CountFromProfile = Result.get().Counts;<br>
+<br>
----------------<br>
Do you mean to do a copy here? Probably `std::vector<uint64_t> &` is intended.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:518<br>
@@ +517,3 @@<br>
+ uint64_t ValueSum = 0;<br>
+ for (unsigned i = 0, E = CountFromProfile.size(); i < E; i++) {<br>
+ DEBUG(dbgs() << " " << i << ": " << CountFromProfile[i] << "\n");<br>
----------------<br>
`e` instead of `E`, as is common in LLVM.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:543<br>
@@ +542,3 @@<br>
+ BasicBlock *DestBB = const_cast<BasicBlock *>(Ei->DestBB);<br>
+ UseBBInfo &SrcInfo = getBBInfo(SrcBB);<br>
+ UseBBInfo &DestInfo = getBBInfo(DestBB);<br>
----------------<br>
`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).<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:566<br>
@@ +565,3 @@<br>
+ BasicBlock *BB = &BBB;<br>
+ UseBBInfo &Count = getBBInfo(BB);<br>
+ if (!Count.CountValid) {<br>
----------------<br>
Just make the iteration variable `BB` and use `getBBInfo(&BB)`, like you do below in the loop `// Assert every BB has a valid counter.`.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:633<br>
@@ +632,3 @@<br>
<span>+ continue;<br>
+ unsigned SuccNum = GetSuccessorNumber(const_cast<BasicBlock *>(SrcBB),<br>
+ const_cast<BasicBlock *>(DestBB));<br>
----------------<br>
</span>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))<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:652<br>
@@ +651,3 @@<br>
+<br>
+void static instrumentOnFunc(PGOGenFunc &Func) { Func.instrumentCFG(); }<br>
+<br>
----------------<br>
Do you mean `instrumentOneFunc`? `instrumentOnFunc` doesn't make sense (weird use of "on").<br>
<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></div></div><br></div></div>
</blockquote></div><br></div></div>