Page MenuHomePhabricator

Implement a new kind of Pass: dynamic pass pipeline
ClosedPublic

Authored by mehdi_amini on Aug 21 2020, 10:30 PM.

Details

Summary

Instead of performing a transformation, such pass yields a new pass pipeline
to run on the currently visited operation.
This feature can be used for example to implement a sub-pipeline that
would run only on an operation with specific attributes. Another example
would be to compute a cost model and dynamic schedule a pipeline based
on the result of this analysis.

Discussion: https://llvm.discourse.group/t/rfc-dynamic-pass-pipeline/1637

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bondhugula added inline comments.
mlir/include/mlir/Pass/Pass.h
169

Who owns / has to free this OpPassManager?

mlir/lib/Pass/Pass.cpp
384

You are leaking memory here?

bondhugula added inline comments.Aug 22 2020, 9:04 PM
mlir/include/mlir/Pass/Pass.h
166–167

Rephrase/reorder this a bit to start with "Builds an OpPassManager containing .... This is equivalent ..."?

mehdi_amini marked an inline comment as done.Aug 22 2020, 9:09 PM
mehdi_amini added inline comments.
mlir/include/mlir/Pass/Pass.h
169

I wrote it initially as returning a reference and changed it to pointer to allow for returning nullptr, I didn't think it could be confusing from an ownership question (also I'd return a unique_ptr if I was transferring ownership). I'll update the doc!

The intent is that the pass can build the pipeline once, or caching multiple versions and return the one to apply.

bondhugula added inline comments.Aug 22 2020, 10:11 PM
mlir/include/mlir/Pass/Pass.h
169

Thanks. Then, it would be good to document that nullptr could be returned. Is nullptr is equivalent to an empty pipeline?

mlir/lib/Pass/Pass.cpp
381–382

The comment is grammatically broken at "... either ..." and also ".. retried ..."

You may also want to add an [MLIR] tag to the title to not confuse LLVM proper folks.

You may also want to add an [MLIR] tag to the title to not confuse LLVM proper folks.

I think this is noise in my git log and emails: it is likely better solved with tooling.

You may also want to add an [MLIR] tag to the title to not confuse LLVM proper folks.

I think this is noise in my git log and emails: it is likely better solved with tooling.

Sure - sounds good. But I had a couple of other questions above - hope you didn't miss them.

Use a callback instead to drive the pipeline run

mehdi_amini added inline comments.Wed, Aug 26, 10:39 PM
mlir/include/mlir/Pass/Pass.h
169

Yes.

Rename: renameDynamicPipeline -> runDynamicPipeline

Remove the special pass and make the callback universally available

silvas added a subscriber: silvas.Thu, Aug 27, 1:17 PM
silvas added inline comments.
mlir/include/mlir/Pass/Pass.h
45

Is it possible to allow this to take an op or set of ops to run on?

For example, an inliner doing SCC visitation might want to runPipeline(pipeline, currentSCC)

mehdi_amini added inline comments.Thu, Aug 27, 4:02 PM
mlir/include/mlir/Pass/Pass.h
45

I would imagine an inlined working on SCC to have to handle SCC somehow: the pass infra does not allow that just now so I'm not sure what would you do with this extra argument?

silvas added inline comments.Fri, Aug 28, 5:07 PM
mlir/include/mlir/Pass/Pass.h
45

Yes, it has to handle the SCC, but it will want to run a simplification pipeline on the current SCC before moving to the parent SCC. An inliner is a ModulePass in MLIR, and so the current runPipeline would just re-run the pipeline on the entire module. But the inliner only wants to run the pipeline on some subset of functions in the module.

Allow to run the pipeline on any op nested under the current operation

mehdi_amini added inline comments.Fri, Aug 28, 5:53 PM
mlir/include/mlir/Pass/Pass.h
45

Right but it would have to reschedule itself the function passes individually on the functions in the SCC.
As long as the PM in MLIR does not have a concept of SCCs, I'm not sure how to express this otherwise?

silvas requested changes to this revision.Mon, Aug 31, 12:38 PM

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

Also, I assume this needs doc updates?

mlir/include/mlir/Pass/Pass.h
45

Correct, all I meant was that the inliner needed a hook to manually run passes on operations in some effectively arbitrary way. Not that the PM needed special knowledge.

The new code looks good.

mlir/lib/Pass/Pass.cpp
360

change to assert or add test?

371

should this be an assert? if not it needs a test.

