This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Add support for reading multiple versions of indexed profile format profile data
ClosedPublic

Authored by davidxl on Nov 30 2015, 10:21 PM.

Details

Summary

LLVM profile reader for indexed format is intended to be backward compatible -- a new version of compiler producing different format of indexed profile data should be able to read old version of indexed profile data produced by older versions of compilers. However currently it is hard to read different versions of profile data with different key types with the current reader implementation.

This patch solves the problem. To read a particular version of indexed profile is will be as simple as instantiating the index template class with the OnDiskHashTable implementation type associated with that version. There will be a follow up patch to templatize the InstrProfLookupTrait class (for different key types associated with the format).

Diff Detail

Event Timeline

davidxl updated this revision to Diff 41460.Nov 30 2015, 10:21 PM
davidxl retitled this revision from to [PGO] Add support for reading multiple versions of indexed profile format profile data.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Dec 1 2015, 11:06 AM

I like this approach. Some comments below --

include/llvm/ProfileData/InstrProfReader.h
263

nit, profile

281

It'd make for less typing if we use typename IndexType and keep std::unique_ptr<IndexType> Index where you have HashTable.

lib/ProfileData/InstrProfReader.cpp
538–544

nullptr?

542

I expected this to be an NFC commit, but this looks like breakage. InstrProfWriter uses IndexedInstrProf::Version (3).

davidxl marked 2 inline comments as done.Dec 1 2015, 11:32 AM
davidxl added inline comments.
include/llvm/ProfileData/InstrProfReader.h
281

Removed OnDisk prefix to shorten the typename.

lib/ProfileData/InstrProfReader.cpp
542

Good catch. Fixed.

This indicates a hole in the test coverage. I will add V2 and V3 format test cases in a follow up patch.

davidxl updated this revision to Diff 41547.Dec 1 2015, 11:43 AM
davidxl edited edge metadata.

Update patch with vsk's comments.

vsk added a comment.Dec 1 2015, 11:52 AM

Thanks. With one simplification this lgtm --

lib/ProfileData/InstrProfReader.cpp
542

The check on line 522 errors out with a diagnostic for this already. We can safely omit the conditional and say Index.reset(new InstrProfReaderIndex ...).

This revision was automatically updated to reflect the committed changes.