[PATCH] D99351: [CSSPGO] Top-down processing order based on full profile.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D99351#2654965 <https://reviews.llvm.org/D99351#2654965>, @wmi wrote:

> Seems there is still build error:
>
> lib/Transforms/IPO/SampleProfile.cpp:1692:23: error: no matching constructor for initialization of 'llvm::sampleprof::ProfiledCallGraph'
>
>   ProfiledCallGraph ProfiledCG;
>
> include/llvm/Transforms/IPO/ProfiledCallGraph.h:43:3: note: candidate constructor not viable: requires 2 arguments, but 0 were provided
>
>   ProfiledCallGraph(StringMap<FunctionSamples> &ProfileMap,

Oh I see. I didn't sync to Wenlei's latest patch whee the profiled callgraph construction was moved into the constructor. Since there is a subtle difference about whether to limit the call graph build to nodes with samples only, I'm adding back the default constructor for now. We can discuss how to make the code well shared.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1665-1667
+    // Considering those cases, a profiled call graph completely independent of
+    // the static call graph is constructed based on profile data, where
+    // function objects are not even needed to handle case #3 and case 4.
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Using ProfiledCallGraph allows us to order without needing function object, but profile could be stale (e.g. missing a new edge after source drift). Can we build with ProfiledCallGraph with static call graph nodes and edges included as well?
> > Actually adding static edges leads to worse performance for some benchmarks because of SCC. In that case, static edges in SCC should be completely removed so that only profile edges are honored. 
> > 
> > On the other hand, yes, profile could be stale, but that's the information FDO relies on. I think without the profile, top-down order isn't important. In other words, static call edges seems not important when they don't correspond to a context in the profile.
> Ok, this makes sense. So static edges can be conflicting then we may end up with SCC order not compatible with context trie. Using strictly profile order makes sure we will get maximum inlining along context trie. I think that (intentionally not adding call graph edges) worth a comment explaining by itself.
Sounds good, comment added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99351