Page MenuHomePhabricator

[NFC][SampleFDO] Preparation to support multiple sections with the same type in ExtBinary format.
ClosedPublic

Authored by wmi on Dec 14 2020, 3:09 PM.

Details

Summary

Currently ExtBinary format doesn't support multiple sections with the same type in the profile. We add the support in this patch. Previously we use the section type to identify a section uniquely. Now we introduces a LayoutIndex in the SecHdrTable and use the LayoutIndex to locate the target section. The allocations of NameTable and FuncOffsetTable are adjusted accordingly.

Currently it works as a NFC because it won't change anything for current layout. The test for multiple sections support will be included in another patch where a new type of profile containing multiple sections with the same type is introduced.

Diff Detail

Event Timeline

wmi created this revision.Dec 14 2020, 3:09 PM
wmi requested review of this revision.Dec 14 2020, 3:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 3:09 PM
wmi retitled this revision from [NFC][SampleFDO] Support multiple sections with the same type in ExtBinary format. to [NFC][SampleFDO] Preparation to support multiple sections with the same type in ExtBinary format..Dec 14 2020, 3:18 PM
wmi edited the summary of this revision. (Show Details)
hoy added a comment.Dec 14 2020, 6:33 PM

Can you please shed light on how multiple same-typed sections will be used? Thanks.

llvm/lib/ProfileData/SampleProfWriter.cpp
269–273

How are the constant indices determined? I'm wondering if it's possible to automatically assign indices to avoid collision.

wmi added a comment.Dec 14 2020, 8:41 PM
In D93254#2453629, @hoy wrote:

Can you please shed light on how multiple same-typed sections will be used? Thanks.

Sorry for not providing enough background. It will be used for the profile we mentioned here: https://reviews.llvm.org/D89524#inline-834804. To reduce the profile loading time during postlink phase when ThinLTO is enabled, we are going to split profile into two parts. One part contains profiles with inline instance and another part contains flattened profiles without inline instance in them. The split profile will have a shared NameTable section, but separate LBRProfile section and FuncOffsetTable section for each part. In prelink, we will load all the sections in the profile while in postlink we will only load the part containing profiles with inline instance. That is because profile annotation in postlink can only get additional information not available in prelink from profiles with inline instance, and that could reduce the profile loading time for the large synthetic profile we use.

llvm/lib/ProfileData/SampleProfWriter.cpp
269–273

The indice order should match the order in the SampleProfileWriterExtBinary::SectionHdrLayout. I find the indice order in the patch is not up-to-date. I will update the patch.

To reduce the profile loading time during postlink phase when ThinLTO is enabled, we are going to split profile into two parts. One part contains profiles with inline instance and another part contains flattened profiles without inline instance in them.

For the flattened profile, do you explicitly flatten part of raw profile, or it's just the portion of function profile that does not have any inlinee profile?

that could reduce the profile loading time for the large synthetic profile we use.

Is that the partial profile that is a merged profile from a few workloads?

wmi added a comment.Dec 14 2020, 9:55 PM

To reduce the profile loading time during postlink phase when ThinLTO is enabled, we are going to split profile into two parts. One part contains profiles with inline instance and another part contains flattened profiles without inline instance in them.

For the flattened profile, do you explicitly flatten part of raw profile, or it's just the portion of function profile that does not have any inlinee profile?

We explicitly flatten part of raw profile. We only keep inlinee profile for hot part. Flattening is necessary for us to reduce the size of the merged profile.

that could reduce the profile loading time for the large synthetic profile we use.

Is that the partial profile that is a merged profile from a few workloads?

Yes, it is.

wmi updated this revision to Diff 312082.Dec 15 2020, 6:02 PM

The const indices passed to writeOneSection were incorrect (from a stale patch). Upload the latest version.

hoy added inline comments.Dec 15 2020, 11:19 PM
llvm/include/llvm/ProfileData/SampleProfWriter.h
255

Can this be made a field of SecHdrTableEntry?

llvm/lib/ProfileData/SampleProfWriter.cpp
269–273

I'm wondering what writing multiple same-typed sections will look like in future. Will writeOneSection take extra arguments? I'm thinking if we could still have a helper like getEntryInLayout to automatically retrieve layout index which may be helpful to adding new sections.

wmi added inline comments.Dec 16 2020, 12:05 PM
llvm/include/llvm/ProfileData/SampleProfWriter.h
255

Good point. Done.

llvm/lib/ProfileData/SampleProfWriter.cpp
269–273

Right, it will be perfect to do it automatically. However currently if the profile contains multiple same-typed sections, we have no way to tell them apart without an index. The index of each section is in the same order as the section order in the profile section header table, but may be different from the order in which writeOneSection is called. That brings some difficulty to automatically assign the index.

An example is: writeOneSection for FuncOffsetTable has to be called after writeOneSection for LBRProfile (We are populating FuncOffsetTable while LBRProfile section is written out), but FuncOffsetTable has to be read before LBRProfile so we can read the function profiles which will actually be used.

wmi updated this revision to Diff 312279.Dec 16 2020, 12:15 PM

Address Hongtao's comment.

hoy added inline comments.Dec 16 2020, 12:49 PM
llvm/lib/ProfileData/SampleProfWriter.cpp
117

May be worth adding a range check here to make sure LayoutIdx is not out-of-range?

269–273

I see. A pre-determined index is probably the easiest way to go. I see the assert in writeOneSection. Perhaps adding more checks there, like a range check, to make sure the index value passed in here is good?

269–273

Can you please also add a comment here on how the indices are determined?

wmi added inline comments.Dec 16 2020, 5:14 PM
llvm/lib/ProfileData/SampleProfWriter.cpp
117

Good point. Assertion added.

269–273

Added comment to describe how indices are determined.

269–273

assertion added.

wmi updated this revision to Diff 312343.Dec 16 2020, 5:16 PM

Address Hongtao's comment.

hoy accepted this revision.Dec 16 2020, 5:20 PM
This revision is now accepted and ready to land.Dec 16 2020, 5:20 PM

The new added field LayoutIndex miss to be initialized in include/llvm/ProfileData/SampleProfWriter.h:281 which cause the compiler(clang) warning if with option -Wmissing-field-initializers.
This is the error message:
error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]

The new added field LayoutIndex miss to be initialized in include/llvm/ProfileData/SampleProfWriter.h:281 which cause the compiler(clang) warning if with option -Wmissing-field-initializers.
This is the error message:
error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]

Yes, the bots are failing. Please fix or revert. http://lab.llvm.org:8011/#/builders/77/builds/2124

In file included from /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp:21:
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:281:33: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecProfSummary, 0, 0, 0},       {SecNameTable, 0, 0, 0},
                                ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:281:64: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecProfSummary, 0, 0, 0},       {SecNameTable, 0, 0, 0},
                                                               ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:282:37: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecFuncOffsetTable, 0, 0, 0},   {SecLBRProfile, 0, 0, 0},
                                    ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:282:65: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecFuncOffsetTable, 0, 0, 0},   {SecLBRProfile, 0, 0, 0},
                                                                ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:283:39: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecProfileSymbolList, 0, 0, 0}, {SecFuncMetadata, 0, 0, 0}};
                                      ^
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/llvm/include/llvm/ProfileData/SampleProfWriter.h:283:67: error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]
        {SecProfileSymbolList, 0, 0, 0}, {SecFuncMetadata, 0, 0, 0}};