[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/include/llvm/ProfileData/SampleProfReader.h:673
+  /// (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,
+  /// so the list container is needed.
----------------
> all prefixes of the matched function can be loaded, 

prefix is caller, but it's meant to read in all callees of matches functions. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:687
+  bool useOrderedFuncOffsets() {
+    return ProfileIsCS || (!useMD5() && Remapper);
+  }
----------------
For readability, i'd suggest expand the logic with inline comments for each category. The big chunk of comment above the function is harder to parse and more prone to be out dated. Something like this:

```
// category and reason
if ....
   return true;

// category and reason
if ...
   return true;

...

// category and reason
return false;
```


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:689
+  /// to determine a cutoff.
+  bool useOrderedFuncOffsets() {
+    return ProfileIsCS || (!useMD5() && Remapper);
----------------
hoy wrote:
> huangjd wrote:
> > davidxl wrote:
> > > does it need to check the section flag?
> > I added back the section flag to check if CS profile has ordered function offset table.  
> Does the function offset table for a remapper profile always come in sorted?
This function now controls whether list or map is used, which is orthogonal to whether it's ordered (still controlled by section flag). So `useOrderedFuncOffsets()` is more like `useFuncOffsetList()` (as opposed to map).

`useOrderedFuncOffsets` and `OrderedFuncOffsets` need to be renamed to remove "Ordered" because with this change there's no guarantee it's actually ordered. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149124