[PATCH] D118515: [llvm-profgen] On-demand track optimized-away inlinees for preinliner.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:351
+  if (UsePseudoProbes && TrackFuncContextSize) {
+    for (const auto &Child : ProbeDecoder.getDummyInlineRoot().getChildren()) {
+      auto *Frame = Child.second.get();
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > Actually ProbeDecoder.getDummyInlineRoot().getChildren() is already a map, wondering can we make it possible to look up by function name without building an extra map? 
> > > > 
> > > > I can see that the key is Guid, ProbeId pair - we can get Guid from names, but ProbeId under dummy root is index. What was the reason for top level nodes to have different probe Id instead of a dummy probe Id? For top level, we don't expect same name to appear more than once. 
> > > Top level nodes should have a dummy probe id, i.e, 0. The key in ProbeDecoder.getDummyInlineRoot().getChildren()  is the guid. and we need a guid to look up a node in the children map. Unfortunately, there isn't a func name to guid map available currently. What we have is the `GUID2FuncDescMap`.  So I'm building a name to top level node map. Alternatively we can build a name to guid map here but that'll incur two hash lookups to get the node. Since either map is only used by llvm-profgen, I'm not placing the map to `MCPseudoProbe` which is also shared by the Bolt probe decorder.
> > > Unfortunately, there isn't a func name to guid map available currently
> > 
> > I thought function guid can be computed from names? 
> > 
> > > Top level nodes should have a dummy probe id, i.e, 0 
> > 
> > if i read the code correctly in `MCPseudoProbeDecoder::buildAddress2ProbeMap`, that's not the case:
> > 
> > ```
> > while (Data < End) {
> >     if (Root == Cur) {
> >       // Use a sequential id for top level inliner.
> >       Index = Root->getChildren().size();
> >     }
> > ```
> > 
> > 
> Ah, you are right on both. Now I completely remember why using guid based lookup didn't work. The key of the children map is <caller's guid, callsite probe id> and I was using callee's guid to lookup.
> 
> ```
> // A DFS-based decoding
>   while (Data < End) {
>     if (Root == Cur) {
>       // Use a sequential id for top level inliner.
>       Index = Root->getChildren().size();
>     } else {
>       ...
>     }
>     // Switch/add to a new tree node(inlinee)
>     Cur = Cur->getOrAddNode(std::make_tuple(Cur->Guid, Index));
> ```
> 
> We are now building callee name to callee node map here.
I guess it then goes back to my original question.. why the index for top level node needs to be non-zero? (I remember it was all zero initially, and then changed to use index). But there's no actual call site calling into top level frames, so the probe id isn't meaningful. If we have 0 probe id for them, name/guid would be sufficient for looking up the probe frame. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118515