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

[Easwaran Raman via llvm-commits]

eraman marked 2 inline comments as done.
eraman added a comment.

I had refactored the ProfileSummary class and made InstrProfSummary as a derived patch. I have merged in that change and added a SampleProfSummary as a new class and avoided the need to use InstrProf specific terms here.


================
Comment at: lib/ProfileData/ProfileSummary.cpp:20
@@ -18,1 +19,3 @@
 
+const std::vector<uint32_t> ProfileSummary::DefaultCutoffs(
+    {10000,  /*  1% */
----------------
dnovillo wrote:
> Could you add a comment describing these cutoffs and a general idea of what the numbers are and mean?
I've added a comment here. Let me know if you want anything more specific.

================
Comment at: lib/ProfileData/SampleProfReader.cpp:403
@@ -399,3 +402,3 @@
     return EC;
   else if (*Version != SPVersion())
     return sampleprof_error::unsupported_version;
----------------
dnovillo wrote:
> 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?
In the case of instrumented profile, summary is available only for indexed format. If we read an older version of the profile, the summary is computed after reading the profile.

There was only one test case that had a binary format directly checked in and I have fixed that test case. 


http://reviews.llvm.org/D17178