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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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;
----------------
wlei wrote:
> wlei wrote:
> > hoy wrote:
> > > `for (auto Address : reverse(CallStack))   ...` 
> > seems it's a bug, it should not be reversed, let me fix.
> it's not a bug, the call stack is leaf to root order, should be reversed.
> But here note that I need to skip the leaf frame. so the code will be like:
> ```
> bool IsLeaf = true;
> for (auto Address : reverse(CallStack)) {
>     if (IsLeaf) {
>         IsLeaf = false;
>         continue;
>    }
>    ...
> }
> ```
> 
> Btw, I didn't use reverse like this way before, so it will act as a iterator wrapper, won't reverse the vector's inside elements, right?
Yes, see llvm/include/llvm/ADT/STLExtras.h


```
// Returns an iterator_range over the given container which iterates in reverse.
// Note that the container must have rbegin()/rend() methods for this to work.
template <typename ContainerTy>
auto reverse(ContainerTy &&C,
             std::enable_if_t<has_rbegin<ContainerTy>::value> * = nullptr) {
  return make_range(C.rbegin(), C.rend());
}
```



================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:288
+  SmallVector<const PseudoProbe *, 16> Probes;
+
+  ProbeBasedCtxKey() : ContextKey(CK_ProbeBased) {}
----------------
wlei wrote:
> 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
> ```
> 
> 
I see. Yeah, compression should be done on vectors. Will the string form computed only once after compression is done? I was wondering if you ever need the string more than once, making a field and fill it out after compression is probably good. But if the sting is only needed once, what you have now is good enough.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896