Page MenuHomePhabricator

Analysis pass to access profile summary info
ClosedPublic

Authored by eraman on May 25 2016, 3:50 PM.

Details

Summary

This analysis provides access to the profile summary information. This is a WIP and the interface will change as we have more clients using it. The immediate task is to get the inline cost analysis to use the isHotFunction/isColdFunction interfaces here and remove the MaxFunctionCount annotation at the module.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 58527.May 25 2016, 3:50 PM
eraman retitled this revision from to Analysis pass to access profile summary info.
eraman updated this object.
eraman added reviewers: silvas, vsk, davidxl.
eraman added a subscriber: llvm-commits.
davidxl added inline comments.May 26 2016, 10:40 AM
include/llvm/Analysis/ProfileSummaryInfo.h
85 ↗(On Diff #58527)

Are those ctors/ assignment operators needed? The result type needs the rvalue ctors for data moving.

92 ↗(On Diff #58527)

Return 'Result'

100 ↗(On Diff #58527)

Nit : ProfileSummaryPrinterPass?

lib/Analysis/ProfileSummaryInfo.cpp
45 ↗(On Diff #58527)

typo.

65 ↗(On Diff #58527)

Should define a enum : SUMMARY_hot, _cold, _unknown (or noprofile)

We probably also need another interface in the future to check body hotness.

79 ↗(On Diff #58527)

Merge with above and make it 'getFunctionHotness' ?

106 ↗(On Diff #58527)

Document with FIXME that the hotness determination here is different from function hotness logic above.

eraman marked 2 inline comments as done.May 26 2016, 2:40 PM
eraman added inline comments.
include/llvm/Analysis/ProfileSummaryInfo.h
85 ↗(On Diff #58527)

This boilerplate code is found in many places. For example, see AssumptionAnalysis which also doesn't have any data members has this. From comments elsewhere, not specifying these breaks in MSVC (search for MSVC under include/llvm/Analysis)

100 ↗(On Diff #58527)

I thought FunctionHotnessPrinterPass accurately reflects what this is doing. I have no strong opinion - will change it to ProfileSummaryPrinterPass if you prefer.

lib/Analysis/ProfileSummaryInfo.cpp
65 ↗(On Diff #58527)

I feel separate isHotX and isColdX is better. You anyway have to check the return value of the enum and the implementation has the combined code of isHotX and isColdX. But if you insist on unifying them, do you have any suggestion for "neither hot nor cold"? Unknown is not the right value.

106 ↗(On Diff #58527)

The fixme is in the is{Hot|Cold}Function since that is the one that needs to be fixed.

davidxl added inline comments.May 26 2016, 3:17 PM
include/llvm/Analysis/ProfileSummaryInfo.h
85 ↗(On Diff #58527)

ok.

lib/Analysis/ProfileSummaryInfo.cpp
65 ↗(On Diff #58527)

Ok -- document that when returns false, it either means it is not hot or we don't have info to tell.

106 ↗(On Diff #58527)

ok

eraman updated this revision to Diff 58829.May 27 2016, 1:19 PM
eraman marked an inline comment as done.

Address David's comments

davidxl accepted this revision.May 27 2016, 3:12 PM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.May 27 2016, 3:12 PM
vsk edited edge metadata.May 27 2016, 3:21 PM

Sorry for the delay. I have some preliminary comments, I'll take a closer look in a second.

lib/Analysis/ProfileSummaryInfo.cpp
34 ↗(On Diff #58829)

I'm a bit confused as to why this is higher than the percentile for hot functions. Sorry if this is a basic question! Could you explain this a bit?

44 ↗(On Diff #58829)

Why not a lower_bound? That should be faster, and it adds a bit of flexibility, I think.

47 ↗(On Diff #58829)

Could you add an error string to this assert?

davidxl added inline comments.May 27 2016, 3:24 PM
lib/Analysis/ProfileSummaryInfo.cpp
34 ↗(On Diff #58829)

Higher percentile means that more (colder) blocks are covered in the accumulative sum -- the higher the percentile, the colder the threshold.

vsk added a comment.May 27 2016, 3:41 PM

Ah ok, I think I understand the *CountPercentiles now. Just two more nits.

lib/Passes/PassBuilder.cpp
71 ↗(On Diff #58829)

This change should be left out of this patch.

test/Analysis/ProfileSummary/basic.ll
5 ↗(On Diff #58829)

It doesn't seem like the contents of these function bodies affect the test. Could you minimize them?

I'll address the rest of your comments and update the patch

lib/Analysis/ProfileSummaryInfo.cpp
44 ↗(On Diff #58829)

If we want the flexibility of not specifying an exact percentile, won't nearest entry (minimum difference) be better than lower_bound?

vsk added inline comments.May 27 2016, 3:52 PM
lib/Analysis/ProfileSummaryInfo.cpp
44 ↗(On Diff #58829)

ISTM that breaks the function's contract. It would no longer always return the minimum execution count for a given percentile: it could return something greater.

silvas accepted this revision.May 27 2016, 6:07 PM
silvas edited edge metadata.

This LGTM with some nits.

include/llvm/Analysis/ProfileSummaryInfo.h
100 ↗(On Diff #58527)

I think if we change ProfileSummaryAnalysis to have more predicates, we would want to extend this pass (e.g. we use it for testing), so ProfileSummaryPrinterPass seems a bit better (but it is just a nit).

lib/Analysis/ProfileSummaryInfo.cpp
29 ↗(On Diff #58527)

If the name is "percentile", I would expect it to be out of 100, but instead it is out of 1,000,000 which is confusing.
Since we require it to be one of the cutoffs in the profile summary, I would call it "profile-summary-cutoff" or similar.

34 ↗(On Diff #58527)

Sorry if I'm missing something obvious, but why is the cold percentile nearly 1,000,000? Wouldn't that mark almost everything as cold?
I.e. I would expect that ColdCountPercentile must be less than HotCountPercentile.

47 ↗(On Diff #58527)

This assertion can be triggered from the command line and is therefore not a correct assertion. Since this is not an option meant to be exposed directly to users (-mllvm is technical an "internal compiler flag"), I think that using report_fatal_error here is appropriate.

138 ↗(On Diff #58527)

You may want to mention that this isn't really a big deal since we are setting the function count directly in the test.

eraman marked 8 inline comments as done and an inline comment as not done.Jun 2 2016, 11:03 AM
eraman added inline comments.
lib/Analysis/ProfileSummaryInfo.cpp
44 ↗(On Diff #58829)

You're right. I've changed the code to use lower_bound

47 ↗(On Diff #58829)

I've changed to report_fatal_error as suggested by Sean

139 ↗(On Diff #58829)

Sorry, I don't understand this comment. Irrespective of how we set the function entry count, the issue is we are not testing the isHotCount and isColdCount code.

eraman updated this revision to Diff 59428.Jun 2 2016, 11:04 AM
eraman edited edge metadata.
eraman marked 2 inline comments as done.

Address comments and rebase

This revision was automatically updated to reflect the committed changes.