[PATCH] D67687: [NewPM][CodeGen] Introduce machine pass and machine pass manager

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

> I think it is a special case. IIUC, MachineOutliner is the last machine pass before emitting and it is intended. So even if it is changing IR or invalidating IR analysis, it is not affect anything. If it is paranoid, it could choose to keep IR analyses up-to-date manually if not already.
> 
> If passes like MachineOutliner become very common (which I doubt), we should probably keep the existing design and let the pass manually handle IR analyses invaliation.
> 
> I gave some thoughts about a potential proxy class between machine function and these IR units. It is kind of hard to implement because you don't know before-hand the implication on IR analyses when MIR analyses is invalidated. The reason is, still, MIR and IR has no much concrete (for example, containing) relationship. If you look at the first few lines of comment above `OuterAnalysisManagerProxy`/`InnerAnalysisManagerProxy`, the relationship is required.

Ok I think you're right about MachineOutliner being special. Craig mentioned that MachineFunctions are emitted one at a time for memory reasons, and I've heard that from other people too. MachineOutliner breaks that so it must be special.

I'm still fuzzy on exactly why a "containing" relationship is required, but maybe I'm belaboring this point too much.

> Let say, `ModuleToMachineFunctionPassAdaptor` which is a module pass. We could use it in two ways: 1) wrap every machine pass in it and run the codegen process as a module pass pipeline (similar goes for a hypothetical FunctionToMachineFunctionPassAdaptor); 2) make the machine pass manager a module pass and insert it in a module pass pipeline.
>  For 1) I think the adaptor does not do much other than calling the contained machine pass. As a module pass, it does not modify module, it does not invalidate any IR analyses.
>  For 2) `ModuleToMachineFunctionPassAdaptor` is almost just a one-time use when setting up the codegen in BackendUtils.cpp/llc/LTO.cpp etc.. It is not generally useful. So we could merge it with the `PassManager<MachineFunction, MachineFunctionAnalysisManager>` class. I guess you meant this, right?
> 
> WDYT?

I *think* what you mean by "we could merge it with the `PassManager<MachineFunction, MachineFunctionAnalysisManager>` class" is what I wanted to say? Putting a pass manager in an adaptor is the normal way of doing things, like at [1].
An adaptor would help with things like https://reviews.llvm.org/D82698.

But maybe since the machine function pass managers returns an Error, it doesn't make as much sense to be able to compose it as a module pass.

[1]: https://github.com/llvm/llvm-project/blob/724bf4ee23a3993689d55afb2845949e05c83b1c/llvm/lib/Passes/PassBuilder.cpp#L792

I think I'm slightly more on board with this than before, although I would like somebody like asbirlea to also take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67687