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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

In D67687#2164806 <https://reviews.llvm.org/D67687#2164806>, @aeubanks wrote:

> >> You mentioned
> >> 
> >>> Because their data structures are separated. Logically they are representations of the same/similar thing at different abstraction level.
> >> 
> >> in the email thread. Do you mean that there should be some sort of `MachineModule`, instead of `Module` corresponding to both IR and MIR?
> > 
> > By saying "their data structures are separated" I meant that modifying/transforming MIR does not cause any change to IR. MIR does not belong to IR class hierarchy. It is not like changing an IR loop also meaning changing its containing IR function or module.
> > 
> > Also, in MIR, there is basically one kind of IR unit (in NPM terminology): machine function. IR has many kinds of IR units: basic block(relatively small scope not so useful for optimization), loop, function, cgscc, module.
> > 
> > In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.
> > 
> > Based on the above, the requirement for MIR pipeline about IR is that 1) we don't run any IR transformation pass; 2) we ran machine pass initiated IR analyses; 3) we could not invalidate IR analyses.
>
> When you say we can't invalidate IR analyses, do you mean we need to assert that any IR analyses that a pass tries to get must not be invalidated? Does that require something like https://reviews.llvm.org/D72893?
>  Also, something like `MachineOutliner::createOutlinedFunction()` does create a new `Function` in order to create a `MachineFunction`, which seems like it could invalidate IR analyses.


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.

>> Defining a proxy class does not do any actual work(the invalidation part is not needed, the rest are just querying the delegated pass manager) although it is definitely conceptually appealing. Likewise, a potential adaptor class (maybe only `ModuleToMachineFunctionPassAdaptor` as you mentioned) is possible but other than iterating MIR functions from an IR module, it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager. The MachineFunctionPassManager is roughly `ModuleToMachineFunctionPassAdaptor` + `PassManager<MachineFunction, MachineFunctionAnalysisManager>` combined.
> 
> I don't understand "it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager", can you explain that more?

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?

>> In conclusion, it is possible to use proxy and adaptor classes here and make things work. But IMHO it is unnecessary coupling into IR pass manager infra. Although it is appealing conceptually, it could also cause confusion. Actually, without the `doInitialization`/`doFinalization` stuff here, all existing code is necessary even if we changed to use proxy and adaptor classes.
>> 
>> Please let me know if this makes any sense to you. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67687