This is an archive of the discontinued LLVM Phabricator instance.

Display detailed profile summary in llvm-profdata tool
ClosedPublic

Authored by eraman on Jan 8 2016, 11:49 AM.

Details

Summary

This adds a detailed profile summary in llvm-profdata. The summary is in the form of one or more triples of the form (P, N, M) which is interpreted as if we look at the Top-N counts in the profile, their sum accounts for P percentage of the sum of all counts in the program and the minimum count in the Top-N is M.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 44353.Jan 8 2016, 11:49 AM
eraman retitled this revision from to Display detailed profile summary in llvm-profdata tool.
eraman updated this object.
eraman updated this object.Jan 8 2016, 12:30 PM
eraman added reviewers: vsk, AndyAyers, davidxl.
eraman added a subscriber: llvm-commits.
davidxl added inline comments.Jan 8 2016, 2:21 PM
tools/llvm-profdata/llvm-profdata.cpp
59 ↗(On Diff #44353)

Add simple summary data such as MaxFunctionCount and MaxBlockCount here.

63 ↗(On Diff #44353)

Make this a private method. Instead add a public method:

addCount(const InstrProfRecord &)

63 ↗(On Diff #44353)

AddCount --> addCount

82 ↗(On Diff #44353)

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

83 ↗(On Diff #44353)

getProfileSummary

83 ↗(On Diff #44353)

Use a scaled up integer to represent percentage. Talk to Cong about introducing a common interface for that (similar to BranchProbability)

91 ↗(On Diff #44353)

use a different name for the local variable (conflict with member variable)

360 ↗(On Diff #44353)

This should be moved to ProfileSummary

362 ↗(On Diff #44353)

PS.addCount(Func);

364 ↗(On Diff #44353)

Remove this

vsk edited edge metadata.Jan 8 2016, 5:10 PM

Thanks eraman! Comments inline --

tools/llvm-profdata/llvm-profdata.cpp
45 ↗(On Diff #44353)

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

74 ↗(On Diff #44353)

I believe this entire conditional can be safely replaced with ++CountFrequencies[Count];.

109 ↗(On Diff #44353)

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);
eraman updated this revision to Diff 44577.Jan 11 2016, 3:57 PM
eraman edited edge metadata.
eraman marked 8 inline comments as done.

Addressed reviewers' comments

tools/llvm-profdata/llvm-profdata.cpp
45 ↗(On Diff #44353)

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.

82 ↗(On Diff #44353)

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.

83 ↗(On Diff #44353)

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?

109 ↗(On Diff #44353)

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?

davidxl added inline comments.Jan 11 2016, 4:16 PM
tools/llvm-profdata/llvm-profdata.cpp
82 ↗(On Diff #44577)

This is reasonable -- the benefit of allowing computing summary of different cutoffs with the same PS is minimal.

83 ↗(On Diff #44577)

so -- for option part, using float is fine. What I am concerned is PorgramSummaryEntry has a float member. Eventually, I will need to write those entries to Indexed Profile header, so an integer representation there is better.

109 ↗(On Diff #44577)

current accumuated count can be passed to the Accumator methods. I think vsk's suggestion makes the code slightly more readable.

vsk added inline comments.Jan 11 2016, 5:30 PM
tools/llvm-profdata/llvm-profdata.cpp
45 ↗(On Diff #44577)

Fair enough. Maybe comments on the fields could help trim down the long-form comment?

109 ↗(On Diff #44577)

You could use a lambda to capture the map iterator, or go with David's suggestion. I imagine both would be clean. I'm mostly concerned about avoiding the 'double-break' pattern, and would prefer less direct manipulation of iterator objects.

eraman updated this revision to Diff 44670.Jan 12 2016, 1:50 PM
eraman marked 2 inline comments as done.

Address review comments.

tools/llvm-profdata/llvm-profdata.cpp
83 ↗(On Diff #44577)

Ok, I've changed even the options to use ints. Main issue is whether the help message makes it clear what the numbers mean.

eraman marked an inline comment as done.Jan 12 2016, 1:52 PM
eraman added inline comments.
tools/llvm-profdata/llvm-profdata.cpp
45 ↗(On Diff #44670)

Let me know if this looks better.

davidxl added inline comments.Jan 12 2016, 2:10 PM
test/tools/llvm-profdata/general.proftext
66 ↗(On Diff #44670)

The default cutoffs only have 3 percentiles, how come the dump shows more?

78 ↗(On Diff #44670)

Add a test with more than one explicit cutoffs?

tools/llvm-profdata/llvm-profdata.cpp
132 ↗(On Diff #44670)

Return vector<..>&.

488 ↗(On Diff #44670)

the scale is defined to be 1000000

eraman updated this revision to Diff 44681.Jan 12 2016, 2:54 PM
eraman marked an inline comment as done.
eraman updated this revision to Diff 44685.Jan 12 2016, 3:10 PM
eraman marked an inline comment as done.
eraman added inline comments.
test/tools/llvm-profdata/general.proftext
66 ↗(On Diff #44681)

Those are not the default values. That's the value description string shown in the help message.

78 ↗(On Diff #44681)

Good point. It helped me identify a bug - I've fixed that and added the test case.

davidxl accepted this revision.Jan 12 2016, 3:14 PM
davidxl edited edge metadata.

LTGM (after making getDetailedSummary() method return reference).

What is the plan to add similar support for Sample FDO?

This revision is now accepted and ready to land.Jan 12 2016, 3:14 PM

LTGM (after making getDetailedSummary() method return reference).

I had addressed it in my last revision.

What is the plan to add similar support for Sample FDO?

I assume we want to treat line numbers as blocks for this purpose. I'll work on it.

Vedant, does this look good to you?

vsk added a comment.Jan 13 2016, 12:46 PM

Lgtm, thanks.

tools/llvm-profdata/llvm-profdata.cpp
45 ↗(On Diff #44670)

Looks great!

This revision was automatically updated to reflect the committed changes.