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


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:76
+SampleCounter &
+VirtualUnwinder::getSampleCounter(const ProfiledBinary *Binary,
+                                  std::list<uint64_t> &CallStack) {
----------------
hoy wrote:
> Name it `getOrCreateSampleCounter`?
renamed


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:110
+  };
+  T &getData() { return *Data; }
+
----------------
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.



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





================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:301
+// Sample counter with context to support context-sensitive profile
+using ContextSampleCounter =
+    std::unordered_map<Hashable<ContextKey>, SampleCounter,
----------------
wmi wrote:
> Can we rename it to ContextSampleCounterMap and its object to something like CtxCounterMap? 
Good point, the value is actually a counter, renamed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92584