This is an archive of the discontinued LLVM Phabricator instance.

Change sample profile format to hierarchical using inline stack.
ClosedPublic

Authored by danielcdh on Sep 24 2015, 1:47 PM.

Details

Reviewers
dnovillo
davidxl
Summary

Updates the profile format to support inline stacks, which provides context-senstive information than flat profile.

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 35671.Sep 24 2015, 1:47 PM
danielcdh retitled this revision from to Change sample profile format to hierarchical using inline stack..
danielcdh updated this object.
danielcdh added a reviewer: dnovillo.
danielcdh added a subscriber: davidxl.
danielcdh updated this revision to Diff 35673.Sep 24 2015, 2:28 PM

Update documentation of the profile format.

danielcdh updated this revision to Diff 35733.Sep 25 2015, 10:20 AM

clang-format and replace std::string with StringRef

danielcdh edited subscribers, added: llvm-commits; removed: davidxl.
dnovillo added inline comments.Sep 28 2015, 12:10 PM
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?

danielcdh updated this revision to Diff 35912.Sep 28 2015, 2:59 PM
danielcdh marked 2 inline comments as done.

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.

dnovillo added inline comments.Sep 29 2015, 8:09 AM
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.

danielcdh updated this revision to Diff 36007.Sep 29 2015, 10:58 AM
danielcdh marked 3 inline comments as done.

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?

dnovillo accepted this revision.Sep 29 2015, 11:04 AM
dnovillo edited edge metadata.

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.

This revision is now accepted and ready to land.Sep 29 2015, 11:04 AM
danielcdh closed this revision.Sep 29 2015, 6:41 PM

submitted