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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:112
+    auto EndT = std::prev(CallStack.rend());
+    for (; Iter != EndT; Iter++) {
+      uint64_t Address = *Iter;
----------------
hoy wrote:
> `for (auto Address : reverse(CallStack))   ...` 
seems it's a bug, it should not be reversed, let me fix.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:288
+  SmallVector<const PseudoProbe *, 16> Probes;
+
+  ProbeBasedCtxKey() : ContextKey(CK_ProbeBased) {}
----------------
hoy wrote:
> 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.
Good point. One question I got is we might not use string here directly due to context compression. The compression will implements on the vector of string/probe, but here we miss the top frame probe. So I just want to redesign to use string vector as the output of virtual unwinding. In ProfileGenerator, the top frame context string will be pushed and fed for the compression to generate the string. e.g:
```
[a:1, b:2, c:3]               virtual unwinding
[a:1, b:2, c:3, b2: d:4]   push back top frame context
[a:1, b:2, d:4]               compression
"a:1 @ b:2 @ d:4"       string joining
```




================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:312
+      // Avoid zero value of HashCode when it's an empty list
+      HashCode = 1;
+    }
----------------
hoy wrote:
> assert `Probes` are empty here?
Same as above, the Probes can be empty since it doesn't include the top frame probe.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:111
 
+class PseudoProbeProfileGenerator : public CSProfileGenerator {
+
----------------
hoy wrote:
> Name it `PseudoProbeCSProfileGenerator` ?
Changed. I see that in our internal tool, we use a switch variable to decide whether to gen CS profile. Here not sure whether we need to merge them in a general class or we should separate in different class. but I guess we can leave it when come to non-cs implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896