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

[Xinliang David Li via llvm-commits]

On Fri, May 27, 2016 at 8:50 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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.
>

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.

David


> -- 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/6db62c21/attachment.html>