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

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:201
   std::unordered_set<const Function *> DeletedFunctions;
+  std::shared_ptr<InlineAdvisor> ReplayAdvisor;
 };
----------------
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.


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

https://reviews.llvm.org/D94334