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

[Lei Wang via Phabricator via llvm-commits]

wlei marked 3 inline comments as done.
wlei added a comment.

> 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.

Good question, Yes, my initial thought is to decouple this by using separated LBRSample when it comes to AutoFDO, perhaps this's good for readability. I guess your suggestion is for the performance since we won't have virtual call. That's good. If so and we don't have other kinds of perf sample, we might don't need the base class `PerfSample` and can just name HybridSample to more general name(like PerfSample).

> 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.

Yes, it's a vector of original call site address(Callstack). I was thinking whether we could use this to replace the string based context key in unwinder stage, later in ProfileGenerator convert to string. Just an idea. Will add more comments to the summary part.



================
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>;
+
----------------
wenlei wrote:
> 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)
Good point. Changed to `StringRef` and remove this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584