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

[Sean Silva via llvm-commits]

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.

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