<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><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><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><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><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><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><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><div>This can be done in the future in a later patch. Not sure what other pass would use MST.</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">
- 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><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><br></div><div>But I could move some of the code, like setting function attributes into a separated pass.</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">
- 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><div>Good suggestion. I'll work on this.</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><br></div></div>