[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:
> 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());
> }
> ```
> 
As we discussed offline, we need to skip the top frame for this, so just keep using the iterator way. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:288
+  SmallVector<const PseudoProbe *, 16> Probes;
+
+  ProbeBasedCtxKey() : ContextKey(CK_ProbeBased) {}
----------------
hoy wrote:
> 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.
Yeah, after compression the string is only used once, so I will keep this.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:312
+      // Avoid zero value of HashCode when it's an empty list
+      HashCode = 1;
+    }
----------------
hoy wrote:
> 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.
Oh, I mean the probes doesn't include the top frame probe, so the probes will have the change to be empty.
See line 98, it has the assertion to check whether the `Hashcode` is 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92896