[PATCH] D92896: [CSSPGO][llvm-profgen] Virtual unwinding with pseudo probe

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/inline-cs-pseudoprobe.test:3
+
+; CHECK-UNWINDER: Binary(inline-cs-pseudoprobe.perfbin)'s Range Counter:
+; CHECK-UNWINDER:   (800, 858): 1
----------------
wmi wrote:
> Add CHECK for context similar as noinline-cs-pseudoprobe.test below? It is good to make sure we get the context right w/wo inline.
Yeah, this is because the context is empty. added the `CHECK-UNWINDER-EMPTY` for this.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:99
+                                            std::list<uint64_t> &CallStack) {
+  std::shared_ptr<ProbeBasedCtxKey> ProbeBasedKey =
+      std::make_shared<ProbeBasedCtxKey>();
----------------
wmi wrote:
> Why it needs to be a shared_ptr?
This is because the `Hashable` interface is using shared_ptr to allow being the key of unordered_map, I haven't found a workable way for unique_ptr yet, could we leave for later refining? I will keep an eye on this.



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:119
+      // not know how to consume a context with unknown callsites.
+      if (CallProbe == nullptr)
+        break;
----------------
wmi wrote:
> Nit: !CallProbe
fixed!


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:135-137
+      State.getBinary()->usePseudoProbes()
+          ? getOrCreateCounterForProbe(State.getBinary(), State.CallStack)
+          : getOrCreateCounter(State.getBinary(), State.CallStack);
----------------
wmi wrote:
> How about move the conditional statement into getOrCreateCounter?
Good suggestion, moved getOrCreateCounterForProbe into getOrCreateCounter


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:293
+  bool isEqual(const ContextKey *K) const override {
+    const ProbeBasedCtxKey *O = dyn_cast<ProbeBasedCtxKey>(K);
+    return std::equal(Probes.begin(), Probes.end(), O->Probes.begin());
----------------
wmi wrote:
> Add an assertion for O != nullptr?
assertion added


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:378
+                                    std::list<uint64_t> &CallStack);
+  // Use pseduo probe based context key to get the sample counter
+  // A context stands for a call path from 'main' to an uninlined
----------------
wmi wrote:
> pseduo => pseudo
fixed


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:306
+    if (Probe.isCall()) {
+      assert(CallProbe == nullptr &&
+             "There should be only one call probe corresponding to address "
----------------
wmi wrote:
> Nit: CallProbe == nullptr --> !CallProbe
fixed


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:322
+    auto It = GUID2FuncDescMap.find(Probe->GUID);
+    assert(It != GUID2FuncDescMap.end());
+    StringRef FuncName = It->second.FuncName;
----------------
wmi wrote:
> Add an assertion message.
assertion added


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896