[PATCH] D20648: Analysis pass to access profile summary info

[Sean Silva via llvm-commits]

On Fri, May 27, 2016 at 8:42 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Fri, May 27, 2016 at 6:07 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> silvas accepted this revision.
>> silvas added a comment.
>>
>> This LGTM with some nits.
>>
>>
>> ================
>> Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:100
>> @@ +99,3 @@
>> +/// \brief Printer pass that uses \c ProfileSummaryAnalysis.
>> +class FunctionHotnessPrinterPass
>> +    : public PassInfoMixin<FunctionHotnessPrinterPass> {
>> ----------------
>> eraman wrote:
>> > davidxl wrote:
>> > > Nit : ProfileSummaryPrinterPass?
>> > I thought FunctionHotnessPrinterPass accurately reflects what this is
>> doing. I have no strong opinion - will change it to
>> ProfileSummaryPrinterPass if you prefer.
>> 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).
>>
>> ================
>> Comment at: lib/Analysis/ProfileSummaryInfo.cpp:29
>> @@ +28,3 @@
>> +static cl::opt<int> HotCountPercentile(
>> +    "hot-count-percentile", cl::Hidden, cl::init(999000), cl::ZeroOrMore,
>> +    cl::desc("A count is hot if it exceeds the minimum count to"
>> ----------------
>> 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.
>> Since we require it to be one of the cutoffs in the profile summary, I
>> would call it "profile-summary-cutoff" or similar.
>>
>> ================
>> Comment at: lib/Analysis/ProfileSummaryInfo.cpp:34
>> @@ +33,3 @@
>> +static cl::opt<int> ColdCountPercentile(
>> +    "cold-count-percentile", cl::Hidden, cl::init(999999),
>> cl::ZeroOrMore,
>> +    cl::desc("A count is cold if it is below the minimum count"
>> ----------------
>> 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?
>> I.e. I would expect that ColdCountPercentile must be less than
>> HotCountPercentile.
>>
>
> See also my reply to Vedant's similar question.
>

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.


>   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).
>
> In other words, the smaller the percentile, the hotter or higher value the
> threshold is.
>

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.

-- Sean Silva


>
> David
>
>>
>> ================
>> Comment at: lib/Analysis/ProfileSummaryInfo.cpp:47
>> @@ +46,3 @@
>> +  // detailed summary.
>> +  assert(It != DS.end());
>> +  return It->MinCount;
>> ----------------
>> 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.
>>
>> ================
>> Comment at: lib/Analysis/ProfileSummaryInfo.cpp:138
>> @@ +137,3 @@
>> +// FIXME: This only tests isHotFunction and isColdFunction and not the
>> +// isHotCount and isColdCount calls.
>> +PreservedAnalyses FunctionHotnessPrinterPass::run(Module &M,
>> ----------------
>> You may want to mention that this isn't really a big deal since we are
>> setting the function count directly in the test.
>>
>>
>> http://reviews.llvm.org/D20648
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160527/88ae7674/attachment.html>