This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] Add symbol whitelist section to discriminate function being cold versus function being newly added
ClosedPublic

Authored by wmi on Aug 26 2019, 12:22 PM.

Details

Summary

This is the second half of https://reviews.llvm.org/D66374.

Symbol whitelist is the collection of function symbols showing up in the binary which generates the current profile. It is used to discriminate function being cold versus function being newly added. Symbol whitelist is only added for profile with ExtBinary format.

During profile use compilation, when profile-sample-accurate is enabled, a function without profile will be regarded as cold only when it is contained in that list.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Aug 26 2019, 12:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 12:22 PM
Herald added a subscriber: mgrang. · View Herald Transcript
davidxl added inline comments.Aug 27 2019, 11:32 AM
lib/ProfileData/SampleProf.cpp
233

Check zlib::isAvailable() -- if it returns false, do not do compression.

It is also better to add an internal option to disable compression.

test/Transforms/SampleProfile/symbol-white-list.ll
4 ↗(On Diff #217222)

The test would fail on platforms without zlib. Need to use REQUIRE to limit the test.

mgrang added inline comments.Aug 27 2019, 12:05 PM
lib/ProfileData/SampleProf.cpp
224

Please use the range-based llvm::sort here.

llvm::sort(SortedList);
247

Please use the range-based llvm::sort here.

llvm::sort(SortedList);
wmi updated this revision to Diff 217745.Aug 28 2019, 5:09 PM

Address David and Mandeep's comments.

Add symbol whitelist support in llvm-profdata. It supports reading in a text format symbols list and adding it to a profile. The output profile has to be ExtBinary format. llvm-profdata can control whether the symbol whitelist will be compressed or not, and whether it will be shown when dumping the profile.

With llvm-profdata, profile with symbol-whitelist used for testing can be generated given an existing text format profile and a text symbol list.

Suggesting on the naming: instead of using SymbolWhiteList, use 'ProfileSymbolList', or just 'SymbolList'. Otherwise the patch looks ok

lib/Transforms/IPO/SampleProfile.cpp
1736–1738

mention that symbol white list include all symbols in sampled binary,

wmi updated this revision to Diff 218166.Aug 30 2019, 2:15 PM

Address David's comment.

wmi marked an inline comment as done.Aug 30 2019, 2:17 PM

Suggesting on the naming: instead of using SymbolWhiteList, use 'ProfileSymbolList', or just 'SymbolList'. Otherwise the patch looks ok

Choose to use ProfileSymbolList.

lib/Transforms/IPO/SampleProfile.cpp
1736–1738

Done.

davidxl accepted this revision.Aug 30 2019, 2:21 PM

lgtm (please consolidate two test cases by factoring the common source code out and have two wrapper tests referencing it).

This revision is now accepted and ready to land.Aug 30 2019, 2:21 PM
wmi updated this revision to Diff 218178.Aug 30 2019, 3:42 PM

Extract the common part of compressed-profile-symbol-list.ll and uncompressed-profile-symbol-list.ll to a separate .ll file under Inputs directory.

wmi added a comment.Aug 30 2019, 3:42 PM

lgtm (please consolidate two test cases by factoring the common source code out and have two wrapper tests referencing it).

Good point. Done.

This revision was automatically updated to reflect the committed changes.