[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 2 inline comments as done.
wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:266
+// String based context id
+struct StringBasedCtxKey : public ContextKey {
+  std::string Context;
----------------
wmi wrote:
> wlei wrote:
> > wmi wrote:
> > > Can you use Hashable as a base class instead so you can remove the isEqual virtual function (The CRTP pattern: https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern#Static_polymorphism)? It will also simplify the interface using Hashable. 
> > > 
> > > struct StringBasedCtxKey : public Hashable<StringBasedCtxKey> {
> > >  ...
> > > }
> > > 
> > > And other type of Key later on:
> > > struct ProbeBasedCtxKey : public Hashable<ProbeBasedCtxKey> {
> > >  ...
> > > }
> > Thanks for your suggestion and detailed example.
> > Here I want to put the different types derived from the same base class in the same container. 
> > like :
> > struct StringBasedCtxKey : public CtxKey {
> > ...
> > }
> > struct ProbeBasedCtxKey : public CtxKey {
> > ...
> > }
> > Then I can use only one container `unordered_map<CtxKey, ...> ContextSampleCounterMap `. If use CRTP, I guess it need two containers. any ideas about this?
> > 
> > 
> > 
> Ok, I don't have better idea. A possible solution is described in https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern#Pitfalls, but that prevents Hashable class from being reused by PerfSample without duplication. 
> 
> I am fine with your existing solution If no one else has better idea. 
Yeah, we want both `PerfSample` and `ContextKey` could implements a unified Hashable. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584