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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/Inputs/inline-cs-pseudoprobe.perfscript:1
+Using perf wrapper that supports hot-text. Try perf.real if you encounter any issues.
+PERF_RECORD_MMAP2 595196/595196: [0x201000(0x1000) @ 0 00:1d 224227621 1042948]: r-xp /home/inline-cs-pseudoprobe.perfbin
----------------
Please remove this line here and in other `.perfscript` files.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:112
+    auto EndT = std::prev(CallStack.rend());
+    for (; Iter != EndT; Iter++) {
+      uint64_t Address = *Iter;
----------------
`for (auto Address : reverse(CallStack))   ...` 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:275
 
-static void printRangeCounter(ContextSampleCounter &Counter) {
+static std::string getContextKeyStr(ContextKey *K,
+                                    const ProfiledBinary *Binary) {
----------------
This can return `StringRef` when a `ProbeBasedCtxKey` holds a string data field.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:288
+  SmallVector<const PseudoProbe *, 16> Probes;
+
+  ProbeBasedCtxKey() : ContextKey(CK_ProbeBased) {}
----------------
How about make a string field here to avoid string copying like in `getContextKeyStr`? Also the pseudo probe related code in `getContextKeyStr` can be moved here as an `toString()` API.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:300
+    for (auto P : O->Probes) {
+      if (P != *Iter++)
+        return false;
----------------
use `std::equal`?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:312
+      // Avoid zero value of HashCode when it's an empty list
+      HashCode = 1;
+    }
----------------
assert `Probes` are empty here?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:111
 
+class PseudoProbeProfileGenerator : public CSProfileGenerator {
+
----------------
Name it `PseudoProbeCSProfileGenerator` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896