mlir/test/Pass/dynamic_pipeline_nested.mlir
1 ↗(On Diff #288737)

use - instead of _ in test names.

5 ↗(On Diff #288737)

I find the FileCheck lines here hard to follow. Do we have to actually check all the inner_mod1 stuff and "Dump Before" lines? (and any mention of CSE itself). Can we just do

NESTED: @foo
NESTED: @bar
NONESTED-NOT: @foo 
NONESTED-NOT: @bar
This revision now requires changes to proceed.Mon, Aug 31, 12:38 PM
mehdi_amini marked an inline comment as done.Mon, Aug 31, 5:32 PM

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

I still see this as dynamic: from the pass pipeline point of view there is a "pass" which will execute "dynamically" sub-pass-pipelines. This feature enables dynamism in the pass pipeline where it does not exist today.

Also, I assume this needs doc updates?

Right, I'll document the final version :)

mlir/lib/Pass/Pass.cpp
371

Could be an assert, but seems more friendly to have this always diagnosed and gracefully fail: it is easy to get wrong in a pass I suspect.

mlir/test/Pass/dynamic_pipeline_nested.mlir
5 ↗(On Diff #288737)

I don't think so: @foo and @bar will be in the output always (we print the IR...).

I'm checking at which granularity we apply the pipeline by checking where the CSE pass applies: will it run on the inner_mod1 module, where the dynamic pass is scheduled? Or will it run on each of the nested functions individually?

mehdi_amini marked 3 inline comments as done.Mon, Aug 31, 5:34 PM
mehdi_amini marked 5 inline comments as done.

Add comments

mlir/lib/Pass/Pass.cpp
360

Made is a report_fatal_error (so that the error message is richer than an assert)

371

Added a test for this one now.

mlir/test/Pass/dynamic_pipeline_nested.mlir
5 ↗(On Diff #288737)

I increase the doc in the test, let me know if it more clear.

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

I still see this as dynamic: from the pass pipeline point of view there is a "pass" which will execute "dynamically" sub-pass-pipelines. This feature enables dynamism in the pass pipeline where it does not exist today.

I mainly don't like the association with "pipeline", since "pipeline" doesn't really mean anything specific currently, except our concept of registering a "pass pipeline" (which is purely a concept for registration / textual parsing of pipelines). In fact, the name of the method is runPipeline which is confusing IMO. It runs an OpPassManager, and we don't really refer to an OpPassManager as a "pipeline" in any public API or documentation AFAICT. We seem to use "pipeline" mostly when talking about a textual description.

Thoughts? Maybe OpPassManager needs commentary that it can be informally be referred to as "a pipeline" and maybe should be renamed to OpPassPipeline? In fact, can OpPassManager just be a Pass, and then we can call this function runPass?

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

I still see this as dynamic: from the pass pipeline point of view there is a "pass" which will execute "dynamically" sub-pass-pipelines. This feature enables dynamism in the pass pipeline where it does not exist today.

I mainly don't like the association with "pipeline", since "pipeline" doesn't really mean anything specific currently,

I thought "pass pipeline" was a fairly common concept, I'm using it as "sequence of passes to apply on some IR". Anecdotally a quick Google query on "pass pipeline compiler" seems to find mostly reference in the same direction (doc from LLVM, Rust, Haskell, Numba).

except our concept of registering a "pass pipeline" (which is purely a concept for registration / textual parsing of pipelines)

Our pipeline aren't tied to parsing a textual representation: https://mlir.llvm.org/docs/PassManagement/#pass-pipeline-registration ; note that the "textual" part is in a separate section after this one.

It runs an OpPassManager, and we don't really refer to an OpPassManager as a "pipeline" in any public API or documentation AFAICT.

https://mlir.llvm.org/docs/PassManagement/#pass-manager "The PassManager class is used to configure and run a pipeline. The OpPassManager class is used to schedule passes to run at a specific level of nesting."

Thoughts? Maybe OpPassManager needs commentary that it can be informally be referred to as "a pipeline" and maybe should be renamed to OpPassPipeline? In fact, can OpPassManager just be a Pass, and then we can call this function runPass?

runPass is misleading to me because of the singular here: I see a difference between one pass and a sequence of passes. It could be runPasses maybe?

silvas added a comment.Tue, Sep 1, 3:31 PM

I still consider this feature naming (such as test case naming) very confusing. It's actually very simple to explain: "it allows a pass to call other passes". The name "dynamic pipeline" is very confusing -- like I'll need to re-explain to every new MLIR user that encounters the feature what the feature really is. "user: How can I call a pass from within my pass? mlir community: oh, use our dynamic pipeline feature. user: what is that? mlir community: it's a way to call a pass from within another pass; user: oh".

Maybe "dynamic pass invocation"?

runPass is misleading to me because of the singular here: I see a difference between one pass and a sequence of passes. It could be runPasses maybe?

runPasses SGTM.

runPass singular does make sense when an OpPassManager is itself a pass that simply runs a sequence of inner passes (such as in the llvm new PM). That was one of my major learnings from working on the LLVM new PM: there is actually no need for a thing to "manage" passes (which *was* necessary in the legacy PM design). After properly factoring things, the only remnant of the previous "management" component is 1) Adaptors that convert from (in MLIR speak) one operation type to another (module pass vs function pass etc.) 2) Passes that aggregate a sequence of other passes (there's no "management"; just "run a sequence") and 3) analysis manager. That is, we could have func(cse,canonicalize) really just be syntax sugar for sequence-of-passes{op-name=std.func passes=cse,canonicalize}.

I still consider this feature naming (such as test case naming) very confusing.

But it seems like you also never encountered "pipeline" which is surprising to me as it seems like a very common terminology (and we've been using it in MLIR "forever").

runPass singular does make sense when an OpPassManager is itself a pass that simply runs a sequence of inner passes (such as in the llvm new PM).

The LLVM PM entry point is run, isn't it?

That was one of my major learnings from working on the LLVM new PM: there is actually no need for a thing to "manage" passes (which *was* necessary in the legacy PM design).

I don't share your view on the new PM apparently. The new PM is a departure in that there is no need to manage *pass dependencies* statically, but that does not mean that passes aren't managed entirely: there is a piece of code that traverse the IR and schedule the execution of these passes on an IR unit, this management hasn't really changed.

After properly factoring things, the only remnant of the previous "management" component is 1) Adaptors that convert from (in MLIR speak) one operation type to another (module pass vs function pass etc.) 2) Passes that aggregate a sequence of other passes (there's no "management"; just "run a sequence") and 3) analysis manager. That is, we could have func(cse,canonicalize) really just be syntax sugar for sequence-of-passes{op-name=std.func passes=cse,canonicalize}.

I don't really get your point: I still see a difference between the management and the passes themselves: the 1, 2, and 3 is exactly this.

silvas added a comment.Tue, Sep 1, 4:01 PM

I still consider this feature naming (such as test case naming) very confusing.

But it seems like you also never encountered "pipeline" which is surprising to me as it seems like a very common terminology (and we've been using it in MLIR "forever").

No, I'm well aware of that terminology. As you mentioned it is very common. It just doesn't really have a precise meaning we can refer to in the code, and is only a specific case of what this feature allows.

The below discussion need not block this review, since we don't currently implement OpPassManager as a pass.

runPass singular does make sense when an OpPassManager is itself a pass that simply runs a sequence of inner passes (such as in the llvm new PM).

The LLVM PM entry point is run, isn't it?

Yes, "pass" has a method "run".

That was one of my major learnings from working on the LLVM new PM: there is actually no need for a thing to "manage" passes (which *was* necessary in the legacy PM design).

I don't share your view on the new PM apparently. The new PM is a departure in that there is no need to manage *pass dependencies* statically, but that does not mean that passes aren't managed entirely: there is a piece of code that traverse the IR and schedule the execution of these passes on an IR unit, this management hasn't really changed.

There isn't actually a piece of code. It's distributed across many different pieces of code. For example, CGSCC "pass manager" is completely outside the core pass management library (it happens to be a template specialization of PassManager, but arguably would be cleaner if it wasn't, but Chandler wanted to avoid a virtual call or something; such is modern C++)

It's like if I have a GUI library and have a GUI element that stacks other GUI elements vertically. One wouldn't call it a "WidgetManager". It would be named it based on what it does, which is stack widgets vertically (and in this case for "pass management", it's to run a sequence of passes). It's the same object oriented object recursion pattern. A "WidgetManager" to me implies a single object that has global visibility of all widgets (think like a FontManager that has the responsibility of caching/recalculating all glyphs for all fonts in an application).

I don't really get your point: I still see a difference between the management and the passes themselves: the 1, 2, and 3 is exactly this.

I'm just speaking from experience here -- I've had an intern ask "well, what 'management' does it actually do? it just runs a sequence of passes. It doesn't 'manage' anything." (referring to the LLVM new PM "PassManager")

I'm just speaking from experience here -- I've had an intern ask "well, what 'management' does it actually do? it just runs a sequence of passes. It doesn't 'manage' anything." (referring to the LLVM new PM "PassManager")

Yeah well, that's what a "pass manager" is in the compiler literature AFAIK, I can't do much for folks who come from a non-compiler background and have a different association with the terminology "manager" or "management".

silvas added inline comments.Wed, Sep 2, 11:10 AM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

This pass needs a getDependentDialects for any passes it might run.

mehdi_amini added inline comments.Wed, Sep 2, 1:35 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

True: so it can't run for any pass that require a new dialect (like a conversion pass), but since it is just used in unit-testing we don't really care right?

silvas added inline comments.Wed, Sep 2, 2:25 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

I think it's an important design aspect that we should at least have an answer to, and exhibit it with this test. We run into a situation analogous to this in IREE (and I suspect anyone using this feature will too), so it would be good to address it.

Can we do the parsePipeline in the constructor, and then implement getDependentDialects by recursively calling getDependentDialects?

mehdi_amini added inline comments.Wed, Sep 2, 3:32 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

I think it's an important design aspect that we should at least have an answer to,

I have an answer, it just does not please IREE ;)

and exhibit it with this test.

I don't consider it a bug though.

We run into a situation analogous to this in IREE (and I suspect anyone using this feature will too), so it would be good to address it.

I don't quite get the problem actually, I can trivially fix it here, it'll just be ugly because taking a string it totally "unconstrained".

silvas added inline comments.Wed, Sep 2, 3:57 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

I don't quite get the problem actually, I can trivially fix it here, it'll just be ugly because taking a string it totally "unconstrained".

I think that parsing the pipeline in the constructor and using that to implement getDependentDialects is not too ugly?

Rebase and add a getDependentDialects() in the Test pass

rriddle added inline comments.Wed, Sep 2, 5:05 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

Parsing a pipeline is not something I would encourage in a constructor, at least not in any production compiler. That relies on a pass registry, and I would expect that real toolchains have a structured way of populating these "dynamic" pass pipelines.

rriddle added inline comments.Wed, Sep 2, 5:11 PM
mlir/include/mlir/Pass/Pass.h
167

nit: invoke -> invoked at any time in a pass to dynamically schedule more passes.

mlir/include/mlir/Pass/PassManager.h
39

Is this used in this file?

mlir/lib/Pass/Pass.cpp
364

nit: Return this directly, it converts to failure.

370

Hmmm, this will prevent us from running multiple dynamic pipelines concurrently. Such a thing would happen if we tried to map the current inliner use case to this framework. Nesting an analysis manager is not thread-safe ATM.

842

If it isn't a direct parent, I would expect that we properly nest each parent. Otherwise, we break the tree structure of the analysis manager.

mlir/test/lib/Transforms/TestDynamicPipeline.cpp
28

Why do we need this here? This is just a test, I would remove it. There is no value to having this here.

GMNGeoffrey added inline comments.
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
56–60

Now that https://github.com/llvm/llvm-project/commit/c0b6bc070e78cbd20bc4351704f52d85192e8804 is in, how about moving this to the constructor so it can be used in getDepedentDialects?

mehdi_amini added inline comments.Thu, Sep 3, 10:36 AM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
56–60

There is a subtlety: it is built with the currently visited operation here, which isn't fixed for this pass and so not available in the ctor.

Also... why? This sole purpose of this pass is to test the codepath in the pass manager.

silvas accepted this revision.Tue, Sep 8, 2:37 PM

LGTM from my side

This revision is now accepted and ready to land.Tue, Sep 8, 2:37 PM
silvas added inline comments.Tue, Sep 8, 2:45 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

So how about we just don't use a textual pipeline in this example and just hardcode a set of passes to run? What value is being added to this test from using a textual pipeline?

mehdi_amini added inline comments.Tue, Sep 8, 3:19 PM
mlir/test/lib/Transforms/TestDynamicPipeline.cpp
25

The only reason for this pass to exist is to test the variation on the dynamic dispatch: the options on this pass really don't make sense (there is even an option run-on-parent that should always make it fail!)

Right now the tests are relying on being able slightly tweak the pipeline I'm passing in, I could probably get around it if you feel it is important. I may have to do add an option --use-pipeline=1 / --use-pipeline=2 to switch between two predefined one.

This revision was automatically updated to reflect the committed changes.