[PATCH] D92584: [CSSPGO][llvm-profgen] Refactor to unify hashable interface for trace sample and context-sensitive counter

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for the refactoring and clean up, looks great!

LBRSample will be added when AutoFDO support is moved into llvm-profgen, right? For AutoFDO, we could use the same infrastructure except that context will always be empty.

What is CallsiteBasedCtxKey in the description? A stack of call site addresses? Brief comment for each of the leaf class that was mentioned in the description (even if not implemented) would be useful.



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:216
+// This is due to a build failure on sanitizer build(asan/msan/ubsan)
+using OrderedCounterForPrint = std::map<std::string, RangeSample>;
+
----------------
Does this map also needs to own a copy of the context string? 

nit: perhaps remove this comment below? determinism mentioned above is good enough. 

> This is due to a build failure on sanitizer build(asan/msan/ubsan)


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:172-175
+  uint64_t hashCombine(uint64_t Hash, uint64_t Value) {
+    // Simple DJB2 hash
+    return ((Hash << 5) + Hash) + Value;
+  }
----------------
nit: make `hashCombine` a lambda function inside `genHashCode` if there's no other use?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:194
+// the sample repeat times.
+using AggregationCounter =
+    std::unordered_map<Hashable<PerfSample>, uint64_t,
----------------
nit: rename it `AggregatedCounter`? we have `AggregatedSamples` as names. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584