Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXME] settle future of MIR pass manager #41712

Closed
nikomatsakis opened this issue May 2, 2017 · 5 comments
Closed

[FIXME] settle future of MIR pass manager #41712

nikomatsakis opened this issue May 2, 2017 · 5 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

In #41625, the role of the MIR pass manager has been significantly scaled back. Once can still register passes, but they always take an &mut Mir, and the set of passes is defined in driver and cannot be readily changed (in particular, the session no longer has a Passes struct). @eddyb would like to go further, I think, and remove the Passes strruct from the tcx altogether -- the main reason it is still there is that it allows the MIR code to (indirectly) invoke passes defined in other crates, e.g. elaborate_drops. I tend to also think it's nice to "collect" the MIR passes into one single data-structure to ensure that (e.g.) we have the option to dump the MIR in between each pass and apply other debugging conventions (as well as to easily reorder passes or introduce new ones).

Longer term, what do we think this should look like? One extreme future might remove the Passes struct altogether and have the invocations of each pass kind of hard-coded -- we could build up the set of Pass objects in the various "mir queries", for example. Another extreme would be to make the Passes struct more complex to support plugins adding custom optimizations -- it seems like we're a long way from there, and that may never make sense.

To do the "radically scaled back" version of futures, we'd have to at least consolidate the borrowck and mir crates, or make one a dependency of the other, or something like that.

cc @rust-lang/compiler -- thoughts?

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 2, 2017
@eddyb
Copy link
Member

eddyb commented May 2, 2017

IMO elaborate_drops should just be in librustc_mir. I believe most of it already is.

@nikomatsakis
Copy link
Contributor Author

@eddyb

IMO elaborate_drops should just be in librustc_mir. I believe most of it already is.

I think I agree. We can probably just merge librustc_borrowck and librustc_mir, for that matter, particularly once borrowck is operating on MIR.

@eddyb
Copy link
Member

eddyb commented May 2, 2017

@nikomatsakis Particularly, I would've expected @pnkfelix to experiment with the new borrowck outside of librustck_borrowck and preferably in librustc_mir but I suppose we still don't have a lot of precedent in doing analysis on MIR there, or at all.

@nagisa
Copy link
Member

nagisa commented May 3, 2017

Longer term, what do we think this should look like? One extreme future might remove the Passes struct altogether and have the invocations of each pass kind of hard-coded -- we could build up the set of Pass objects in the various "mir queries", for example. Another extreme would be to make the Passes struct more complex to support plugins adding custom optimizations -- it seems like we're a long way from there, and that may never make sense.

So when I was implementing the MirPass stuff, I was imagining one would eventually be able to declaratively specify dependencies between the passes and the pass manager would sort of figure out the ideal order of the passes for each function, not unlike passes are handled in LLVM. All this falls out pretty nicely out of the query interface. So IME the future would look something like this, maybe?

fn query_optimised_mir(did: DefId) { if is_function(did) { query_inlined_mir(did) } else { query_constevaluated_mir(did) }
fn query_inlined_mir(did: DefId) { ... do inlining on mir returned by `query_some_other_mir` ... }
fn query_some_other_mir(did: DefId) { fn get_base_mir(did) }

There are a few things to be careful about. For example currently we have a number of distinct invocations of SimplifyCfg. We currently have a case like this:

        passes.push_pass(box borrowck::ElaborateDrops);
        passes.push_pass(box mir::transform::no_landing_pads::NoLandingPads);
        passes.push_pass(box mir::transform::simplify::SimplifyCfg::new("elaborate-drops"));

Here SimplifyCfg has to run after both ElaborateDrops and NoLandingPads; it is fine if it doesn’t run between ElaborateDrops and NoLandingPads. If for some reason query chain ends up not running NoLandingPads here, the SimplifyCfg should still run after ElaborateDrops. I do not know how one would encode such requirement in a simple way. It seems to me like there has to be some code somewhere that’s smarter than just running a pass on MIR and handing out the result.

Perhaps a lazy thunk approach could be used? Query interface builds up the plan to run the passes and the optimal way to run all the passes could then be found when the thunk is forced (when a fully optimised mir is requested)… or something like that anyway.


Now, everything I said here assumes we still want to implement a number of "real" optimisations targetting MIR. IME idea of such optimisations is falling out of favour. At least I do not see MIR as a good target for full blown optimisations anymore.

@nikomatsakis
Copy link
Contributor Author

I don't know that dependencies "fall out" from the query interface in particular -- first off, these are "linear" queries (since we don't want to be copying the MIR interface). So if you "demand" the result of a query, and someone else does, you'll get an error (this all depends on how we model linear queries, I suppose, which is #41710).

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 27, 2017
bors added a commit that referenced this issue Nov 14, 2017
rustc_mir: hardcode pass list internally and remove premature pluggability.

Fixes #41712 by moving the MIR pass lists from `rustc_driver` to `rustc_mir`.
The application of the passes is done with the `rustc_mir::transform::run_passes` macro, which is public, as are all the passes AFAIK, and can be used to apply MIR passes outside of `rustc_mir`.

With the ability to override query providers through the `rustc_driver` (orthogonal to, and not included in this PR), custom drivers will be able to substitute the entire pass list if they want to.
**EDIT**: the aforementioned ability is added by #45944.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants