[PATCH] D68601: [SampleFDO] Add indexing for function profiles so they can be loaded on demand in ExtBinary format

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:554
+  /// Collect functions to be used when compiling Module \p M.
+  void collectFuncsToUse(const Module &M) override;
 };
----------------
davidxl wrote:
> Nit: collectFuncsFrom(const Module &M)
Fixed


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:507
+  for (auto &F : M) {
+    StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
+    FuncsToUse.insert(CanonName);
----------------
davidxl wrote:
> Skip declarations?
Fixed


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:533
+std::error_code SampleProfileReaderExtBinary::readFuncProfiles(uint64_t Size) {
+  const uint8_t *Start = Data;
+  if (UseAllFuncs) {
----------------
davidxl wrote:
> End = Data  + Size
It is set in the parent function: readOneSection.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:536
+    while (Data < Start + Size) {
+      if (std::error_code EC = readFuncProfile())
+        return EC;
----------------
davidxl wrote:
> It is more readable if readFuncProfile is taking  the pointer to the data address:
>      readFuncProfile(&Data);
Fixed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68601