[RFC-ish][MIR] Add a pass manager #31448

nagisa · 2016-02-06T12:46:13Z

Previously we would run MIR passes in a very ad-hoc way by calling passses directly everywhere across the compiler. This PR adds a proper pass manager which will run all the passes in some order (decided by the priority method in Pass) right after the MIR is constructed. I believe the pass manager should be general enough to be able to accommodate passes as complex as MIR-based borrowck.

The next step here would be to add something like -Z mir-opt-level=[0..] defaulting to 1 + -C opt-level and a fn should_run(&Session) so the passes could decide whether they want to run under a given optimisation level or not.

Depends on #31425 landing first.

r? @nikomatsakis

nikomatsakis · 2016-02-10T01:59:15Z

cc @rust-lang/compiler -- I'd be curious to get others' take here.

nrc · 2016-02-10T02:53:57Z

I'm somewhat aghast that mir pass plugins have sneaked into the compiler with no discussion, I think that is a bit premature.

Anyway, looking at this PR, I'm in favour of reducing the ad-hoc calling of passes, but this feels like unnecessary flexibility and complexity.

oli-obk · 2016-02-10T09:03:05Z

src/librustc/mir/transform.rs

+
+/// Pass which only inspects basic blocks in MIR.
+///
+/// Invariant: The blocks are considered to be fully self-contained for the purposes of this pass –


Why do we need this invariant? For the MirPass implementor it's irrelevant, as it's the same as making sure that the visitor and the run_pass method don't modify the Mir object. For the compiler there's no advantage to knowing this as I see it.

nagisa · 2016-02-10T14:17:07Z

Anyway, looking at this PR, I'm in favour of reducing the ad-hoc calling of passes, but this feels like unnecessary flexibility and complexity.

I’ve modelled the passes to match the LLVM’s pass structure somewhat closely – namely their ModulePass, FunctionPass and BasicBlockPass. The pass manager itself is pretty barebones currently and I would expect it to grow considerably over time to e.g. share dataflow analysis information between passes.

What I’m currently worried about the most, after sleeping on it for a while, is that the passes might not be flexible enough yet (for e.g. @arielb1’s typeck pass).

nikomatsakis · 2016-02-10T17:24:11Z

On Tue, Feb 09, 2016 at 06:54:29PM -0800, Nick Cameron wrote:

I'm somewhat aghast that mir pass plugins have sneaked into the compiler with no discussion, I think that is a bit premature.

I was also surprised.

arielb1 · 2016-02-10T17:45:06Z

Are there any users for MirBlockPass?

nagisa · 2016-02-10T17:47:50Z

@arielb1 currently only the test introduced with #31425.

arielb1 · 2016-02-10T17:48:44Z

I mean, what would you use it for?

nagisa · 2016-02-10T17:59:13Z

@arielb1 I was initially considering it to be a good fit for:

A pass which removes the unwind target in case of -Z no-landing-pads;
Unnecessary drop removal pass;

The drop removal pass, however, would rely on passes being able to share the analysis results with other passes, namely the liveness checker.

I think typeck pass could also use it, provided we happen to grow support for something similar to doInitialization. Lack of such a method is one of the things I had in mind when I was saying that the current implementation might not be flexible enough in its current form.

EDIT: to summarize, I, myself, think that in current state, the MirBlockPass is not very useful.

nagisa · 2016-02-10T18:43:17Z

src/librustc/mir/transform.rs

