[PATCH] D92896: [CSSPGO][llvm-profgen] Virtual unwinding with pseudo probe
Lei Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 11 10:22:51 PST 2021
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
More information about the llvm-commits
mailing list