[PATCH] D94334: [InlineAdvisor] Allow replay of inline decisions for the CGSCC inliner from optimization remarks

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:201
   std::unordered_set<const Function *> DeletedFunctions;
+  std::shared_ptr<InlineAdvisor> ReplayAdvisor;
 };
----------------
modimo wrote:
> mtrofin wrote:
> > wenlei wrote:
> > > By adding a `ReplayAdvisor` field into every `InlineAdvisor`, we're allowing advisors to be chained. It'd be weird if a replay advisor itself has a non-empty replay advisor though the current implementation doesn't prohibit that. However if we change the names to be like a fall back advisor, and let replay advisor be just a use case of the fall back chain, I think that would be more reasonable. 
> > Instead of all advisors knowing about replay, why not doing it vice-versa: Replay wraps other advisors. Then, in InlineAdvisorAnalysis::Result::tryCreate (in InlineAdvisor.cpp), you see if replaying is requested, and build the ReplayInlineAdvisor wrapping the advisor requested initially - something like adding, right before return:
> > 
> > if (ReplayRequired)
> >   Advisor = std::make_unique<ReplayInlineAdvisor>(<params>, std::move(Advisor))
> > 
> > I believe this keeps the concerns (replaying vs regular advising) separated, while also allowing future usecases where the the replay advisor can delegate to some other advisor, generically.
> > 
> > Note: I think you'd also need to have a ReplayInlineAdvice wrapping the InlineAdvice coming from the contained InlineAdvisor.
> > However if we change the names to be like a fall back advisor, and let replay advisor be just a use case of the fall back chain, I think that would be more reasonable.
> Fall back would be better wording for it, agreed.
> 
> > Instead of all advisors knowing about replay, why not doing it vice-versa: Replay wraps other advisors.
> I like it, knowing about `tryCreate` makes it easier than I first thought given it's a centralizing creation point where we can wrap.
> 
> >Note: I think you'd also need to have a ReplayInlineAdvice wrapping the InlineAdvice coming from the contained InlineAdvisor.
> Can you elaborate on what would go into `ReplayInlineAdvice`? My thinking is that if the `ReplayInlineAdvisor::getAdviceImpl` declines to offer advice then we go to `OriginalAdvisor->getAdvice(CB)` so wrapping doesn't seem needed.
TL;DR; right now it's probably fine.

Longer story: the ML advisors are stateful - they track module-wide changes. So *if* we wanted to combine replaying with one of those, then the replayer would always have to get an advice from the underlying advisor, so it'd be able to notify back through it on what actually happened.

But the motivation for that scenario is kind of tenuous, I think, and it'd complicate the design unnecessarily. May be better to just disallow replaying with anything else other than the default advisor and we can add there a comment as to why.


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

https://reviews.llvm.org/D94334