[PATCH] D108180: [CSSPGO] Track and use context-sensitive post-optimization function size to drive global pre-inliner in llvm-profgen

[Hongtao Yu via Phabricator via llvm-commits]

hoy accepted this revision.
hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:97
+  // of inlinee under different context.
+  if (!Size && !CurrNode) {
+    CurrNode = PrevNode;
----------------
wenlei wrote:
> hoy wrote:
> > When `Size` is invalid, can it mean `Context` is shorter than an inline path in previous build? For example, given a leaf function with a previous inline path  `A @ B @ leaf`, and the current context `B @ leaf`, if we don't find a valid size on the reverse tri `A @ B @ leaf` , should we fall back to use the size stored in node `A`, instead of finding a sibling context like `A @ C @ leaf`?
> > if we don't find a valid size on the reverse tri `A @ B @ leaf`
> 
> You meant to say `B @ leaf`?
> 
> That's a good point though currently in this case, we won't find sibling `A @ C @ leaf` either, because the node for `B @ leaf` exists. We would get zero from `B @ leaf`. 
> 
> Assuming zero means unknown before we start to track zero size, we need to tweak it slightly to retrieve `A @ B @ leaf` in this case. 
> 
> ```
>   if (!Size) {
>     if  (!CurrNode)
>         CurrNode = PrevNode;
>     while (!Size && CurrNode) {
>       CurrNode = &CurrNode->getAllChildContext().begin()->second;
>       Size = CurrNode->getFunctionSize();
>     }
>   }
> ```
> 
Looks good. The code works for the `A @ B @ leaf` case.



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:310
+        outs() << getReversedLocWithContext(
+            symbolize(IP, ShowCanonicalFnName, ShowPseudoProbe));
       }
----------------
wenlei wrote:
> hoy wrote:
> > should use `UsePseudoProbes` instead of `ShowPseudoProbe`?
> This was intentional as it though we might still want to have a way to dump line callsite even for binary with probes. I don't have a strong opinion though.
`ShowPseudoProbe` makes sense. I actually misunderstood this code as on the non-printing path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108180