<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 27, 2016 at 8:50 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"><div><div class="h5">On Fri, May 27, 2016 at 8:42 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: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"><div><div>On Fri, May 27, 2016 at 6:07 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">silvas accepted this revision.<br>
silvas added a comment.<br>
<br>
This LGTM with some nits.<br>
<span><br>
<br>
================<br>
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:100<br>
@@ +99,3 @@<br>
+/// \brief Printer pass that uses \c ProfileSummaryAnalysis.<br>
+class FunctionHotnessPrinterPass<br>
+    : public PassInfoMixin<FunctionHotnessPrinterPass> {<br>
</span><span>----------------<br>
eraman wrote:<br>
> davidxl wrote:<br>
</span><span>> > Nit : ProfileSummaryPrinterPass?<br>
> I thought FunctionHotnessPrinterPass accurately reflects what this is doing. I have no strong opinion - will change it to ProfileSummaryPrinterPass if you prefer.<br>
</span>I think if we change ProfileSummaryAnalysis to have more predicates, we would want to extend this pass (e.g. we use it for testing), so ProfileSummaryPrinterPass seems a bit better (but it is just a nit).<br>
<br>
================<br>
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:29<br>
@@ +28,3 @@<br>
+static cl::opt<int> HotCountPercentile(<br>
+    "hot-count-percentile", cl::Hidden, cl::init(999000), cl::ZeroOrMore,<br>
+    cl::desc("A count is hot if it exceeds the minimum count to"<br>
----------------<br>
If the name is "percentile", I would expect it to be out of 100, but instead it is out of 1,000,000 which is confusing.<br>
Since we require it to be one of the cutoffs in the profile summary, I would call it "profile-summary-cutoff" or similar.<br>
<span><br>
================<br>
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:34<br>
@@ +33,3 @@<br>
+static cl::opt<int> ColdCountPercentile(<br>
+    "cold-count-percentile", cl::Hidden, cl::init(999999), cl::ZeroOrMore,<br>
+    cl::desc("A count is cold if it is below the minimum count"<br>
----------------<br>
</span>Sorry if I'm missing something obvious, but why is the cold percentile nearly 1,000,000? Wouldn't that mark almost everything as cold?<br>
I.e. I would expect that ColdCountPercentile must be less than HotCountPercentile.<br></blockquote><div><br></div></div></div><div>See also my reply to Vedant's similar question.</div></div></div></div></blockquote><div><br></div></div></div><div>Apologies for the duplication; I actually was replying yesterday and when I came in today I noticed that I hadn't submitted it. It seems like this stale comment got through.</div><span class=""><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>  Setting a very high (close to 100) percentile does not mark everything as cold. What it means is that that count threshold computed is so cold that the accumulative counts of (all BBs hotter than that value) covers almost 100% of the total count. Suppose this value is 100, it means that BBs with count <= 100 are considered cold (because the total count of all such cold BBs accounts for almost none of the total execution time).</div><div><br></div><div>In other words, the smaller the percentile, the hotter or higher value the threshold is. </div></div></div></div></blockquote><div><br></div></span><div>Is there a particular reason for doing it this way? (e.g. is this a common convention?) It seems that both Vedant and I were expecting it to be the opposite.</div></div></div></div></blockquote><div><br></div><div>It is related to how the threshold is computed -- counts are sorted in descending order and the cutoffs are computed based on how many percent of the total execution time (from the hottest BBs) you want to cover.</div><div><br></div><div>David</div><div><br></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"><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span class=""><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"><span><font color="#888888"><div><br></div><div>David</div></font></span><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
================<br>
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:47<br>
@@ +46,3 @@<br>
+  // detailed summary.<br>
+  assert(It != DS.end());<br>
+  return It->MinCount;<br>
----------------<br>
</span>This assertion can be triggered from the command line and is therefore not a correct assertion. Since this is not an option meant to be exposed directly to users (`-mllvm` is technical an "internal compiler flag"), I think that using report_fatal_error here is appropriate.<br>
<br>
================<br>
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:138<br>
@@ +137,3 @@<br>
+// FIXME: This only tests isHotFunction and isColdFunction and not the<br>
+// isHotCount and isColdCount calls.<br>
+PreservedAnalyses FunctionHotnessPrinterPass::run(Module &M,<br>
----------------<br>
You may want to mention that this isn't really a big deal since we are setting the function count directly in the test.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D20648" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20648</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>