[PATCH] D68901: [SampleFDO] Add profile remapping support for profile on-demand loading used by ExtBinary format profile
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 14 11:30:10 PDT 2019
wenlei added inline comments.
================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:658
+ : SampleProfileReader(std::move(B), C, SPF_None),
+ Remappings(std::move(R)) {}
+
----------------
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.
================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:660
+
+ void setUnderlyingReader(std::unique_ptr<SampleProfileReader> Underlying) {
+ Format = Underlying->getFormat();
----------------
Check/assert clean state at the beginning? e.g. disallow setting/updating underlying reader repeatedly - was guaranteed when this was done in ctor.
================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1213-1214
std::error_code SampleProfileReaderItaniumRemapper::read() {
+ if (getFormat() == SPF_None)
+ llvm_unreachable("UnderlyingReader must be setup before remapper reads");
+
----------------
nit: assert(getFormat() != SPF_None && "UnderlyingReader must be setup before remapper reads")
================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1718-1720
+ if (RemapReader) {
+ RemapReader->setUnderlyingReader(std::move(Reader));
+ Reader = std::move(RemapReader);
----------------
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.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68901/new/
https://reviews.llvm.org/D68901
More information about the llvm-commits
mailing list