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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1074
+  // module pipeline by walking the SCCs in postorder (or bottom-up).
+  if (MaxDevirtIterations > 0)
+    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
----------------
mtrofin wrote:
> davidxl wrote:
> > mtrofin wrote:
> > > davidxl wrote:
> > > > Is this guard needed?
> > > Well... only to avoid creating a wrapper if one isn't needed?
> > While this is a good change, it seems irrelevant to this patch though.
> Fair enough - removed. The devirt wrapper is noop with MaxDevirtIterations == 0 anyway.
Undid this - see tejohnson's observation about the LTO pipeline sensitivity to compiletime.


================
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.
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042