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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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();
----------------
wenlei wrote:
> 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. 
The sequential id was made intentionally in https://reviews.llvm.org/D100235 for the use of reporting zero samples towards non-executed probes in a frame. Using zero index caused all top-level inliners to share the same probe inline frame.

Even with the zero index, guid based hash lookup still won't work. Note that the guid part of the key of the Children map is the caller guid, which is the guid of the dummy root, which is zero. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118515