-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rework the queries for the MIR pipeline #41625
rework the queries for the MIR pipeline #41625
Conversation
You can't really do this, not if you have linear queries, which is what I wanted to do. |
That's true, I don't have linear queries, I have stealable queries, so you can read the result until they are stolen. |
There are no clones, though, or at least not deep ones. |
Yeah, I meant, without introducing a potentially fragile abstraction like |
@eddyb and I had a long conversation on IRC. Just to leave some record of some of what we said:
What we agreed was I should remove support for inlining, which would let me simplify to one query per suite (they would yield stealable results, except the last one). Within a suite, we can run the passes (and trigger the dump mir etc) in a simple loop. This was the design I was heading for before I decided to try and support inlining. I am happy though to do something simple and then think about how to best support inlining and other such things in follow-up work. |
To add to that: I'm not even sure we need a complex IPO manager for inlining: querying |
☔ The latest upstream changes (presumably #41593) made this pull request unmergeable. Please resolve the merge conflicts. |
OK, I spent some more time thinking about this over the weekend. I just want to lay out the various designs that are under discussion a bit more as well as their pros/cons. @eddyb I'd really like if you can double-check that my description of your proposed plans around linear maps is accurate. On the topic of the MIR optimization manager
This is true only if we do not plan to do any further optimization after inlining (which we certainly do -- part of the point of inlining is to unlock other optimizations). That is, If we adopted the approach of having "IPO" being a distinct suite of optimizations, then I envision it working like this. Every "suite" of optimizations would have a query named after it. There aren't many so I'd probably just write the queries by hand. There would probably be 4 suites, then:
The idea would be that if the MIR optimization level is not 2, then If we wish to get better incremental re-use with opt-level 2, there are two known approaches: we could apply the partitionings we do for LLVM earlier or we could refactor the optimization passes to produce intermediate steps to which we can apply the red-green algorithm (e.g., one might use this to check if the SCCs have changed, and if not we could perhaps reuse the post-inlining MIR for an entire SCC). There may yet be better approaches here, although I suspect that a lot of that work will be factoring things well so that red-green applies. Seems like an area we will need to explore over time. On the topic of "multi" queriesOver the weekend, @eddyb and I were chatting about the multi-queries that I added here. He is worried (iiuc) that the current system does not guarantee "coherence". In particular, the way I have it setup, if you query He would prefer some kind of system where you don't just return multiple values, but instead express (somehow) the "dependencies" between keys, so that the system can ensure that, regardless of which query you ask for first (A, B, or C), it will generate the results in the same way. This seems like a nice thing to guarantee, but I am not sure of its importance. It'd be good (I think) to drill into more specific use cases. I know of various few uses for multi-queries:
Looking at these, I see two overall patterns:
On the topic of "linear" queriesI modeled linear queries as ordinary queries that return a In all the MIR cases, at least, there really isn't much to intercoordinate. For the most part, each MIR optimization is just used by one other query: the next optimization in the sequence. (In the current PR, each optimization pass has its own query, but if we convert to just having a query-per-suite, then this would apply at the level of suites.) Other parts of the compiler should use one of the queries (e.g., However, there are a few exceptions. One example is const qualification. This is a lightweight scan of the contents of a To make the example a bit more abstract, what we have here is a "stealable" query A that needs to be read by query B but stolen by query C. Under my system, if you steal a query, you must be aware of all possible readers and ensure that they are forced before you do so. I believe @eddyb had in mind a different system, which I will call linear queries. In that scheme, once you execute a query once, and further attempt to request the same query is a bug. So we can't have a query A that is read by B and consumed by C, as I had, since both B and C must consume, and that would violate linearity. To support this scenario, then, eddyb wanted to introduce "conjoined" providers (name is mine). Basically, when setting up the fn produce_b(tcx: TyCtxt, def_id: DefId, a: &A) -> (B, C) { }
fn produce_c(tcx: TyCtxt, def_id: DefId, a: A) -> (B, C) { }
// this method will initialize `providers.b` and `providers.c`
// with generated glue functions:
providers.conjoin().take_a().read(produce_b).consume(produce_c); Under the hood, the The major advantage of the linear scheme that I see is that it will more robustly fail if you screw it up. That is, to compare:
In particular, in the stealable scheme, if you screw it up, things may still work when both queries are used, as long as they are used in the right order. In the linear case, if you fail to use conjoin, it will fail. This is definitely good. However, it's also true that the linear scheme requires more machinery, and that the Therefore, I at least feel pretty good about going forward with the stealable scheme. I'd be happy to see the linear scheme come into being as well at some point. TL;DROK, sorry for the detailed notes. Just wanted to jot down my current thoughts in detail. To my mind, the question of linear-vs-stealable can be deferred (as I noted), but it still remains to decide how to handle inlining. I feel... not great about having inlining in the codebase but not executable. I feel better about adopting the "suite-based" approach that I described in the first section. I can tinker with that today, I don't think it would be all that hard and it should let me pare down the amount of "framework" that the MIR optimization stuff contains, while still keeping all the benefits -- in fact, it improves on some of them, in that there would never be a need to deal directly with the concept of "stealable" MIR, all the tricky cases would be bound up in the suites. However, that would require keeping "multi-queries". Hmm, or perhaps I can use a different hack, i.e., having |
So I wrote this, but I forgot about the query cycle detection. After some discussion on IRC, we were thinking that a good way to handle inlining (maybe in this PR, maybe not...) would be to have it request the fully optimized form of callees using We will probably want to be using stacker or something else to permit deeper stacks here. |
09aded8
to
7df756c
Compare
@eddyb take a look at the latest edits and see what you think; not fully tested etc but all I have time for now. |
7df756c
to
74a93da
Compare
☔ The latest upstream changes (presumably #41611) made this pull request unmergeable. Please resolve the merge conflicts. |
6ec92d9
to
be32805
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments, but this is already leaner and inlining might just work with no further changes.
src/librustc/ty/mod.rs
Outdated
pub fn item_mir(self, did: DefId) -> Ref<'gcx, Mir<'gcx>> { | ||
self.mir(did).borrow() | ||
/// Given the did of an item, returns its (optimized) MIR, borrowed immutably. | ||
pub fn item_mir(self, did: DefId) -> &'gcx Mir<'gcx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this now in favor of just calling the query directly?
@@ -685,6 +697,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
pub fn create_and_enter<F, R>(s: &'tcx Session, | |||
local_providers: ty::maps::Providers<'tcx>, | |||
extern_providers: ty::maps::Providers<'tcx>, | |||
mir_passes: Rc<Passes>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are they an input and not hardcoded in librustc_mir
? My intuition is that they will have to be interacting with some sort of scheduler within a function, interleaving "passes" with inlining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for one thing, only drive knows all the names (e.g., elaborate drops is in borrowck
). We could make librustc_mir
depend on borrowck
, but I thought it was nice to have the set of mir passes that will execute be defined in the same place that we are generally defining the overall flow of the compiler code. (I guess that, more and more, that overall flow will be diffused through the system.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that is annoying, I did say something to @pnkfelix along the lines of moving the MIR code in borrowck in rustc_mir
. Unlike you I don't think we could sustain a more diffuse flow.
At least not on transformations, maybe only on analysis checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point about 'diffuse flow' was that as we move to queries, you can't really expect the driver source to tell you much about the order in which things happen. So in that sense we might as well define the passes in librustc_mir
.
As far as passes coming from "outside", I think eventually we will want to support this, but I am happy to put that day off for the time being. Mostly I like having the MIR passes execute in a common framework so we get uniform dumping of IR between passes and so forth (and a common numbering, etc).
/// - ready for constant evaluation | ||
/// - unopt | ||
/// - optimized | ||
pub const MIR_SUITES: usize = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hope we don't really need these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constants? We don't need them, we could use distinct variables instead of a Vec<Vec<>>
. But it just seems a bit silly to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, having them configurable at all.
src/librustc_mir/lib.rs
Outdated
mod hair; | ||
mod shim; | ||
pub mod mir_map; | ||
mod queries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really avoid calling a module that.
src/librustc_mir/queries.rs
Outdated
tcx.alloc_steal_mir(mir) | ||
} | ||
|
||
fn optimized_mir<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx Mir<'tcx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 queries should go in transform
IMO.
src/librustc_mir/shim.rs
Outdated
@@ -119,8 +117,6 @@ fn make_shim<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>, | |||
debug!("make_shim({:?}) = {:?}", instance, result); | |||
|
|||
let result = tcx.alloc_mir(result); | |||
// Perma-borrow MIR from shims to prevent mutation. | |||
mem::forget(result.borrow()); | |||
result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove the temporary variable.
src/librustc_mir/transform/inline.rs
Outdated
// inline. | ||
// | ||
// We use a queue so that we inline "broadly" before we inline | ||
// in depth. It is unclear if this is the current heuristic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/current/previous, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I meant by that sentence. I think best heuristic. It certainly is the current (and previous) heuristic, whether or not it was intended that way.
src/librustc_mir/transform/mod.rs
Outdated
}; | ||
} | ||
|
||
pub(crate) fn run_suite<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you moved those 3 entry points here, you'd definitely not need to make this public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 seems good.
@eddyb Should I interpret your "approval" as I was considering whether to remove the pass manager from the I was planning on also opening an issue to discuss the best design for linear queries (and probably adding a FIXME referencing it to |
b48f547
to
1a20104
Compare
@nikomatsakis I would prefer it if |
☔ The latest upstream changes (presumably #41702) made this pull request unmergeable. Please resolve the merge conflicts. |
Each MIR key is a DefId that has MIR associated with it
Overall goal: reduce the amount of context a mir pass needs so that it resembles a query. - The hooks are no longer "threaded down" to the pass, but rather run automatically from the top-level (we also thread down the current pass number, so that the files are sorted better). - The hook now receives a *single* callback, rather than a callback per-MIR. - The traits are no longer lifetime parameters, which moved to the methods -- given that we required `for<'tcx>` objecs, there wasn't much point to that. - Several passes now store a `String` instead of a `&'l str` (again, no point).
Also, store the completed set of passes in the tcx.
this temporary disables `inline`
68f7821
to
af2c59d
Compare
I tested this with it enabled 100% of the time, and we were able to run mir-opt tests successfully.
af2c59d
to
488b2a3
Compare
@bors r=eddyb |
📌 Commit 488b2a3 has been approved by |
…-cell-mir, r=eddyb rework the queries for the MIR pipeline This PR refashions the MIR pipeline. There are a number of changes: * We no longer have "MIR passes" and the pass manager is completely reworked. Unless we are doing the interprocedural optimization (meaning, right now, the inline pass), we will process a single MIR from beginning to finish in a completely on-demand fashion; i.e., when you request `optimized_mir(D)`, that will trigger the MIR for `D` to actually be built and optimized, but no other functions are built or touched. * We no longer use `&'tcx RefCell<Mir<'tcx>>` as the result of queries, since that spoils the view of queries as "pure functions". To avoid however copying the MIR, we use a `&'tcx Steal<Mir<'tcx>>` -- this is something like a ref-cell, in that you can use `borrow()` to read it, but it has no `borrow_mut()`. Instead, it has `steal()`, which will take the contents and then panic if any further read attempt occurs. * We now support `[multi]` queries, which can optionally yield not just one result but a sequence of (K, V) pairs. This is used for the inlining pass. If inlining is enabled, then when it is invoked on **any** def-id D, it will go and read the results for **all** def-ids and transform them, and then return the results for all of them at once. This isn't ideal, and we'll probably want to rework this further, but it seems ok for now (note that MIR inlining is not enabled by default). **Tips for the reviewer:** The commits here are meant to build individually, but the path is a *bit* meandering. In some cases, for example, I introduce a trait in one commit, and then tweak it in a later commit as I actually try to put it to use. You may want to read the README in the final commit to get a sense of where the overall design is headed. @eddyb I did not wind up adding support for queries that produce more than one *kind* of result. Instead, I decided to just insert judicious use of the `force()` command. In other words, we had talked about e.g. having a query that produced not only the MIR but also the `const_qualif` result for the MIR in one sweep. I realized you can also have the same effect by having a kind of meta-query that forces the const-qualif pass and then reads the result. See the README for a description. (We can still do these "multi-query results" later if we want, I'm not sure though if it is necessary.) r? @eddyb cc @michaelwoerister @matthewhammer @arielb1, who participated in the IRC discussion.
This PR refashions the MIR pipeline. There are a number of changes:
optimized_mir(D)
, that will trigger the MIR forD
to actually be built and optimized, but no other functions are built or touched.&'tcx RefCell<Mir<'tcx>>
as the result of queries, since that spoils the view of queries as "pure functions". To avoid however copying the MIR, we use a&'tcx Steal<Mir<'tcx>>
-- this is something like a ref-cell, in that you can useborrow()
to read it, but it has noborrow_mut()
. Instead, it hassteal()
, which will take the contents and then panic if any further read attempt occurs.[multi]
queries, which can optionally yield not just one result but a sequence of (K, V) pairs. This is used for the inlining pass. If inlining is enabled, then when it is invoked on any def-id D, it will go and read the results for all def-ids and transform them, and then return the results for all of them at once. This isn't ideal, and we'll probably want to rework this further, but it seems ok for now (note that MIR inlining is not enabled by default).Tips for the reviewer: The commits here are meant to build individually, but the path is a bit meandering. In some cases, for example, I introduce a trait in one commit, and then tweak it in a later commit as I actually try to put it to use. You may want to read the README in the final commit to get a sense of where the overall design is headed.
@eddyb I did not wind up adding support for queries that produce more than one kind of result. Instead, I decided to just insert judicious use of the
force()
command. In other words, we had talked about e.g. having a query that produced not only the MIR but also theconst_qualif
result for the MIR in one sweep. I realized you can also have the same effect by having a kind of meta-query that forces the const-qualif pass and then reads the result. See the README for a description. (We can still do these "multi-query results" later if we want, I'm not sure though if it is necessary.)r? @eddyb
cc @michaelwoerister @matthewhammer @arielb1, who participated in the IRC discussion.