[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#2557591 <https://reviews.llvm.org/D95988#2557591>, @wmi wrote:

> In D95988#2557503 <https://reviews.llvm.org/D95988#2557503>, @hoy wrote:
>
>> 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?
>
> I understand profile count returning is a benefit for top-down inlining, but profile count returning are all related with cold profiles, so it may not be the major factor here? I think letting an inlined function annotated with inline instance profile with context instead of outline instance profile without context is the major reason that top-down inlining brings benefit, at least for non-CS profile. This is the original patch from Wenlei to add top-down inlining support: https://reviews.llvm.org/D70655
>
> Sure, it is ok to address it in a separate change. Better add a TODO in the comment.
>
>> https://github.com/llvm/llvm-project/blob/f8772da8cc9a0be65c9ba028c2b5a895c1ed4f91/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1346

I see. Thanks for the explanation. Yes, the annotation quality for inlined instances are more important for outlined instances. TODO added for indirect calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95988