This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Pass] Include anchor op in -pass-pipeline
ClosedPublic

Authored by rkayaith on Sep 29 2022, 1:16 PM.

Details

Summary

In D134622 the printed form of a pass manager is changed to include the
name of the op that the pass manager is anchored on. This updates the
-pass-pipeline argument format to include the anchor op as well, so
that the printed form of a pipeline can be directly passed to
-pass-pipeline. In most cases this requires updating
-pass-pipeline='pipeline' to
-pass-pipeline='builtin.module(pipeline)'.

This also fixes an outdated assert that prevented running a
PassManager anchored on 'any'.

I've split out the mechanical test updates into D136193, but will
combine them into one commit before landing.

Diff Detail

Event Timeline

rkayaith created this revision.Sep 29 2022, 1:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
rkayaith updated this revision to Diff 464827.Oct 3 2022, 2:55 PM
rkayaith edited the summary of this revision. (Show Details)

update description

rkayaith updated this revision to Diff 464844.Oct 3 2022, 3:42 PM

add extra test

rkayaith published this revision for review.Oct 3 2022, 3:44 PM
rkayaith added a reviewer: mehdi_amini.
rkayaith updated this revision to Diff 465125.Oct 4 2022, 12:56 PM
rkayaith edited the summary of this revision. (Show Details)

add more info to description

The approach used here currently (add a new option instead of changing pass-pipeline) has the downside that pass-pipeline used to be symmetric with the printed form, but no longer is. If we wanted to preserve that, some options I see:

Any thoughts on how to proceed here?

The approach used here currently (add a new option instead of changing pass-pipeline) has the downside that pass-pipeline used to be symmetric with the printed form, but no longer is. If we wanted to preserve that, some options I see:

Any thoughts on how to proceed here?

I think we should proceed with changing -pass-pipeline. I'd rather have fewer things than more things. We've also already had people confused on the implicit nature of the anchor op in pass-pipeline before, and it seems better to just have that option represent the full pipeline.

I'm aligned with River on the goal here.

rkayaith updated this revision to Diff 467025.Oct 11 2022, 10:49 PM
rkayaith retitled this revision from [mlir-opt] Add a '-pass-manager' option to [mlir][Pass] Include anchor op in -pass-pipeline.
rkayaith edited the summary of this revision. (Show Details)

Rework patch to change the -pass-pipeline option instead. Other than
pipeline-parsing.mlir all the test updates here are mechanical
(`-pass-pipeline='pipeline' -> -pass-pipeline='builtin.module(pipeline)')

Did you also scrub the docs for uses of pass-pipeline?

mlir/lib/Pass/Pass.cpp
780–784

Please avoid using if initializers with nullable things. Can you also spell out auto here?

rkayaith updated this revision to Diff 467225.Oct 12 2022, 12:20 PM
rkayaith marked an inline comment as done.

rebase, update docs, address comment

rkayaith updated this revision to Diff 467274.Oct 12 2022, 3:11 PM

unnecessary 'llvm::'

rkayaith updated this revision to Diff 468697.Oct 18 2022, 2:21 PM
rkayaith edited the summary of this revision. (Show Details)

move mechanical test updates into a separate diff

rriddle accepted this revision.Oct 20 2022, 6:09 PM
This revision is now accepted and ready to land.Oct 20 2022, 6:09 PM
This revision was automatically updated to reflect the committed changes.