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

[Diego Novillo via llvm-commits]

dnovillo added a comment.

I just started looking at this.  Some early feedback so we can discuss whether my idea makes sense.  Mostly, I'm thinking we want to support summary data in text profiles for testing and debugging.  Does that make sense to you?


================
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)
----------------
Well, this is always exactly one for sampling profiles.  There's always one count per 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.
----------------
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.

================
Comment at: lib/ProfileData/SampleProfWriter.cpp:126
@@ -123,2 +125,3 @@
+    return EC;
   // Generate the name table for all the functions referenced in the profile.
   for (const auto &I : ProfileMap) {
----------------
One blank line above, please.


http://reviews.llvm.org/D17178