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

[Xinliang David Li via llvm-commits]

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

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