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

[Sean Silva via llvm-commits]

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

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

Ah, for some reason I was thinking that it was sorted in ascending order.
Thanks for the explanation.

-- Sean Silva


>
> 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/218a37d3/attachment-0001.html>