[PATCH] D99146: [CSSPGO][llvm-profgen] Context-sensitive global pre-inliner

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:63
+
+    // Add calls for context, if both caller and callee has context profile.
+    for (auto &Child : Caller->getAllChildContext()) {
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > I'm wondering in the future if functions without profile should be considered so that a broader inline decisions can be made regardless of callsite hotness.
> > For pre-inlining, we need to have callee profile otherwise there's no profile to be adjusted regardless of whether we inline or not. 
> > 
> > For caller profile, we currently requires it to trigger pre-inline, but we could do pre-inline without caller profile. Is that what you meant? Currently compiler's inlining also requires caller profile, so what we have here aligns with compiler. 
> > 
> > (Not sure if I get the question, it's building top-down order here, not actual pre-inlining.. )
> Sorry for the confusion. I actually meant to explore non-profiled callees (such as getters/setters) here and in `getInlineCandidates` to mimid the situation that both hot callees of those getters/setters and themselves are inlined into the current caller. The getter/setter inlining, though their callsite are not hot, are likely done in prelink CGSCC inlining. I was thinking about simulating the prelink inlining if possible. There's no such need if prelink inlining is disabled. 
What do you mean by non-profiled callees? If function does not have profile, there's no profile to be adjusted, then doing pre-inline or not doesn't matter. CGSCC inline in prelink can happen but since there's no profile for the callee, no profile need to be adjusted and there's no count quality issue.

Or are you suggesting considering looking at multiple levels of callees when evaluating a call site? That is orthogonal to whether a function has profile. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99146