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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:354-360
+    if (Remapper) {
+      if (!Remapper->hasApplied())
+        Remapper->applyRemapping(Ctx);
+
+      if (auto FS = Remapper->getSamplesFor(Fname))
+        return FS;
+    }
----------------
wenlei wrote:
> If client calls `getSamplesFor` before reading profile, `RemappingApplied` would be set without doing anything. Then even if client later calls `read` followed by `getSamplesFor`, remapping will never be applied. Probably not a big deal though as it's kind of corner case. But alternatively we could eliminate that possibility by calling `applyRemapping` right after reading profile, e.g. have `read` from base class call something like `readImpl` and `applyRemapping`, and different readers only override `readImpl` instead of `read`.
Good point. Done. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1317
 
 /// Create a sample profile reader based on the format of the input data.
 ///
----------------
wenlei wrote:
> comment needs update for the new parameter (there're a few other places too).
Done. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68901/new/

https://reviews.llvm.org/D68901