[PATCH] D79042: [llvm] Add interface to drive inlining decision using ML model

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

The PM changes lgtm



================
Comment at: llvm/test/Other/new-pm-lto-defaults.ll:79
+; CHECK-O2-NEXT: Starting llvm::Module pass manager run.
+; CHECK-O2-NEXT: Running pass: ModuleToPostOrderCGSCCPassAdaptor<{{.*}}LazyCallGraph{{.*}}>
+; CHECK-O2-NEXT: Starting CGSCC pass manager run.
----------------
mtrofin wrote:
> tejohnson wrote:
> > mtrofin wrote:
> > > tejohnson wrote:
> > > > mtrofin wrote:
> > > > > tejohnson wrote:
> > > > > > What causes this one to get added?
> > > > > See PassBuilder.cpp:1329 - the patch uses the ModuleInlinerWrapper, so that we always inject, canonically, the InlineAdvisor outside the inline pass.
> > > > Right, I understand why the InlineAdvisorAnalysis gets added. But why does LazyCallGraph get added when it wasn't before?
> > > Oh! It's actually the devirt pass, which should be a no-op here. If we prefer, for simplicity/clarity, I can bypass using it in Inliner.cpp:1076
> > Oh I see. It is a noop because in the regular LTO case there is no simplification pass added to the CGSCC PM, and the inliner pass itself cannot create devirtualization opportunities? The regular LTO pipeline is going to be more sensitive to compile time, what is the overhead of adding this even when it is a noop? Might be better to avoid it.
> It's no-op because the MaxDevirtIterations parameter of the ModuleInlinerWrapperPass ctor is 0. I also prefer not instantiating it if not needed - addressed.
> 
> I looked deeper, here's what happens:
> 
>   -  originally, we had this:
> 
> 
> ```
> MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
>       InlinerPass(getInlineParamsFromOptLevel(Level))));
> 
> ```
>   - with the change, we have:
> 
> 
> ```
> MPM.add(ModuleInlinerWrapper)
> 
> ``` 
> 
> Now, ModuleInlinerWrapper has a ModuleToPostOrderCGSCCPassAdaptor wrapping a CGSCCPassManager, which inside has the InlinerPass (or, just before, the ModuleInlinerWrapper had a ModuleToPostOrderCGSCCPassAdaptor wrapping a devirt pass wrapping the CGSCCPassManager wrapping the InlinerPass)
> 
> When we used the devirt wrapper, the ModuleToPostOrderCGSCCPassAdaptor's template type parameters included the devirt wrapper type - hence my initially (and incompletely) observing that as a reason for the diff. Now, it includes PassManager as type parameters (and LazyCallGraph, but probably a more apt pattern match is on PassManager though)
> 
> I'd argue the CGSCCPassManager doesn't add much overhead in this case (as opposed to the devirt wrapper) - its run method basically goes straight to running the contained pass. 
> 
> Alternatively, if we want to completely avoid any overhead, we could do some customization in the ModuleInlinerWrapper to avoid using the CGSCCPassManager if there's just the inliner.
> 
> A third option I'd prefer not to do is to just instantiate (in this case) the InlinerPass like before. I prefer not going that way because the inliner parameters are now a concept owned by the InlineAdvisor.
> 
> WDYT?
I think I agree that wrapping in the additional CGSCC PM shouldn't be a problem. So I think this is fine now with the change to avoid the devirt wrapper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042