Page MenuHomePhabricator

[SampleFDO] Add profile remapping support for profile on-demand loading used by ExtBinary format profile
ClosedPublic

Authored by wmi on Oct 11 2019, 5:07 PM.

Details

Summary

profile on-demand loading was added for ExtBinary format profile in https://reviews.llvm.org/rL374233, but currently profile on-demand loading doesn't work well with profile remapping. The patch adds the support.

Suppose a function in the current module has outline instance in the profile. The function name in the module is different from the name of the outline instance, but remapper knows the two names are equal. When loading profile on-demand, the outline instance has to be loaded with remapper's help.

Before the patch, the steps to read the profile is as follows:

  • create the profile reader
  • profile reader read the profile.
  • create the profile remapper
  • remapper set the underlying reader.
  • reset the profile reader to remapper.

With the patch, the steps to read the profile is changed to:

  • create the profile reader
  • create the profile remapper
  • profile reader set the underlying remapper.
  • profile reader read the profile.
  • remapper set the underlying reader.
  • reset the profile reader to remapper.

Diff Detail

Event Timeline

wmi created this revision.Oct 11 2019, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 5:07 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
wenlei added inline comments.Oct 14 2019, 11:27 AM
llvm/include/llvm/ProfileData/SampleProfReader.h
723

while turning this Remappings object into unique_ptr decouples things a bit and enables moving error handling of reading remapping file earlier, there's now one more thing to check, Remappings could be null.

725

Check/assert clean state at the beginning? e.g. disallow setting/updating underlying reader repeatedly - was guaranteed when this was done in ctor.

llvm/lib/ProfileData/SampleProfReader.cpp
1212–1213

nit: assert(getFormat() != SPF_None && "UnderlyingReader must be setup before remapper reads")

llvm/lib/Transforms/IPO/SampleProfile.cpp
1700–1701

Wondering if we can move this up to line 1705, by forwarding SampleProfileReaderItaniumRemapper's read(), getProfileSymbolList(), etc. to the underlying reader.

If we can do that, we would be able to avoid the interleaved structure of setting up RemapReader, Reader, and let client of Reader to call read(), etc., then swap the two readers. Without that interleaved structure, we could hide this complexity inside SampleProfileReader::create by letting it take profile file name, as well as remapping file name, and return SampleProfileReaderItaniumRemapper (proxy) or a regular reader depending on whether remapping file is used.

davidxl added inline comments.Oct 15 2019, 10:31 AM
llvm/lib/Transforms/IPO/SampleProfile.cpp
1691

Better to move line 1705 together with block 1678 to 1693 and make block a helper function 'createRemapper' which does the following

  1. create the remapper
  2. set reader for the remapper
  3. set remapper for the reader.

The call to createRemapper can be moved to line 1705.

wmi added a comment.Oct 15 2019, 11:45 AM

Thanks Wenlei and David for the reviews.

It is a good suggestion to try to make remapper and reader less interleaving. That leads me to find a more fundamental problem. Current mechanism to make SampleProfileReaderItaniumRemapper a proxy of underlying reader is easy to have bugs. Currently, some internal data structures like Profiles and NameTables of underlyingReader have been transfered to SampleProfileReaderItaniumRemapper in order for SampleProfileReaderItaniumRemapper to function as a normal reader. However, for every virtual function in SampleProfileReader, we need to have an override function in SampleProfileReaderItaniumRemapper and delegate the function to the underlying reader, otherwise, it will call the wrong version in SampleProfileReader. It is confusing and it is easy to have bugs -- the underlying reader is ExtBinary format but SampleProfileReaderItaniumRemapper behaves like a SampleProfileReader.

So I plan to make SampleProfileReaderItaniumRemapper a helper inside SampleProfileReader instead of a proxy. From what I already saw that will simplify the code a lot. Will update the patch when it is done.

wmi updated this revision to Diff 225161.Oct 15 2019, 10:34 PM

Address Wenlei and David's comments.
Change SampleProfileReaderItaniumRemapper from a proxy of SampleProfileReader to a helper object inside of SampleProfileReader, and it is used in two places: one in SampleProfileReader::getSamplesFor for looking up a function sample and one in SampleProfileReaderExtBinary::readFuncProfiles for loading profiles on demand.

wenlei added inline comments.Oct 16 2019, 10:44 AM
llvm/include/llvm/ProfileData/SampleProfReader.h
363–369

If client calls getSamplesFor before reading profile, RemappingApplied would be set without doing anything. Then even if client later calls read followed by getSamplesFor, remapping will never be applied. Probably not a big deal though as it's kind of corner case. But alternatively we could eliminate that possibility by calling applyRemapping right after reading profile, e.g. have read from base class call something like readImpl and applyRemapping, and different readers only override readImpl instead of read.

llvm/lib/ProfileData/SampleProfReader.cpp
1323

comment needs update for the new parameter (there're a few other places too).

wmi updated this revision to Diff 225318.Oct 16 2019, 3:28 PM
wmi marked 2 inline comments as done.

Address Wenlei's comment.

llvm/include/llvm/ProfileData/SampleProfReader.h
363–369

Good point. Done.

llvm/lib/ProfileData/SampleProfReader.cpp
1323

Done.

wenlei accepted this revision.Oct 16 2019, 7:03 PM

Thanks for making the changes, LGTM!

This revision is now accepted and ready to land.Oct 16 2019, 7:03 PM
This revision was automatically updated to reflect the committed changes.