[PATCH] D110658: [InlineAdvisor] Add -inline-replay-strict to replay inline decisions only in callers that have remarks

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:835
       // Check whether we want to inline this callsite.
-      if (!Advice->isInliningRecommended()) {
+      if (!Advice || !Advice->isInliningRecommended()) {
         Advice->recordUnattemptedInlining();
----------------
modimo wrote:
> mtrofin wrote:
> > modimo wrote:
> > > modimo wrote:
> > > > mtrofin wrote:
> > > > > modimo wrote:
> > > > > > Since getAdvice returns a unique_ptr, returning a null seems like a good way to indicate "no advice". Does that also make sense for the ML inliner @mtrofin?
> > > > > The design intent was for the advice to be clear, i.e. either inline or not. You probably want to delegate the decision to some other advisor if you don't have one? i.e. the remarks advisor could delegate to the default one - and return a non-null Advice. WDYT?
> > > > It does delegate it in the CGSCC case because we have a nicely nested system with the default one taking over. 
> > > > 
> > > > Unfortunately the SampleProfile inliner currently isn't using the InlineAdvisor setup. That being said, perhaps the better approach is to adapt the SampleProfile inliner to an InlineAdvisor so the replay advisor will always return a non-null.
> > > The SampleProfile inliner needs additional information beyond `CallBase` to make its decision. Namely, `InlineCandidate` which contains sampling information. Does it make sense to extend `getAdvice` to take additional information?
> > I don't follow why, but to the scope of this patch, couldn't the ReplayInlineAdvisor return an Advice that says "no" when it has no advice to provide?
> In SampleProfile, it needs 3 states:
> 1. Yes when it matches replay
> 2. No when it doesn't match replay
> 3. HasNoAdvice when in strict mode and we want the SampleProfile inliner to make the decision
> 
> ATM there's only (1) and (2), I'm using null to represent (3). An alternative could be to have a HasNoAdvice state that maps to no inline but can be queried to differentiate between (2) and (3)
I think I see now. OK, let's allow a null return then from `getAdvice`; to answer your original question, I don't think it affects advisors that want to be categorical (i.e. an advisor can always return yes/no), but it affects consumers, and your patch handles that. The ML stuff doesn't consume, it just implements the advisor interfaces.

We can explore afterwards if/how to adapt SampleProfile to an advisor design; we'd definitely not want to introduce that `InlineCandidate` in the advisor interface, the former is too specific to the sample profiler.

Could you leave a comment on the getAdvice() abstract API re. the fact that it can return nothing, a pointer to this review, and a TODO that maybe we can tighten it?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110658