Page MenuHomePhabricator

[NewPM] Port the MIR Printing pass to new pass manager.
Needs ReviewPublic

Authored by czhang on Jul 3 2019, 6:38 PM.

Event Timeline

czhang created this revision.Jul 3 2019, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 6:38 PM
czhang updated this revision to Diff 208561.Jul 8 2019, 5:51 PM

rebase & clang-format

Minor stylistic things. Someone who understands passes should look at this.

llvm/include/llvm/CodeGen/MIRPrintingPass.h
2

Filename is .h not .cpp; also, needs a 'C++' tag.

llvm/lib/CodeGen/MIRPrintingPass.cpp
14

Includes aren't sorted properly.

czhang marked 2 inline comments as done.Jul 9 2019, 10:05 AM
czhang updated this revision to Diff 208731.Jul 9 2019, 10:08 AM

Alphabetize

I'm not familiar with MIR printing usecases, and there are no tests for MIR printer.
Yet if MIR printing pass is to be applied to the Module only then there is no need to make it a MachineFunction pass, just do everything in a single run(Module...) method.

I would expect to see a registration of this pass in PassRegistry.def as a module pass. Perhaps named as print-mir?

Also, this patch should definitely have some dependencies on prior work introducing MachineFunction as IRUnit etc...

llvm/lib/CodeGen/MIRPrintingPass.cpp
26–30

This looks as something artificial.
If you want to iterate through machine functions and print them there is no need to use pass manager.
Just manually iterate and print.

I took the liberty of setting a parent revision, hopefully thats what you did intend to do...

Thank you! Phabricator is still a mystery to me.

czhang marked an inline comment as done.Tue, Jul 30, 2:58 PM

I'm not familiar with MIR printing usecases, and there are no tests for MIR printer.
Yet if MIR printing pass is to be applied to the Module only then there is no need to make it a MachineFunction pass, just do everything in a single run(Module...) method.

I would expect to see a registration of this pass in PassRegistry.def as a module pass. Perhaps named as print-mir?

Also, this patch should definitely have some dependencies on prior work introducing MachineFunction as IRUnit etc...

I'm not so clear on the use case of the MIR printing pass either, but for sure, llc uses it. To be safe, I just followed how the legacy pass manager did things to try and keep the original intent the same. For example, I did not register the pass in the pass registry, because it is not actually part of the created pass pipeline in the legacy pass manager in llc. There is no way to specify print-mir as part of -run-pass, perhaps because it is already triggered by other command flags. So since MIR printing is done separately in llc, I have followed that convention and done the same in my llc patch.

llvm/lib/CodeGen/MIRPrintingPass.cpp
26–30

In the legacy pass manager, printing MIR functions is done exactly this way. I suspect the reason is that MIR printing for individual MachineFunctions is needed for debugging. Another reason I followed the design of the legacy pass manager is that it is quite a lot of boiler plate and analysis querying just to iterate over every function of a module, and then querying MachineModuleInfo to get the MachineFunction out of each Function. In fact, since the adaptors abstract exactly that, I followed the established precedent. Perhaps we could abstract the idea of iterating over the machine functions of a module some other way?

The MIRPrinter is used to print MIR when used with -stop-before / -stop-after / -run-pass flags in llc (see https://llvm.org/docs/MIRLangRef.html#mir-testing-guide).

In the case of -stop-*, LLVMTargetMachine.cpp:addPassesToEmitFile sets it up.
In the case of -run-pass, llc.cpp:compileModule does (if (!RunPassNames->empty()) { ...).

As for tests, basically all .mir tests that use the flags above use it.