[PATCH] D17178: Add profile summary support for sample profile

[Diego Novillo via llvm-commits]

dnovillo requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/ProfileData/SampleProfReader.h:139
@@ +138,3 @@
+//    MAX_BLOCK_COUNT (uint64_t)
+//        Maximum number of samples on a line.
+//    MAX_FUNCTION_COUNT (uint64_t)
----------------
eraman wrote:
> dnovillo wrote:
> > Well, this is always exactly one for sampling profiles.  There's always one count per line.
> I use count to mean samples. The fields of ProfileSummary are named *Counts and in the context of sample profiles, I use those fields to represent samples. Thus, MaxBlockCount contains the max among samples on any given line. 
Ah, yes, that makes sense.  Thanks.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:20
@@ -18,1 +19,3 @@
 
+const std::vector<uint32_t> ProfileSummary::DefaultCutoffs(
+    {10000,  /*  1% */
----------------
Could you add a comment describing these cutoffs and a general idea of what the numbers are and mean?

================
Comment at: lib/ProfileData/SampleProfReader.cpp:403
@@ -399,3 +402,3 @@
     return EC;
   else if (*Version != SPVersion())
     return sampleprof_error::unsupported_version;
----------------
I'm wondering if we don't want to do something similar to what we do for instrumented profiles here.  We could make up a fake summary for older versions of the profile.

Right now, you should be seeing test failures for older binary profiles.  Are you not?

================
Comment at: lib/ProfileData/SampleProfReader.cpp:792
@@ +791,3 @@
+
+// For text and GCC file formats, we compute the summary aftter reading the
+// profile. Binary format has the profile summary in its header.
----------------
eraman wrote:
> dnovillo wrote:
> > s/aftter/after/
> > 
> > I would prefer to see a text version of the summary profile.  This is invaluable for generating test cases and general debugging.
> The reason why I decided to compute the summary after reading is that it made generating test cases easy. If it is part of the header, either we need to make it optional (and compute it while reading) or write the summary to the profile file. 
> 
> One thing I want to point out is that I'll add support to llvm-profdata to generate summaries for sample profile after this change. At present, it does generate summaries for instrumented profile, but that also needs to be cleaned up since it uses a different set of cutoffs. 
OK, thanks.  That sounds fine.

Still need to s/aftter/after/ in the comment.


http://reviews.llvm.org/D17178