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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:730
   case SecFuncOffsetTable:
-    FuncOffsetsOrdered = hasSecFlag(Entry, SecFuncOffsetFlags::SecFlagOrdered);
-    if (std::error_code EC = readFuncOffsetTable())
-      return EC;
+    // If module is absent, we are using llvm-profdata tool, and need to read
+    // all profiles, so skip reading the function offset table.
----------------
> we are using llvm-profdata tool

nit: as library code/comment, make no assumption of the specific tool as user. just say tools as opposed to compiler. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:768
+  // (e.g. [A, A:1 @ B, A:1 @ B:2.3 @ C] [D, D:1 @ E]), so that when a match
+  // in the module is found, all prefixes of the matched function can be
+  // loaded, a list is needed to preserve that order.
----------------
> all prefixes of the matched function 

You meant "all function profile with matching prefix/caller"?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:773-776
+  // If the profile is MD5, use the map container to lookup functions in
+  // the module. A remapper has no use on MD5 names.
+  if (useMD5())
+    return false;
----------------
Do we need a separate entry for this? Or merge it with the `return false` in the end?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149124