[PATCH] D16005: Display detailed profile summary in llvm-profdata tool

[Easwaran Raman via llvm-commits]

eraman added inline comments.

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:45
@@ +44,3 @@
+// counts). The N in the above Top-N is the NumBlocks of the triplet and the
+// smallest count in the Top-N is the MinBlockCount.
+struct ProfileSummaryEntry {
----------------
vsk wrote:
> Nit: I have a hard time parsing this comment. Wdyt of omitting this paragraph and opting for shorter comments on the fields instead? I suggest;
> 
>   float Cutoff;  //< The execution count percentile, [0, 100].
>   uint64_t MinExecuteCount;  //< Minimum execution count for this percentile.
>   uint64_t NumBlocks; //< Number of blocks in this percentile.
> 
> Or something along those lines, I don't think my wording there is perfect :).
I agree that the comments I have leave much to be desired, but I am not sure the comments on the fields, in isolation, give a clear picture on what is being done. 

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:82
@@ +81,3 @@
+// value is a vector of (Cutoff, MinBlockCount, NumBlocks) triplets.
+std::vector<ProfileSummaryEntry>
+ProfileSummary::GetProfileSummary(std::vector<float> Cutoffs) {
----------------
davidxl wrote:
> Make this vector a member of the ProfileSummary class.
> 
> The use model (and in the future by InstrProfWriter ) should be like this:
> 
> ProfileSummary PS;
> 
> for each InstrProfRecord R:
>      PS.addCount(R);
> 
> PS.computeProfileSummary(cutoffs);
> 
> vector<..>& sums = PS.getProfSummary(..);
> ... dump
My problem with this approach is what this allows doing

PS.computeProfileSummary(cutoffs1);
PS.computeProfileSummary(cutoffs2);

PS.getProfSummary();

If you want to separate compute and get calls, I prefer passing the cutoffs in the constructor, provide a public get method which returns cached results if available and calls compute method if no cached data is available. This is what I have done now.

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:83
@@ +82,3 @@
+std::vector<ProfileSummaryEntry>
+ProfileSummary::GetProfileSummary(std::vector<float> Cutoffs) {
+  std::vector<ProfileSummaryEntry> Summary;
----------------
davidxl wrote:
> davidxl wrote:
> > getProfileSummary
> Use a scaled up integer to represent percentage.  Talk to Cong about introducing a common interface for that (similar to BranchProbability)
cl::opt<float> is used in a few places, so this is not new. I don't do much FP computation here. Letting the user pass a scaled up percentage value as an integer seems inconvenient to me (especially documenting how to use it). Alternatively, I can get the floats, scale it to an integer value and then use integer arithmetic elsewhere. Thoughts?

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:109
@@ +108,3 @@
+      if (Iter == End) {
+        Done = true;
+        break;
----------------
vsk wrote:
> You can avoid this construct by inverting the order of your loops, i.e iterate over the cutoffs then the histogram entries. Briefly;
> 
>   PSE NextEntry;
>   for (float Cutoff : Cutoffs)
>     NextEntry.Cutoff = Cutoff;
>     std::tie(NextEntry.MinExecuteCount, NextEntry.NumBlocks) = AccumulateCountsTo(Cutoff);
I don't see a good way of doing this without passing an iterator of the CountFrequencies map to the Accumulate method. Do you prefer that over the current implementation?


http://reviews.llvm.org/D16005