Page MenuHomePhabricator

[mlir] Include anchor op when printing pass managers
ClosedPublic

Authored by rkayaith on Sep 25 2022, 9:06 PM.

Details

Summary

Previously a pipeline nested on anchor-op would print as just
'pipeline', now it will print as 'anchor-op(pipeline)'. This ensures
the text form includes all information needed to reconstruct the pass
manager.

Diff Detail

Event Timeline

rkayaith created this revision.Sep 25 2022, 9:06 PM
Herald added a project: Restricted Project. · View Herald Transcript
rkayaith updated this revision to Diff 462807.Sep 25 2022, 9:51 PM
rkayaith retitled this revision from [mlir] Inlude anchor op when printing pass managers to [mlir] Include anchor op when printing pass managers.

clang-format

rkayaith updated this revision to Diff 463392.Sep 27 2022, 6:56 PM
rkayaith edited the summary of this revision. (Show Details)

update description

rkayaith published this revision for review.Sep 27 2022, 7:19 PM
rriddle accepted this revision.Sep 29 2022, 12:16 AM
This revision is now accepted and ready to land.Sep 29 2022, 12:16 AM
mehdi_amini requested changes to this revision.Sep 29 2022, 12:33 AM

I'm not sure I understand the behavior right now, but the test changes look concerning at first look.

mlir/test/python/pass_manager.py
49

The behavior here is not clear to me: we're not really round-tripping anymore? Seems like some asymmetry on the parser/printer.

This revision now requires changes to proceed.Sep 29 2022, 12:33 AM
rkayaith added inline comments.
mlir/test/python/pass_manager.py
49

Yes this is one of the places that builtin.module is baked into the C/python api, when parsing a pass manager it always creates a pm anchored on builtin.module, parses the string as a pipeline, and appends it to the pm. Printing the anchor op just makes the implicit builtin.module apparent.

I think it should be updated to round-trip this new printed format properly, but that would break existing usage; I can look into that if @stellaraccident doesn't plan to already.

mehdi_amini added inline comments.Sep 29 2022, 1:02 PM
mlir/test/python/pass_manager.py
49

OK for Python but what about the others above?

rkayaith added inline comments.Sep 29 2022, 1:33 PM
mlir/test/python/pass_manager.py
49

the C api roundtripping is the same issue as python, and then the other one where -pass-pipelineis asymmetric with the printed form is because -pass-pipeline is really -append-pipeline-to-current-passmanager; it can be specified multiple times and interleaved with the --pass-name options, so including the anchor op in the string doesn't really make sense. It seemed better to me to leave that how it is, and instead add a new option which includes the anchor op (wip here: https://reviews.llvm.org/D134900), which would be symmetric with the printed form.

And then one other place I noticed was the reproducer pipeline string not including the anchor op, wip patch here: https://reviews.llvm.org/D134623

rkayaith added inline comments.Oct 3 2022, 3:45 PM
mlir/test/python/pass_manager.py
49

@mehdi_amini any remaining concerns here?

mehdi_amini added inline comments.Oct 3 2022, 4:51 PM
mlir/test/python/pass_manager.py
49

I guess I'd rather simplify the --pass-pipeline flow to be self-contained.

I don't see a reason to allow mix-and-match of the -pass-name form (which is a bit underspecified in itself and should be reserved to isolated test cases) and the --pass-pipeline form.

I'd look into making --pass-pipeline be the exact specification of the pipeline, without any extra implicit nesting, and forbid any other --pass-name option along with it. Ultimately we should have all this trivially symmetric.

rkayaith added inline comments.Oct 3 2022, 9:58 PM
mlir/test/python/pass_manager.py
49

Sure that's doable, I was mainly concerned about changing the behaviour of a commonly-used option. The new behaviour would also be pretty verbose in some cases, e.g. if you convert a pass from a FunctionOp pass to a FunctionOpInterface pass, running it goes from -my-pass to -pass-pipeline='builtin.module(func.func(my-pass))' because of the implicit module op.

Anyways, what do you think about merging this change and continuing discussion of -pass-pipeline on https://reviews.llvm.org/D134900?

mehdi_amini added inline comments.Oct 4 2022, 1:10 AM
mlir/test/python/pass_manager.py
49

I rather get some agreement on a plan first right now: these can stay independent changes after we get to a plan. Otherwise I'm slightly concerned about not converging and what kind of intermediate state we'll end-up in.

rkayaith added inline comments.Oct 4 2022, 1:30 PM
mlir/test/python/pass_manager.py
49

fair enough, I commented about this on D134900, please take a look

rkayaith added inline comments.Oct 17 2022, 12:28 PM
mlir/test/python/pass_manager.py
49

seems there's agreement on the end state, @mehdi_amini is this good to go in now?

rkayaith updated this revision to Diff 468661.Oct 18 2022, 12:29 PM

rebase to try and fix patch application in stack

mehdi_amini added inline comments.Oct 19 2022, 6:00 PM
mlir/test/python/pass_manager.py
49

So all of these test changes will be reverted later at the end of your work?

mehdi_amini added inline comments.Oct 19 2022, 6:02 PM
mlir/test/python/pass_manager.py
49

I think I see in https://reviews.llvm.org/D134900 that you're adjusting for some things, but not all (like the python tests)

rkayaith added inline comments.Oct 19 2022, 6:24 PM
mlir/test/python/pass_manager.py
49

the test changes won't be reverted, instead the python and C api will need a change similar to D134900 that makes the anchor op explicit when parsing. i'll work on patches for that.

rkayaith added inline comments.Oct 19 2022, 7:13 PM
mlir/test/python/pass_manager.py
49

ah actually I see what you mean, yes some of this will end up reverted

mehdi_amini accepted this revision.Oct 19 2022, 7:34 PM
mehdi_amini added inline comments.
mlir/test/python/pass_manager.py
49

LG

This revision is now accepted and ready to land.Oct 19 2022, 7:34 PM
This revision was automatically updated to reflect the committed changes.