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

[Wei Mi via Phabricator via llvm-commits]

wmi 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
----------------
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.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:99
+                                            std::list<uint64_t> &CallStack) {
+  std::shared_ptr<ProbeBasedCtxKey> ProbeBasedKey =
+      std::make_shared<ProbeBasedCtxKey>();
----------------
Why it needs to be a shared_ptr?


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


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:135-137
+      State.getBinary()->usePseudoProbes()
+          ? getOrCreateCounterForProbe(State.getBinary(), State.CallStack)
+          : getOrCreateCounter(State.getBinary(), State.CallStack);
----------------
How about move the conditional statement 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());
----------------
Add an assertion for O != nullptr?


================
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
----------------
pseduo => pseudo


================
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 "
----------------
Nit: CallProbe == nullptr --> !CallProbe


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896