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

[Easwaran Raman via llvm-commits]

eraman added inline comments.

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

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


http://reviews.llvm.org/D17178