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

[Wenlei He via Phabricator via llvm-commits]

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