Updates the profile format to support inline stacks, which provides context-senstive information than flat profile.
Diff Detail
Event Timeline
include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
90 | Perhaps make CallsiteLocation inherit from LineLocation? Or embed it. I think inheriting from it may make better sense. | |
200 | Move this typedef below. No need to forward declare FunctionSamples. | |
245 | Too similar to samplesAt(). Merge them? | |
260 | Likewise with functionSamplesAt. Why not allow functionSamplesAt to return an empty FunctionSamples instance? Seems better than dealing with potential segfaults because of the nullptrs. To avoid unnecessary insertions, we can use ::find() in functionSamplesAt(). Similarly with samplesAt(). Having two functions with almost the same semantics is confusing. | |
lib/ProfileData/SampleProfReader.cpp | ||
37 | Add a short explanation of what the 'inline stack' is. Also, the user documentation in tools/clang/docs/UsersManual.rst will need to be updated. | |
212 | Can we split this in two changes? The new parsing code is not completely related to the inline stack functionality. While I agree that not using REs may be faster, it's not really something we care about. Could you modify it so the switch out of RE-based parsing is done in a later patch? | |
lib/Transforms/IPO/SampleProfile.cpp | ||
301 | s/NULL/nullptr/ This happens in several places. | |
368 | Isn't it sufficient to mark these functions as hot or always_inline? |
Integrate comments.
include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
200 | You mean move typedef inside the class? But the type is global and other modules actually uses this type. | |
245 | This is a const member function, while samplesAt can modify member variables. Not sure how to merge them? | |
260 | Save as above. Want to make the function as const member function. I will have a separate patch to remove samplesAt sampleRecordAt and functionSamplesAt entirely, leaving only const member funcntions. | |
lib/ProfileData/SampleProfReader.cpp | ||
37 | Updated the document in this file. Will have a separate patch to update the clang doc bits. | |
212 | That is what I initially tried to do. But I need to write new REs for the new profile format (with hierarchical profile format), spent 1+ hour designing REs, and finally gave up. So if we are gonna abandon the RE, then it does not make sense to spend effort to invent new RE for the new format. | |
lib/Transforms/IPO/SampleProfile.cpp | ||
368 | No, we actually need to have inlining done before annotation. |
include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
200 | I meant moving it after FunctionSamples's definition, but I now see that it makes no sense. This map is used inside FunctionSamples. Ignore me :) | |
260 | Ah, good. Yeah, your plan makes perfect sense. | |
lib/ProfileData/SampleProfReader.cpp | ||
37 | SGTM. | |
41 | s/tip/top/ | |
43 | r/symol in/symbol to/ | |
212 | Ah, OK. Yeah, REs are sometimes annoying to define. No worries then. We can refine this later. I've no problems with the code as you wrote it. | |
lib/Transforms/IPO/SampleProfile.cpp | ||
368 | Oh, yes, we discussed this before. The way propagation works now, we need to have the interesting inline decisions applied to align the block weights properly. I think we will want to revisit this at some point. It would be nice if we can avoid doing the inliner's job here and still deal with the weights properly in the propagator. Could you add a TODO here? Thanks. | |
test/Transforms/SampleProfile/Inputs/calls.prof | ||
8 | Could you leave the original comment in? Useful for documentation. | |
test/Transforms/SampleProfile/syntax.ll | ||
8 | Hm, not sure about losing this capability of tolerating badly mangled functions. I specifically added this because the optimized binaries sometimes don't have properly mangled symbols. The thinking was to be more tolerant of this. See the comment in SampleProfileReaderText::read(). If we are really going to not tolerate this anymore, could you please make it part of a different patch? This will break internal tests. |
Integrate comments.
test/Transforms/SampleProfile/Inputs/calls.prof | ||
---|---|---|
8 | The new format does not take comments in profile. Do you think we want to support it? |
LGTM with the support of '#' comments in the input text profile.
test/Transforms/SampleProfile/Inputs/calls.prof | ||
---|---|---|
8 | Yeah. It's easy enough to support and since the text format is used for tests, it's a useful feature to have. |
Perhaps make CallsiteLocation inherit from LineLocation? Or embed it. I think inheriting from it may make better sense.