Page MenuHomePhabricator

[SampleFDO] add suffix elision control for fcn names
ClosedPublic

Authored by thanm on Mar 1 2019, 9:04 AM.

Details

Summary

Add hooks for determining the policy used to decide whether/how
to chop off symbol 'suffixes' when locating a given function
in a sample profile.

Prior to this change, any function symbols of the form "X.Y" were
elided/truncated into just "X" when looking up things in a sample
profile data file.

With this change, the policy on suffixes can be changed by adding a
new attribute "sample-profile-suffix-elision-policy" to the function:
this attribute can have the value "all" (the default), "selected", or
"none". A value of "all" preserves the previous behavior (chop off
everything after the first "." character, then treat that as the
symbol name). A value of "selected" chops off only the rightmost
".llvm.XXXX" suffix (where "XXX" is any string not containing a "."
char). A value of "none" indicates that names should be left as is.

Diff Detail

Repository
rL LLVM

Event Timeline

thanm created this revision.Mar 1 2019, 9:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 9:05 AM
mtrofin added inline comments.Mar 5 2019, 1:48 PM
include/llvm/ProfileData/SampleProf.h
415 ↗(On Diff #188926)

Nit: getCanonicalFunctionName

416 ↗(On Diff #188926)

Coding style (throughout): variable names start with capital letter, e.g. AttrName, Suffix, etc.

427 ↗(On Diff #188926)

please run clang-format (there should be spaces surrounding the '-' sign)

unittests/ProfileData/SampleProfTest.cpp
203 ↗(On Diff #188926)

addFunctionSamples

also, paremeters should be capitalized (SMap, FName, TotalSamples, HeadSamples)

228 ↗(On Diff #188926)

For clarity, please give this a more verbose name. If I understand it correctly, this is (for example) ensureSampleProfileSuffixElisionPolicy

thanm marked 6 inline comments as done.Mar 6 2019, 7:27 AM
thanm added inline comments.
unittests/ProfileData/SampleProfTest.cpp
228 ↗(On Diff #188926)

OK. I've renamed to createFunctionWithSampleProfileElisionPolicy

thanm updated this revision to Diff 189511.Mar 6 2019, 7:29 AM
thanm marked an inline comment as done.

Formatting and variable/function naming.

wmi added a comment.Mar 7 2019, 3:27 PM

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

thanm added a comment.Mar 8 2019, 12:50 PM
In D58832#1422205, @wmi wrote:

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

We want to be able to eventually support ThinLTO in which you have a mix of C and Go functions (it is fairly common for Go programs to call out into C code). Without such a change we'd have to pick one policy or another and hope that it works for code generated by every front end.

wmi added a comment.Mar 8 2019, 4:12 PM
In D58832#1422205, @wmi wrote:

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

We want to be able to eventually support ThinLTO in which you have a mix of C and Go functions (it is fairly common for Go programs to call out into C code). Without such a change we'd have to pick one policy or another and hope that it works for code generated by every front end.

That makes sense to me. Thanks for explaining.

wmi added inline comments.Mar 11 2019, 11:56 AM
include/llvm/ProfileData/SampleProf.h
420–429 ↗(On Diff #189511)

It may be better to use a suffix array here so that it is easier for others to add new suffix later.

433 ↗(On Diff #189511)

Add an error message here: assert(false && "xxx")

include/llvm/ProfileData/SampleProfReader.h
289–290 ↗(On Diff #189511)

Now that the function names are not necessarily all stripped, we want to update the comment accordingly.

thanm marked 4 inline comments as done.Mar 11 2019, 1:17 PM
thanm added inline comments.
include/llvm/ProfileData/SampleProfReader.h
289–290 ↗(On Diff #189511)

I've updated the comment.

thanm updated this revision to Diff 190148.Mar 11 2019, 1:27 PM
thanm marked an inline comment as done.

Additional changes suggested by Wei.

wmi accepted this revision.Mar 11 2019, 2:33 PM

LGTM. Thanks for working on it!

This revision is now accepted and ready to land.Mar 11 2019, 2:33 PM
thanm added a comment.Mar 14 2019, 6:14 AM

Thanks for reviewing.

This revision was automatically updated to reflect the committed changes.