[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/lib/Transforms/IPO/Inliner.cpp:669
+
+  if (!CGSCCInlineReplayFile.empty()) {
+    if (!ReplayAdvisor)
----------------
wenlei wrote:
> modimo wrote:
> > wenlei wrote:
> > > Plug in replay inline advisor here isn't extensible. In the future we want to be able to use inline replay only for a specific function, or enforce/prevent certain inlining at particular callsite, and fall back to  regular advisor for the rest (see comments in D83743). 
> > > 
> > > That means we would need to be able to fall back from replay advisor to default advisor (or whatever main advisor being used) when replay advisor doesn't have info. For that cascaded model, we would need inline advise to have something like `hasInlineRecommendation` in addition to `isInliningRecommended`. We should probably still record inlining on each advice, but don't want to emit duplicated remarks from each advice. These changes can come later, but current change better offer that flexibility - we don't to stick to replay advisor for the entire module inliner pass.  
> > I think your suggested change is to initialize both Advisors and allow fallback if we defer on one. With how the current scheme is setup up though advisors are single entities and we only ask it once:
> > `auto Advice = Advisor.getAdvice(*CB);`
> > A proven approach to doing something like this is with alias analysis where we query AA in a specific order until we hit a real recommendation. That would cause a rehaul of this function rather than a small tweak so I don't see a good intermediate step to take for this patch. Let me know if you think there's something to do here now for it.
> Well, we don't have to have everything setup for the cascaded query like how AA works, but something more flexible than having entire inliner sticking to one advisor would be good (and does not seem like a significant change)
> 
> What I was thinking about is that the main advisor can still go through `getAdvisor` interface, then for inline replay, we can just let `ModuleInlinerWrapperPass` own an `ExternalInlineAdvisor` just like how `SampleProfileLoader` owns one. Then it can be passed to `InlinerPass` and serve as a short-circuit look up or side look up when available in addition to the main advisor from `getAdvisor`.
> 
> The changes to add `hasInlineRecommendation` etc are not what I'm suggesting for this patch though I don't think these are significant either. It can evolve into cascaded advice support in the framework if needed, but if replay inline advice is the only case needing that support, generalizing it doesn't not seem like a must do. 
+1 to what @wenlei  said - you can wrap advisors in advisors, basically. There may be some helper utilities that we may need factored in InlineAdvisor, and I was at a point thinking of doing that, but the motivating scenario at the time ended up not really needing that. If this turns out to be that scenario, I'd be happy to help!


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

https://reviews.llvm.org/D94334