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

[Hongtao Yu via Phabricator via llvm-commits]

hoy accepted this revision.
hoy added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:110
+  };
+  T &getData() { return *Data; }
+
----------------
wlei wrote:
> hoy wrote:
> > I don't see a consumer of the two APIs. Maybe exclude them from this patch? I'm wondering if the shared pointer should be returned (for reference counting) when there is a need of exposing the underlying data.
> Removed `getData()`, `getPtr()` is used when getting the key data, like PerfRead.cpp:232
> > if the shared pointer should be returned 
> Not sure `const shared_ptr<Key> *K = dyn_cast<shared_ptr<Key>>(I.first.getPtr());` can work for this, let me try.
> Also since it's the hashkey, it's only used like: 
> ```
> for(auto I : Map) {
>     const Key *K = dyn_cast<Key>(I.first.getPtr());
>     // use K
> }
> ```
> So `K` is always used inside of `Map`'s scope.
> 
Current `getPtr()` implementation and its use looks good. I was thinking `getPtr()` might leak the underlying data out of the shared pointer scope when destroyed.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:266
+// String based context id
+struct StringBasedCtxKey : public ContextKey {
+  std::string Context;
----------------
wlei wrote:
> 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.
Both patterns have their specialties. The current solution looks a bit cleaner though it falls back to use virtual methods to solve hash collision. Hopefully that could be mitigated by using a more efficient hash code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584