[PATCH] D109111: [CSSPGO] Use preinliner decision by default when available

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1314
+  if (UsePreInlinerDecision &&
+      Candidate.CalleeSamples->getContext().hasAttribute(
+          ContextShouldBeInlined))
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > wenlei wrote:
> > > > > hoy wrote:
> > > > > > wenlei wrote:
> > > > > > > hoy wrote:
> > > > > > > > Will non-inlined context be merged into the base profile by the preinliner? Wondering if we can have contexts without `ContextShouldBeInlined` in the profile after processed by the preinliner.
> > > > > > > Not inlined context will all be merged currently. See the comment I added above.
> > > > > > So we'll not see the context profile here? Can such context only be seen when the preinliner is off?
> > > > > Correct. When preinliner is on in llvm-profgen, we will only see context profile if preinliner decides to inline. When preinliner is off, we will see all context profiles. 
> > > > Thanks for the confirmation. Can you update the comment to reflect that?
> > > The comment mentions that:
> > > 
> > > > Note that we don't need to handle negative decision from preinliner as context profile for not inlined calls are merged by preinliner already.
> > Oh, I actually meant to say there shouldn't be a negative decision from preinliner, since `Candidate.CalleeSamples` would be null in that case. I might misunderstand your comment.
> > I actually meant to say there shouldn't be a negative decision from preinliner, since Candidate.CalleeSamples would be null in that case.
> 
> By decision from preinliner, it literally means inline decision made by preinliner in llvm-profgen. Preinliner would reject inlining for some calls, and these are the negative decision from preinliner. 
> 
> Candidate.CalleeSamples will never be null. If preinliner makes a negative inline decision, the corresponding context profile won't exist, and we won't see the callee as inline candidate (see getInlineCandidate). That's why we don't need to handle negative decisions explicitly.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109111