+}
+
+/// Pass which only inspects MIR of distinct functions.
+pub trait MirPass: Pass {


The 'tcx lifetime might be better on the trait itself.

arielb1 · 2016-02-10T20:18:52Z

I was kind-of working on an unnecessary drop removal as a part of non-zeroing drop, but I think that work is going to be stuck for a while. Of course, that would probably want to be a full function pass because of the dataflow/analysis.

nikomatsakis · 2016-02-11T20:59:20Z

OK so I think these are my overall thoughts:

Having more flexibility in the kind of pass seems good, and the various choices here some convenient. I like being able to implement the right trait for whatever I want.
That said, I'm reluctant to add kinds of passes without concrete consumers. Feels maybe like premature abstraction?
I am wary of integer priorities (as I wrote earlier).
- I'd rather have a central listing declaring the ordering in which passes will run, personally, so that we can just look at it and know what is going to happen.
- But then I think you said this was based on whatever LLVM is doing, and maybe it works well for them, I don't know. But it also feels like a case where I'd rather wait to feel the pain before addressing the problem.
- To accommodate plugin passes, I'd make some "meta passes" at defined points that invoke plugin code.
  - But see https://internals.rust-lang.org/t/mir-compiler-plugins-for-custom-mir-passes/3166/ for more on that topic.

(It may be worth writing an RFC on this, but it also feels like overkill.)

nagisa · 2016-02-11T21:04:03Z

But then I think you said this was based on whatever LLVM is doing, and maybe it works well for them, I don't know. But it also feels like a case where I'd rather wait to feel the pain before addressing the problem.

Only the pass hierarchy (MirMapPass, MirPass, ...) is based on LLVM. For pass ordering LLVM is using a full-blown dependency graph.

nagisa · 2016-02-11T21:09:33Z

I’m fine with stripping this PR to a bare minimum so only the barebones pass manager (executes pass in order they’ve been pushed), MirPass and MirMapPass stay (these all are useful). I’ll adjust the PR accordingly… someday.

Tiered passes allow to avoid some of the introspection of MIR map for passes which do not need it and perhaps improve performance of the more-restricted passes (i.e. BlockPasses could be run in parallel regardless of function for which the block belongs). It also could allow to e.g. run dead block removal only after passes which modify the CFG graph and not after those which only change blocks.

Previously SimplifyCfg pass was unecessarily interleaving many different functions – actual simplification, dead block removal and compaction. This patch splits off dead block removal and compaction into their own passes.

nikomatsakis · 2016-02-18T19:18:07Z

The simplified version looks nice @nagisa! I see though that travis fails with the following:

---- [run-pass] run-pass-fulldeps/mir-pass.rs stdout ----

error: auxiliary build of "/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs" failed to compile: 
status: exit code: 101
command: x86_64-unknown-linux-gnu/stage2/bin/rustc /home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs -L x86_64-unknown-linux-gnu/test/run-pass-fulldeps/ --target=x86_64-unknown-linux-gnu --crate-type=dylib -L x86_64-unknown-linux-gnu/test/run-pass-fulldeps/mir-pass.stage2-x86_64-unknown-linux-gnu.run-pass.libaux -C prefer-dynamic --out-dir x86_64-unknown-linux-gnu/test/run-pass-fulldeps/mir-pass.stage2-x86_64-unknown-linux-gnu.run-pass.libaux --cfg rtopt -C rpath -O -L x86_64-unknown-linux-gnu/rt
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:21:35: 21:47 error: unresolved import `rustc::mir::transform::MirBlockPass`. There is no `MirBlockPass` in `rustc::mir::transform` [E0432]
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:21 use rustc::mir::transform::{self, MirBlockPass};
                                                                                                            ^~~~~~~~~~~~
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:21:35: 21:47 help: run `rustc --explain E0432` to see a detailed explanation
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:34:5: 36:6 error: method `priority` is not a member of trait `transform::Pass` [E0407]
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:34     fn priority(&self) -> usize {
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:35         1000
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:36     }
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:34:5: 36:6 help: run `rustc --explain E0407` to see a detailed explanation
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:39:6: 39:18 error: `MirBlockPass` is not a trait [E0404]
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:39 impl MirBlockPass for Pass {
                                                                               ^~~~~~~~~~~~
/home/travis/build/rust-lang/rust/src/test/auxiliary/dummy_mir_pass.rs:39:6: 39:18 help: run `rustc --explain E0404` to see a detailed explanation
error: cannot continue compilation due to previous error

------------------------------------------

thread '[run-pass] run-pass-fulldeps/mir-pass.rs' panicked at 'explicit panic', /home/travis/build/rust-lang/rust/src/compiletest/runtest.rs:1529
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [run-pass] run-pass-fulldeps/mir-pass.rs

bors · 2016-02-18T23:25:45Z

☔ The latest upstream changes (presumably #31600) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2016-02-23T12:29:32Z

This will be a pain to rebase. I’ll submit a new one later.

@nikomatsakis

Add Pass manager for MIR A new PR, since rebasing the original one (#31448) properly was a pain. Since then there has been several changes most notable of which: 1. Removed the pretty-printing with `#[rustc_mir(graphviz/pretty)]`, mostly because we now have `--unpretty=mir`, IMHO that’s the direction we should expand this functionality into; 2. Reverted the infercx change done for typeck, because typeck can make an infercx for itself by being a `MirMapPass` r? @nikomatsakis

rust-highfive assigned nikomatsakis Feb 6, 2016

nagisa force-pushed the mir-tiered-passes branch 2 times, most recently from 665e9a0 to 530beae Compare February 7, 2016 18:58

nagisa mentioned this pull request Feb 7, 2016

[MIR] A pass to type-check MIR #31474

Merged

nagisa force-pushed the mir-tiered-passes branch 2 times, most recently from c2cc646 to b1d9e88 Compare February 9, 2016 23:16

oli-obk reviewed Feb 10, 2016
View reviewed changes

nagisa changed the title ~~[MIR] Add a pass manager~~ [RFC-ish][MIR] Add a pass manager Feb 10, 2016

nagisa reviewed Feb 10, 2016
View reviewed changes

nagisa added 5 commits February 16, 2016 00:20

Reuse MIR visitors for EraseRegions pass

da8a388

Split SimplifyCfg pass into smaller passes

af69f95

Previously SimplifyCfg pass was unecessarily interleaving many different functions – actual simplification, dead block removal and compaction. This patch splits off dead block removal and compaction into their own passes.

Factor out MIR printer into a pass

363ac0f

Address comments

bd4ddf3

nagisa force-pushed the mir-tiered-passes branch from b1d9e88 to bd4ddf3 Compare February 15, 2016 23:35

nagisa closed this Feb 23, 2016

nagisa mentioned this pull request Feb 26, 2016

Add Pass manager for MIR #31916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC-ish][MIR] Add a pass manager #31448

[RFC-ish][MIR] Add a pass manager #31448

nagisa commented Feb 6, 2016

nikomatsakis commented Feb 10, 2016

nrc commented Feb 10, 2016

oli-obk Feb 10, 2016

nagisa commented Feb 10, 2016

nikomatsakis commented Feb 10, 2016

arielb1 commented Feb 10, 2016

nagisa commented Feb 10, 2016

arielb1 commented Feb 10, 2016

nagisa commented Feb 10, 2016

nagisa Feb 10, 2016

arielb1 commented Feb 10, 2016

nikomatsakis commented Feb 11, 2016

nagisa commented Feb 11, 2016

nagisa commented Feb 11, 2016

nikomatsakis commented Feb 18, 2016

bors commented Feb 18, 2016

nagisa commented Feb 23, 2016

[RFC-ish][MIR] Add a pass manager #31448

[RFC-ish][MIR] Add a pass manager #31448

Conversation

nagisa commented Feb 6, 2016

nikomatsakis commented Feb 10, 2016

nrc commented Feb 10, 2016

oli-obk Feb 10, 2016

Choose a reason for hiding this comment

nagisa commented Feb 10, 2016

nikomatsakis commented Feb 10, 2016

arielb1 commented Feb 10, 2016

nagisa commented Feb 10, 2016

arielb1 commented Feb 10, 2016

nagisa commented Feb 10, 2016

nagisa Feb 10, 2016

Choose a reason for hiding this comment

arielb1 commented Feb 10, 2016

nikomatsakis commented Feb 11, 2016

nagisa commented Feb 11, 2016

nagisa commented Feb 11, 2016

nikomatsakis commented Feb 18, 2016

bors commented Feb 18, 2016

nagisa commented Feb 23, 2016