<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 23, 2015 at 4:39 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Sep 23, 2015 at 3:28 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: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"><span>On Tue, Sep 22, 2015 at 6:09 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 reviewer: kcc.<br>
silvas added a comment.<br>
<br>
This should not be called "late instrumentation". That is clang-specific terminology. Think from the perspective of a random frontend (say, one that doesn't have any other PGO mechanism). There should be no mention of "FE instrumentation", "late instrumentation" "IR-level instrumentation", etc. This is just "edge profiling" or something like that.<br></blockquote><div><br></div></span><div>these terms are mainly for the reviewers to distinguish the two instrumentation. I'll clean up the code and comments to use "edge profiling".</div></div></div></div></blockquote><div><br></div></span><div>Thanks.</div><span class=""><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><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>
A couple other high-level questions/comments:<br>
<br>
- is there a reason that this is breaking critical edges itself instead of using break-crit-edges pass? Is break-crit-edges dead? SanitizerCoverage doesn't seem to use it either.<br></blockquote><div><br></div></span><div>As I mentioned in the email to Kostya: this is to avoid unnecessary split. We only need to split a small set of the critical edges. </div><span><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">
- why is this a module pass? It operates entirely on a function-by-function basis.<br></blockquote></span><div>Good question. When I started the implementation, I thought I would create global functions and global vars that needed to be a module pass. But now it seems function pass should work. I'll change this in a later patch.</div><span><div><br></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">
- can we use GraphTraits for the MST algorithm, so that it is reusable? (could probably be a separate patch if so).<br></blockquote></span><div>This can be done in the future in a later patch. Not sure what other pass would use MST.</div></div></div></div></blockquote><div><br></div></span><div>At the very least, it may help to be able to unittest the MST algorithm. Your current patch has no testing for this; how are you sure that your MST algorithm is correct? (actually, this patch has no tests at all; please add testing to the patch once we have converged on the final design).</div><span class=""><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><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">
- could we break the PGO data reading into a separate analysis? Also could the pass be split into more fine-grained passes? (I think the answer is yes to both questions)<br></blockquote><div><br></div></span><div>I would prefer to put instrumentation and reading profile (read the count out and populate to each BB) in the same pass. This two passes need to see the matched IR. Separating them will create more changes of mismatches. </div></div></div></div></blockquote><div><br></div></span><div>The problem is that now we have a pass that really is two separate passes. If we look in PGOLateInstrumentationFunc:: PGOLateInstrumentationFunc, we can see that there is a shared block of code:</div><div><br></div><div><div> setFuncName(F);</div><div> buildEdges();</div><div> sortEdges();</div><div> computeMinimumSpanningTree();</div><div> computeCFGHash();</div><div> dumpEdges();</div></div><div><br></div><div>Then the pass does two radically different things:</div><div><br></div><div><div> if (!IsUse) {</div><div> instrumentCFG();</div><div> return;</div><div> }</div><div><br></div><div> if (readCounters(Filename)) {</div><div> populateCounters();</div><div> setBranchWeights();</div><div> dumpEdges(true);</div><div> }</div></div><div> </div><div>The shared block of code seems like it could be pulled out into a helper class that can be shared by the two passes.</div><div><br></div><div>Precisely because the two different operations are so tightly coupled, we have to clearly separate the single point of truth that they both rely on. Otherwise it is difficult to reason about this close coupling. This piece of functionality should be clearly separated (separate class/function; a method is insufficient isolation because then there can be accidental interdependence due to member variables).</div><div><br></div></div></div></div></blockquote><div><br></div><div>Moving CfgMst analysis into its own class as utility/help class, and make Instrumentation and Profile Annotation two distinct passes sounds reasonable.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>Also, in order to fully benefit from the MST, we will need to be able to read the counters earlier (but if I understand correctly, don't need to set the branch weight metadata). This indicates that, roughly speaking, `populateCounters()` should be clearly separated.</div></div></div></div></blockquote><div><br></div><div>Not sure what you mean here. populateCounters depend on readCounters ..</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Even if we don't expose two different passes (for the reason you described), internally in the implementation there has to be a clear separation. I'm still not convinced that we shouldn't expose two different passes, but it should be easy to make that choice later once the internal implementation is cleanly factored.</div><span class=""><div><br></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"><div><br></div><div>But I could move some of the code, like setting function attributes into a separated pass.</div><span><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">
- We should directly accept a buffer containing the profile feedback data instead of a file name. That lets the frontend do the error handling, and is more flexible for usage as a library.<br></blockquote><div><br></div></span><div>Good suggestion. I'll work on this.</div></div></div></div></blockquote><div><br></div></span><div>Now that I look at the code again, instead of a raw buffer, it probably makes sense for the frontend to pass in an IndexedInstrProfReader directly. The idea is that as many "error conditions" as possible should be pushed up into the frontend.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div class="h5"><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"><div><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>
Adding Kostya, as this has a lot of similarities to sanitizer coverage; also just for general knowledge of instrumentation passes.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:122-126<br>
@@ +121,7 @@<br>
+<br>
+ // Threshold of the hot functions.<br>
+ const float HotFunctionThreshold = 0.01;<br>
+<br>
+ // Threshold of the cold functions.<br>
+ const float ColdFunctionThreshold = 0.0002;<br>
+<br>
----------------<br>
Use integers.<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:301-302<br>
@@ +300,4 @@<br>
+<br>
+// Compute the CRC32 of unsigned integers.<br>
+unsigned PGOLateInstrumentationFunc::crc32UnsignedBits(unsigned Chksum,<br>
+ unsigned Value,<br>
----------------<br>
Why crc32?<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/PGOLateInstr.cpp:313-314<br>
@@ +312,4 @@<br>
+<br>
+// Compute Hash value for the CFG: the lower 32 bits are CRC32 of the index<br>
+// value of each BB in the CFG. The higher 32 bits record the number of edges.<br>
+void PGOLateInstrumentationFunc::computeCFGHash() {<br>
----------------<br>
This seems like a very poor use of the bits of the hash. Why not use md5 like clang's `PGOHash`?<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></div></div><br></div></div>
</blockquote></div><br></div></div>