[CodeGen][Pass] Add TargetPassBuilder

[paperchalice]

This implementation supports --start/stop-before/after options by hijacking original pass managers. With injected-class-name, user should not feel any difference from original pass manager in most cases.

Except virtual functions to add necessary passes (e.g. instruction select pass), the only method to extend the pipeline is injectBefore, which accept a callback to extend pipeline at the specified point. The return type of the callback is depend on the selected pass

injectBefore<SomeFunctionPass>([](){
  FunctionPassManager FPM;
  // Add passes you want.
  return FPM;
});

// If you really want to add module pass between machine function passes...
injectBefore<SomeMachineFunctionPass, ModulePassManager>([](){
  ModulePassManager MPM;
  MachineFunctionPass MFPM;
  // Add passes you want.
  MPM.createModuleToFunctionPassAdaptor(createFunctionToMachineFunctionPassAdaptor(std::move(MFPM)));
  return MPM;
});

[optimisan]

injectBefore and disablePass methods are independent of addPass calls which is nice.

But there are no virtual methods, so are we removing distinct phases (like addPreEmitPasses()) completely and only using injectBefore() now?
If we don't want overrides (virtual/CRTP) we could add "phase marker" passes (that are dummy passes like ISelPrepareBegin()) to be used with injectBefore.

[paperchalice]

But there are no virtual methods, so are we removing distinct phases (like addPreEmitPasses()) completely and only using injectBefore() now?

IMHO some phases here is not clear, the example is pre emit, we have addPreEmitPasses and addPreEmitPasses2() thus here only provides injectBefore.

If we don't want overrides (virtual/CRTP) we could add "phase marker" passes (that are dummy passes like ISelPrepareBegin()) to be used with injectBefore.

Yeah pre isel is one of phases injectBefore() can't handle easily, may add them in future.

---

To be more radical, I even want to use type erasure to ensure backends only add dag isel and asm printer to replace the getSelectionDAGISelPass in current code.

[paperchalice]

Ping @aeubanks @arsenm

[paperchalice]

Support dummy injection points now, they are PreISel, PostBBSections and PreEmit.

[cdevadas]

Instead of force pushing, can you keep separate commits each time you add a new patch? It would be easier to identify the new changes coming in.

[paperchalice]

ping @aeubanks

[paperchalice]

Ping? @aeubanks

[weliveindetail]

If I understand correctly, the idea in this patch is that TargetPassBuilder mimics the interface of PassBuilder, but doesn't build the final structure of PassManagers yet. Instead it stores the the final PassInfoMixin instances in an intermediate structure of PassManagerWrappers and PassWrappers. Backends will derive from TargetPassBuilder and adjust this intermediate structure with operations like filtPassList() and injectBefore(). Eventually, TargetPassBuilder's constructRealPassManager() will transcribe it into the final structure of actual PassManagers and return the executable pipeline. (This part of the patch has no test coverage yet.) Is that about right?

[paperchalice]

If I understand correctly, the idea in this patch is that TargetPassBuilder mimics the interface of PassBuilder, but doesn't build the final structure of PassManagers yet. Instead it stores the the final PassInfoMixin instances in an intermediate structure of PassManagerWrappers and PassWrappers. Backends will derive from TargetPassBuilder and adjust this intermediate structure with operations like filtPassList() and injectBefore(). Eventually, TargetPassBuilder's constructRealPassManager() will transcribe it into the final structure of actual PassManagers and return the executable pipeline. (This part of the patch has no test coverage yet.) Is that about right?

You are right. This is a dirty hack to implement --{start,stop}-{before,after} with following the design principle of new pass manager (users need to consider pass nesting explicitly, pass manager/builder should not handle this).

[weliveindetail]

Thanks! The --{start,stop}-{before,after} feature seems very important for testing purposes. The RFC first proposed to drop it in favor of testing individual passes in isolation or batches of passes with intermediate serialization. This is what the optimization pipeline already uses with the new pass manager and we should bring the codegen pipeline in line with it.

However, the RFC also notes that this change to the testing strategy would involve a "huge amount of tedious work porting tests". This part of the proposal also received critical feedback from @arsenm and @akorobeynikov, who explain why it doesn't fit the nature of the codegen pipeline. I think their arguments are sounds and convincing.

Coming back to your patch: If we need an intermediate structure to provide this feature, it should be fine to have it. It might be worth to make it more explicit though. Perhaps consider it more as a planning mode for the PassBuilder, rather than hiding it behind a hack?

You are right. This is a dirty hack to implement --{start,stop}-{before,after} with following the design principle of new pass manager (users need to consider pass nesting explicitly, pass manager/builder should not handle this).

What exactly do you consider the hack here? The logic in constructRealPassManager() with respect to the explicit nesting requirement? Or do you mean the hijacking of the pass-managers for the intermediate structure?

[paperchalice]

What exactly do you consider the hack here? The logic in constructRealPassManager() with respect to the explicit nesting requirement? Or do you mean the hijacking of the pass-managers for the intermediate structure?

Shadowing llvm::PassManager with llvm::TargetPassBuilder::PassManager, because I want to minimize the differences in usage between the two manager. Fortunately, it doesn't trigger warnings from -Wshadow.

[weliveindetail]

I agree that the interface should be as close as possible, but matching the interface doesn't mean we have to fake the classes. Let's look at an example that might make the code simpler. It might also help reviewers :)

[weliveindetail]

This looks very much like we use a llvm::PassManager<Module> while we actually get a llvm::TargetPassBuilder::PassManagerWrapper<llvm::PassManager<Module>, <more templates here>>. That might be surprising and figuring out the relations behind the scenes isn't exactly trivial :)

[weliveindetail]

Couldn't we just have a TargetModulePassManager instead? It wouldn't need to be hidden as a subclass anymore. And maybe even without templates? After all, PassManagerWrapper doesn't do so much.

[weliveindetail]

Then we could get rid of the type detection here (which btw doesn't work out-of-the-box on current main anymore)

[weliveindetail]

And then.. maybe we could spell these out for the actual pass managers we have? This looks a lot simpler to me:

protected:
  template <typename PassT>
  void injectModulePassManagerBefore(std::function<ModulePassManager()> F) {
    injectCallback(PassT::name(), std::move(F));
  }

private:
  template <typename PassManagerT>
  void injectCallback(StringRef PassName, std::function<PassManagerT()> F) {
    InjectionCallbacks.push_back(
    ...

And the interface still seems acceptable:

injectModulePassManagerBefore<ExpandFpPass>([] {
  TargetModulePassManager MPM;
  MPM.addPass(NoOpModulePass());
  return MPM;
});

[paperchalice]

We need syntactic salt for pass pipeline building DSL here, i.e. let users know they are injecting module pass into function or machine function pass pipeline, module pass would cause higher memory usage in codegen pipeline.
Maybe we can make injectModulePassManagerBefore usable only when PassT is a function pass or machine function pass.