This is an archive of the discontinued LLVM Phabricator instance.

[AutoFDO][llvm-profgen] Profile generation for LBR(non-CS) sample
ClosedPublic

Authored by wlei on Sep 9 2021, 3:34 PM.

Details

Summary

This patch introduces non-CS AutoFDO profile generation into LLVM. The profile is supposed to be well consumed by compiler using -fprofile-sample-use=[profile].

After range and branch counters are extracted from the LBR sample, here we go through each addresses for symbolization, create FunctionSamples and populate its sub fields like TotalSamples, BodySamples and HeadSamples etc. For inlined code, as we need to map back to original code, so we always add body samples to the leaf frame's function sample.

Diff Detail

Event Timeline

wlei created this revision.Sep 9 2021, 3:34 PM
wlei requested review of this revision.Sep 9 2021, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 3:34 PM
wlei retitled this revision from [llvm-profgen] Profile generation for LBR(non-CS) sample to [AutoFDO][llvm-profgen] Profile generation for LBR(non-CS) sample.Sep 9 2021, 4:10 PM
wlei edited the summary of this revision. (Show Details)
wlei added reviewers: wenlei, hoy, wmi.
hoy added inline comments.Sep 10 2021, 11:22 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
250

nit: use setContext. It's also consistent with getTopLevelFunctionProfile.

277

This is covered by the following while loop?

305

I just noticed that this call may result in empty entries populated in FuncStartAddrMap. Maybe refactor it to use find?

StringRef getFuncFromStartOffset(uint64_t Offset) {
   return FuncStartAddrMap[Offset];
}
wlei updated this revision to Diff 372022.Sep 10 2021, 3:29 PM
wlei marked an inline comment as done.

addressing Hongtao's feedback

llvm/tools/llvm-profgen/ProfileGenerator.cpp
250

Done!

277

Good catch, also remove the one in CSProfileGenerator.

305

Good catch!

hoy added inline comments.Sep 10 2021, 5:28 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
294

may still be good to keep the comment.

llvm/tools/llvm-profgen/ProfileGenerator.h
25–28

Since we are now using this base class to generate non-CS non-probe profile, add a header comment to clarify that?

64

Make these new functions private since they are not supposed to be called by the derived classes?

68

Nit: a helper function ...

wlei updated this revision to Diff 372291.Sep 13 2021, 10:11 AM

addressing Hongtao's feedback

hoy accepted this revision.Sep 13 2021, 10:25 AM

lgtm

This revision is now accepted and ready to land.Sep 13 2021, 10:25 AM
wenlei added inline comments.Sep 21 2021, 12:49 PM
llvm/test/tools/llvm-profgen/inline-noprobe.test
11

Add a test case to cover call site profile for inlinees?

llvm/test/tools/llvm-profgen/noinline-noprobe.test
12

We may want to strengthen the tests a bit since breaking llvm-profgen would be quite impactful for AutoFDO performance.

  • Add a test to cover call site profile with multiple targets?
  • Test discriminator: we have correct base discriminator as well as duplication factor in the generated profile.
  • Different profile format, extended binary format has profiled symbol list (it's ok to deal with symbol list later and separately)
  • Cover these cases for nested inlinee profile too.
llvm/tools/llvm-profgen/ProfileGenerator.cpp
317

move this assertion to the top?

wenlei added inline comments.Sep 22 2021, 6:05 PM
llvm/tools/llvm-profgen/ProfileGenerator.h
25–28

I am thinking about whether profile generation should follow the structure of perf reader. It looks like there's not much overlap for CS and non-CS profile as the profile format is different -pretty much updateBodySamplesforFunctionProfile is the only function from the base that is being reused? If that's the case, would it be better to have a separate ProfileGenerator base as interface then two subclass, one for CS one for non-CS?

49–50

I was initially a bit confused about why these are not virtual since CSProfileGenerator defines them too. Then I found they have different params (still confused why).. Now I understand here it's for all functions, but in CSProfileGenerator it's for one function. Perhaps we can name them populateBodySamplesForFunction vs populateBodySamplesForAllFunctions? Same for populateFunctionBoundarySamples?

wlei added inline comments.Sep 24 2021, 9:17 AM
llvm/test/tools/llvm-profgen/noinline-noprobe.test
12

added a large test case (quick_sort), it will cover the call site with multiple targets and nested inlinee profile.

Test discriminator: we have correct base discriminator as well as duplication factor in the generated profile

I had a standalone test case for discriminator test , see https://reviews.llvm.org/D109934

Different profile format, extended binary format has profiled symbol list (it's ok to deal with symbol list later and separately)

Sounds good, will add it shortly.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
317

fixed.

llvm/tools/llvm-profgen/ProfileGenerator.h
25–28

Good point, four sub-class is really harder to maintain and extend, refactored.

49–50

Sounds good!

wlei updated this revision to Diff 374873.Sep 24 2021, 9:19 AM

Addressing feedback:

  1. add a stonger test case through a quick sort program
  2. refactor ProfileGenerator breaking into two subclass: CS and non-CS

Thanks for refactoring, looks good. and we can deal with other format and profiled symbol list later in separate patch. Two more comments inline.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
220

This might be a behavior change for AutoFDO. I did see negative offsets in the past - if the code is from macro expansion which is defined before the function start. We would not have any profile if we filter them out.

llvm/tools/llvm-profgen/ProfileGenerator.h
34

why do we need this?

I just looked through features we have in internal tool, here's a list of things we need in addition to the basic support here and duplication factor. We can port them in later patches.

  • profiled symbol list as mentioned.
  • coroutine support to attribute sample on coroutine funclets back to main function.
  • split function support to handle profiling on BOLT binaries.
  • heuristic to compute and report profile density and make profiling suggestion.

cc @wmi @davidxl @xur hope this will soon be the unified tool for sample PGO profile generation including FS-AFDO too. We plan to switch to llvm-profgen for AutoFDO production use when it's ready.

wlei added a comment.Sep 24 2021, 1:14 PM

I just looked through features we have in internal tool, here's a list of things we need in addition to the basic support here and duplication factor. We can port them in later patches.

  • profiled symbol list as mentioned.
  • coroutine support to attribute sample on coroutine funclets back to main function.
  • split function support to handle profiling on BOLT binaries.
  • heuristic to compute and report profile density and make profiling suggestion.

Patches are on the way, thanks for the summary.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
220

The invalid number will be fixed in https://reviews.llvm.org/D110081. As we discuss offline, we might have the invalid case in non-macro case.

llvm/tools/llvm-profgen/ProfileGenerator.h
34

We need a virtual destructor for the inheritance class.

wenlei accepted this revision.Sep 24 2021, 1:19 PM

lgtm, thanks.

This revision was landed with ongoing or failed builds.Sep 24 2021, 1:56 PM
This revision was automatically updated to reflect the committed changes.