Page MenuHomePhabricator

[CodeGen] Define an interface for the new pass manager.
Needs RevisionPublic

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

Event Timeline

czhang created this revision.Jul 3 2019, 6:27 PM
czhang updated this revision to Diff 208555.Jul 8 2019, 5:46 PM

rebase & clang-format

arsenm added a subscriber: arsenm.Jul 29 2019, 12:19 PM
arsenm added inline comments.
llvm/include/llvm/CodeGen/PassManager.h
110

I thought it was OK for function passes to add new functions, just not delete

fedor.sergeev requested changes to this revision.Jul 30 2019, 5:35 AM

This definitely needs unit tests.
Please, check llvm/unittests/IR/PassManagerTest.cpp for Function/Module unit tests, and, say, llvm/unittest/Analysis/LoopPassManagerTest.cpp for Loop unit tests.

llvm/include/llvm/CodeGen/PassManager.h
86–88

If you do want to emit size remarks after each machine-function pass then its best to be done through pass instrumentation.
Since you do not implement pass instrumentation right now, skip this code as well.

This revision now requires changes to proceed.Jul 30 2019, 5:35 AM

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

In particular:

  • how MachineFunction is being produced
  • when MachineFunction is invalidated
  • how MachineFunctions are being iterated (any particular order required etc)
  • what is the "container" relation of MachineFunction to other IRUnits

Currently we have an hierarchical structure of IRUnits, with Module "containing" Functions, Function "containing" Loops, CGSCC "containing" Functions and contained in the Module.
Container is a rather principal relation here, which in particular defines the way analysis proxies are organized (Inner and Outer ones).

Currently in your code Function contains (a single) MachineFunction.
And iteration through MachineFunctions happens "by Function" (through FunctionToMachinePassAdaptor).
I see nothing wrong with this solution, just as it is a very principal decision I would better see it mentioned somewhere in high-level comments.
And then it seems to be more akin to Loops, so I would recommend to take a look at what LoopPassManager and FunctionToLoopPassAdaptor are doing.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor
paquette added inline comments.
llvm/include/llvm/CodeGen/PassManager.h
86–88

Size remarks aren't supported in the new PM at all right now. I'm not entirely sure that this would work. (Testcase?)

It would be nice to have a port that would work for both codegen and IR-level passes though. :)

philip.pfaffe added inline comments.Jul 31 2019, 10:19 AM
llvm/include/llvm/CodeGen/PassManager.h
86–88

I'm not sure I understand this comment. Can you be specific in what you think the issue is currently?

czhang added a comment.EditedAug 1 2019, 1:14 PM

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

In particular:

  • how MachineFunction is being produced

MachineFunction is produced from the MachineModuleInfo analysis on Functions. For the purposes of this patch, it doesn't really matter where the MachineFunction comes from. However, the next patch in the series addresses this, since the MachineModuleInfo analysis produces MachineFunctions for Functions in the legacy pass manager (and that change keeps it that way).

  • when MachineFunction is invalidated

MachineFunctions do not get invalidated. The contract is that MachineFunction passes do not mutate their containing Functions.

  • how MachineFunctions are being iterated (any particular order required etc)

MachineFunctions are never the result of any iteration. Modules are iterators yielding Functions, and MachineFunctions are associated with Functions. So any kind of ordering is induced from the ordering of Functions in a Module, which is all modeled explicitly with the adaptors. Since no implicit ordering is introduced with this change, there is not much to say about it which isn't said already by the ModuleToFunctionPassAdaptor.

  • what is the "container" relation of MachineFunction to other IRUnits

    Currently we have an hierarchical structure of IRUnits, with Module "containing" Functions, Function "containing" Loops, CGSCC "containing" Functions and contained in the Module. Container is a rather principal relation here, which in particular defines the way analysis proxies are organized (Inner and Outer ones).

Yes, the relationship between MachineFunction and Function is currently a little awkwardly modeled in LLVM. MachineFunctions are treated like loops in the sense that they must be queried from a Function analysis.

