Page MenuHomePhabricator

[mlir][Pass] Make PassManager default to op-agnostic
ClosedPublic

Authored by rkayaith on Nov 9 2022, 12:36 PM.

Details

Summary

Currently PassManager defaults to being anchored on builtin.module.
Switching the default makes PassManager consistent with
OpPassManager and avoids the implicit dependency on builtin.module.

Specifying the anchor op type isn't strictly necessary when using
explicit nesting (existing pipelines will continue to work), but I've
updated most call sites to specify the anchor since it allows for better
error-checking during pipeline construction.

Diff Detail

Event Timeline

rkayaith created this revision.Nov 9 2022, 12:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
rkayaith updated this revision to Diff 474374.Nov 9 2022, 2:40 PM

fix for doc change

rkayaith published this revision for review.Nov 9 2022, 2:41 PM
rkayaith added a reviewer: mehdi_amini.
rriddle added inline comments.Nov 9 2022, 2:49 PM
mlir/include/mlir/Pass/PassManager.h
211–220

Can you add a utility template class that takes the operation type? It'd be nice to preserve the simplicity when the anchor type is known (common for real pipelines), e.g.:

template <typename OpType>
class PassManagerOn : public PassManager ...

That would remove the need for most of the "dialect.opname"/"Op::getOperationName()"/"getName" calls when building a known passmanager.

rkayaith updated this revision to Diff 474561.Nov 10 2022, 8:54 AM
rkayaith marked an inline comment as done.
rkayaith edited the summary of this revision. (Show Details)

add PassManagerOn class

rkayaith added inline comments.Nov 10 2022, 8:57 AM
mlir/include/mlir/Pass/PassManager.h
211–220

I added this + ModulePassManager as an alias, though I think there's a potential footgun since it's possible to change the anchor after the pass manager is created, e.g. https://github.com/llvm/llvm-project/blob/ecab1bc0dcdc04ec863f7aa3eaa5daad8232ba65/mlir/lib/Pass/PassCrashRecovery.cpp#L477-L480

rriddle accepted this revision.Nov 15 2022, 12:25 PM
rriddle added inline comments.
mlir/include/mlir/Pass/PassManager.h
211–220

Good point there. We could have a run method in PassManagerOn that verifies that the anchor is actually correct. We could also just have a templated construction method that returns a pass manager? Just making sure we don't lose the niceties of the API when we don't want "any" anchor.

466–467

I'd just drop any special usings for ModuleOp, it helps keep the API free from builtin deps.

This revision is now accepted and ready to land.Nov 15 2022, 12:25 PM
rkayaith planned changes to this revision.Dec 29 2022, 12:33 PM

I plan to rebase this + switch to a constructor method (static PassManager::on<T>(MLIRContext*) -> PassManager)

rkayaith updated this revision to Diff 491206.Sun, Jan 22, 2:25 PM
rkayaith edited the summary of this revision. (Show Details)

Switch from PassManagerOn<T> to constructor method

This revision is now accepted and ready to land.Sun, Jan 22, 2:25 PM
This revision was automatically updated to reflect the committed changes.