[PATCH] D148868: [llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring

[William Junda Huang via Phabricator via llvm-commits]

huangjd marked an inline comment as done.
huangjd added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:500
+  /// are present.
+  virtual void setProfileUseMD5() { ProfileIsMD5 = true; }
 
----------------
davidxl wrote:
> Is there a need to make it virtual?  Even though text format does not support it, it is harmless to keep the method (it does nothing).
Yes this is needed (for now, until MD5 refactoring patch is in place). If this is set, the context uses MD5 as function name, and for the case with Binary format or ExtBinary with function names, these names are converted to MD5 upon read. ProfileIsMD5 is referred by FunctionSamples::UseMD5, which is referred by IPO passes for profile matching. The transform pass needs to know if the profiles are **Actually** in MD5 because it behaves differently. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:538
+    assert(MD5NameMemStart);
+    auto FID = reinterpret_cast<const uint64_t*>(MD5NameMemStart)[*Idx];
+    SR = MD5StringBuf.emplace_back(std::to_string(FID));
----------------
davidxl wrote:
> Can you explain the cleanup done here?
This corresponds to 
```
Data = MD5NameMemStart + ((*Idx) * sizeof(uint64_t));
End = reinterpret_cast<const uint8_t *>(
        std::numeric_limits<uintptr_t>::max());
auto FID = readUnencodedNumber<uint64_t>();
if (std::error_code EC = FID.getError())
  return EC;```

However the check is not necessary because when reading the name table we already ensured the name table contains correct number of entries, and readStringIndex in this function ensures the index is in range, so the fixed length MD5 can be directly accessed with an index into the base address


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148868