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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:266
+// String based context id
+struct StringBasedCtxKey : public ContextKey {
+  std::string Context;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584