This is an archive of the discontinued LLVM Phabricator instance.

[mlir:Pass] Add support for op-agnostic pass managers
ClosedPublic

Authored by rriddle on Apr 11 2022, 12:16 PM.

Details

Summary

This commit refactors the current pass manager support to allow for
operation agnostic pass managers. This allows for a series of passes
to be executed on any viable pass manager root operation, instead
of one specific operation type. Op-agnostic/generic pass managers
only allow for adding op-agnostic passes.

These types of pass managers are extremely useful when constructing
pass pipelines that can apply to many different types of operations,
e.g., the default inliner simplification pipeline. With the advent of
interface/trait passes, this support can be used to define FunctionOpInterface
pass managers, or other pass managers that effectively operate on
specific interfaces/traits/etc (see #52916 for an example).

Diff Detail

Event Timeline

rriddle created this revision.Apr 11 2022, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 12:16 PM
rriddle requested review of this revision.Apr 11 2022, 12:16 PM

I don't quite get the merging yet, I need to wrap my head around it...

mlir/docs/PassManagement.md
324

wrap?

mlir/include/mlir/Pass/PassManager.h
48–49

"This class is generally not constructed directly" > I think this predates "dynamic pipeline" where now it is commonly built like that.

63

Nice doc improvements :)

214

That would be nice!

mlir/lib/Pass/Pass.cpp
99

Can you spell out types here?

rriddle updated this revision to Diff 424367.Apr 21 2022, 8:51 PM
rriddle marked 5 inline comments as done.
stellaraccident accepted this revision.Apr 21 2022, 10:23 PM

Sorry for the delay - it has been a rough couple of weeks.

This looks really good. My main comments are around documentation tweaks, so marking as accepted. I'm a little bit shaky on the merging logic/intent, but it looks like Mehdi is dialed in there.

mlir/docs/PassManagement.md
298

I had to read through the docs a couple of times to get my head around the definitions. Perhaps an explicit tie-in that op-agnostic passes can be filtered (and then refer to below). Maybe change the parenthetical:

..., or op-agnostic (not restricted to a static operation type, but optionally filtered -- see below)

Basically, I think it bears more explicit calling out that filtered passes are a type of op-agnostic passes.

I also think that a person not coming in with the historic context would ask why we distinguish these two types of passes/pass-managers (i.e. aren't op-specific passes/pass-managers just a type of op-agnostic but filtered to a specific operation type). I suspect that there is a reason (perf?) for special casing this, but might be useful to call this out.

443

Just so I'm clear: it is not an error for a filtered pass to encounter an op in an op-agnostic pass manager for which it does not support (i.e. it will just ignore it)? This is different from if added to a mismatched op-specific pass manager, right? (i.e. there it is assumed to be a programmer error and reported)

mlir/include/mlir/Pass/PassManager.h
140

Just so I understand: this doesn't constrain us in terms of any actual op names in dialects (i.e. even if the builtin dialect had an "any" op, it would be selected by "builtin.any", not "any").

Still, the part of me that has feelings about special bare-words would wish that this anchor name were syntactically not an op name in some way readily distinguishable. Something like "#match-any" or some such thing.

But I now see below that this is also used in the textual pass pipeline. I think I would still prefer some kind of special syntax, but not a strong opinion.

214

Agreed - would be very nice. I was looking in dismay at all of the places where we overly constrain to builtin.module (because we have to) and not looking forward to baking that further in.

mlir/lib/Pass/Pass.cpp
589

sp. "conflict"

596

Why switching to /// form here and below?

mlir/lib/Pass/PassDetail.h
32

Can you expand on what this is meaning to do? I was trying to match the implementation to my mental assumed model and it was not completely unambiguous to me.

This revision is now accepted and ready to land.Apr 21 2022, 10:23 PM
rriddle added inline comments.Apr 21 2022, 10:30 PM
mlir/docs/PassManagement.md
443

A filtered pass will never encounter an op that it doesn't support. The part that this paragraph is trying to describe is that an op-agnostic pass manager inherits the constraints of all of the passes inside of it. More specifically, if an operation can't run on one of the passes inside a pass manager, it won't run on the pass manager at all.

mehdi_amini added inline comments.May 9 2022, 12:23 PM
mlir/lib/Pass/Pass.cpp
743

Should this be: "find an executor for this operation"?

mlir/test/Pass/pipeline-parsing.mlir
55

Seems like this one could be merged with the first any pipeline?

rriddle updated this revision to Diff 428546.May 10 2022, 6:16 PM
rriddle marked 9 inline comments as done.
rriddle added inline comments.May 10 2022, 6:17 PM
mlir/docs/PassManagement.md
298

Restructured the docs and merged this into the section on passes.

mlir/include/mlir/Pass/PassManager.h
140

Right, operation names require dialect namespace prefixes so we would never hit a conflict here. I chose 'any' because its easy to be consistent across the textual format, documentation, etc.

I could switch this to <any> if that is less confusing?

mlir/test/Pass/pipeline-parsing.mlir
55

We don't know what type of filtering a pass is doing, given that it's opaquely behind a callback, so we never merge any pipelines.; given that merging could overly constrain the pass manager, which is potentially not what the user expected.

stellaraccident accepted this revision.May 10 2022, 10:39 PM

Thank you for the clarifications. lgtm.

mlir/include/mlir/Pass/PassManager.h
140

I don't feel strongly about it. I'm fine leaving it as it is with your explanation.

mehdi_amini accepted this revision.May 11 2022, 11:34 AM
This revision was landed with ongoing or failed builds.May 12 2022, 1:13 PM
This revision was automatically updated to reflect the committed changes.