[PATCH] D95988: [CSSPGO] Process functions in a top-down order on a dynamic call graph.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D95988#2557460 <https://reviews.llvm.org/D95988#2557460>, @wmi wrote:

> Thanks for adding the support for non-CS profile!
>
>> Extending profile-based top-down order support to non-CS profile. Only adding support for SCC. Indirect call edges are not needed since uninlined counts are not returned to indirect call targets with non-CS profiles.
>
> Indirect call edges are still helpful for non-CS profiles. That is because top-down inlining will be helpful for better non-CS profile matching fundamentally (unless annotated profile can be updated repeatedly, but that is not the case for branch probablity for non-CS profile). Using non top-down order, function may be annotated with outline instance's profile before it can be inlined and get more precise profile with context. Because there are no indirect call edges in the static call graph, it will be helpful to add them based on dynamic call graph, to enforce the top-down order inlining more thoroughly.

Yeah, it is fundamentally useful but I'm not sure there's way to justify the benefit right now. If you look at code below, nested callee profile is never returned to the outlined instance for unsuccessful inlining of indirect calls. I was thinking about a separate change to enable that as well as top-down order for indirect calls. What do you think?

https://github.com/llvm/llvm-project/blob/f8772da8cc9a0be65c9ba028c2b5a895c1ed4f91/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1346



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2299-2305
+  for (const auto &Sample : Samples.getBodySamples()) {
+    for (const auto &Target : Sample.second.getCallTargets()) {
+      Function *Callee = SymbolMap.lookup(Target.first());
+      if (Callee && !Callee->isDeclaration())
+        CG[Caller]->addCalledFunction(nullptr, CG[Callee]);
+    }
+  }
----------------
wmi wrote:
> We may not need this block. Top down inlining for non-CS profile is to get more precise profile matching, not to enable more inlining. If there is not inline instance profile for a callsite, early inlining in sample loader won't inline it so it doesn't need to be added into the dynamic call graph. 
Agreed, this is not needed since it does not add any benefit to profile counts returning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95988