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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
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? |
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. |
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? |
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. |
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. |
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? |
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. |
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. |