Currently in your code Function contains (a single) MachineFunction.
And iteration through MachineFunctions happens "by Function" (through FunctionToMachinePassAdaptor).
I see nothing wrong with this solution, just as it is a very principal decision I would better see it mentioned somewhere in high-level comments.
And then it seems to be more akin to Loops, so I would recommend to take a look at what LoopPassManager and FunctionToLoopPassAdaptor are doing.

Ditto. Except it's not just my code that treats MachineFunctions as being contained in a Function. The legacy pass manager also treated MachineFunction as being contained inside Functions. Of course, that doesn't necessarily mean we need to repeat that design decision, which may just be an artifact of how the inheritance model worked, but enough of the existing code in CodeGen outside of the pass manager assumes such a container relationship, so this is the least intrusive way to model this.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

Okey, everything you said there makes perfect sense. Thanks for clarification.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

I mean both should work :)
In particular, MachineFunctionPassManager.
It seems that you just defined a typedef in a hope that it will work... for one you need some tests to verify that.
And my guess is that as soon as you instantiate MachineFunctionPassManager::run you will hit missing interfaces that generic PassManager template refers to.
At least I'm sure you will hit PassInstrumentation ones.

Lets first start with some basic unittests (see llvm/unittests/IR/PassManagerTest.cpp etc).

Which part of the basics specifically?

Also, avoid adding anything into the adaptor except just basic functionality of passing control to the target pass.
(say, as I commented already - skip remarks stuff altogether). This commit better contain the general comments on what MachineFunction is
and basics required for PassManager/Adaptor to start working, and nothing else.
Since you will have to add PassInstrumentation interfaces in order for PassManager to operate you might be able to add its usage into PassAdaptor as well.
But that could be done as a follow up change.

@fedor.sergeev

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

Okey, everything you said there makes perfect sense. Thanks for clarification.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

I mean both should work :)
In particular, MachineFunctionPassManager.
It seems that you just defined a typedef in a hope that it will work... for one you need some tests to verify that.
And my guess is that as soon as you instantiate MachineFunctionPassManager::run you will hit missing interfaces that generic PassManager template refers to.
At least I'm sure you will hit PassInstrumentation ones.

Lets first start with some basic unittests (see llvm/unittests/IR/PassManagerTest.cpp etc).

But everything does work :). The reason I know is because the 6 follow up patches after this allow llc to run and produce bitwise identical inputs for the 5 other passes I ported after this compared to the legacy pass manager. (using --run-new-pass after the prerequisite passes are manually run with stop-before). It's harder to see that's the case because I had split up this patch series,

Nonetheless, you are absolutely correct that some amount of unit testing is needed.

By the way, the reason why I've had a hard time with the unit tests is that MIR modules are not so easily serialized and read as IR modules are, because the representation isn't as static. Any suggestions on how to achieve something similar to parseIR cleanly without resorting to the mess in llc would be highly appreciated. For now, testing is done by checking bitwise identical outputs from both pass managers, leveraging the parsing/machine function extraction boilerplate in llc.

If there is in fact a need to use the same machine function extraction functions from MachineModuleInfoAnalysis that are only present in the next patch, we start to have coupling issues with the child dependencies of this pass, since properly handling the MIR parsing, serializing, and machine function extraction is done in the next few patches. So then there are two ways to go forward:

  1. Add unit tests at the end of this patch series.
  2. Leverage the legacy pass manager to do parsing and readout of machine functions from the yaml format for the unit tests.

Any particular opinions? I would be in favor of 1. so we can start to land some of these patches in the series (2/7 with lgtm) that have already been approved and depend on this one so that they don't fall prey to more rebase issues later. It is rather hard to maintain a seven patch series.

Otherwise the unit tests would mostly be a straight copy of the tests for the Module info ones in PassManagerTest.cpp, with the proper renaming done, modulo the differences in parsing and matching of serialized MIR.

ychen added a subscriber: ychen.Aug 20 2019, 11:19 AM
ychen added a comment.EditedTue, Sep 17, 8:47 PM

Hello, Charlie went back to school after the internship. I'm trying the address the comments in a new patch. D67687