MIR cleanups and predecessor cache #34149
Conversation
arielb1
force-pushed
the
remove-remove-dead-blocks
branch
3 times, most recently
from
b74fdd6
to
72f2715
Compare
| //! | ||
| //! Here the block (`{ return; }`) has the return type `char`, | ||
| //! rather than `()`, but the MIR we naively generate still contains | ||
| //! the `_a = ()` write in the unreachable block "after" the return. |
|
I'm not keen on making the |
|
(I don't have a strong opinion on whether to keep removal of dead blocks separate or not.) |
arielb1
force-pushed
the
remove-remove-dead-blocks
branch
from
4de9cf9
to
62133d7
Compare
| // fn should_run(Session) to check if pass should run? | ||
| fn dep_node(&self, def_id: DefId) -> DepNode<DefId> { | ||
| DepNode::MirPass(def_id) | ||
| } | ||
| fn name(&self) -> &str; |
There was a problem hiding this comment.
A default implementation of this could be ::std::intrinsics::type_name::<Self>(). With that as a default, I doubt many passes would like to manually implement this method.
|
Regarding the privacy of basic_blocks: First, there is an accessor that gives I think if we wanted to guarantee things, we would need to have the I'm not sure how much it is worth using the type system to prevent these sorts of bugs. It might be. Dynamic asserts or other things might also be effective. What I had originally envisioned was maybe something where the cc @scottcarr, who was also talking to me about this independently over IRC |
|
See a later commit. The accessor invalidates the cache to prevent you from using it incorrectly. I might also want to add a more complicated set of methods that does not invalidate the cache, but ATM all passes work in an analyze-commit pattern, except for |
|
Yes, I stand corrected. I am trying to decide my revised opinion. =) I like that we would cache between passes. I dislike losing the exhaustive visitor code, but then I think that the danger of visitor falling out of date is probably lowest at this very high-level. I imagine there will be other bits of analysis data that might be at a similar risk of going out of date, and which we might want to use a different pattern, but probably in those cases we can be careful instead. Conclusion: I think I'm happy with this version for time being. |
|
added commits for the suggestions |
|
r? @nikomatsakis for the changes |
|
btw, something that @scottcarr was asking me last night -- do we expect the predecessor cache to really persist between passes? That is, do we have passes that don't make changes? I figure the answer is that yes, we do expect it to persist, at least long term, for two reasons:
On a related note, what other kinds of information may make sense to put in this cache. My rule of thumb was that it should contain analysis results that are properties of the graph structure and which more than one pass is interested in. Examples beyond predecessors might include dominators, post-dominators, loop interval trees, that sort of thing. I guess there is no fundamental reason the results must be tied to the graph structure. I suggested that however because it ensures that the number of "setter" methods we might want are relatively limited:
(In particular there is no reason to track whatever random changes are made to statements, since it cannot affect cached data that is tied to the graph structure.) |
| if seen.insert(succ.index()) { | ||
| worklist.push(*succ); | ||
| // we can't use mir.predecessors() here because that counts | ||
| // dead blocks, which we don't want to. |
There was a problem hiding this comment.
to be clear, we will be adjusting this count to keep it up to date, right?
I guess this code is simple enough it's fine to cut-and-paste. In theory, it might be nice if we could "re-use" some underlying predecessor abstraction independent from the cache.
|
r=me for new commits |
|
(still needs rebase) |
|
Sure there are - |
Use it instead of a `panic` for inexhaustive matches and correct the
comment. I think we trust our match-generation algorithm enough to
generate these blocks, and not generating an `unreachable` means that
LLVM won't optimize `match void() {}` to an `unreachable`.
arielb1
force-pushed
the
remove-remove-dead-blocks
branch
from
f0d40eb
to
f5b1ba6
Compare
|
@bors r=nikomatsakis |
|
📌 Commit f5b1ba6 has been approved by |
|
@bors r- |
|
@bors r=nikomatsakis |
arielb1
force-pushed
the
remove-remove-dead-blocks
branch
from
944b183
to
4cfb145
Compare
|
@bors r=nikomatsakis |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 4cfb145 has been approved by |
MIR cleanups and predecessor cache This PR cleans up a few things in MIR and adds a predecessor cache to allow graph algorithms to be run easily. r? @nikomatsakis
|
💔 Test failed - auto-linux-64-opt-rustbuild |
arielb1
force-pushed
the
remove-remove-dead-blocks
branch
from
4cfb145
to
ce4fdef
Compare
|
@bors r=nikomatsakis silly |
|
📌 Commit ce4fdef has been approved by |
MIR cleanups and predecessor cache This PR cleans up a few things in MIR and adds a predecessor cache to allow graph algorithms to be run easily. r? @nikomatsakis
[MIR] Add Dominators to MIR and Add Graph Algorithms ~~This PR assumes PR #34149 lands.~~ Add generic graph algorithms to rustc_data_structures. Add dominators and successors to the ~~cache (that currently only holds predecessors).~~ `Mir`.
This PR cleans up a few things in MIR and adds a predecessor cache to allow graph algorithms to be run easily.
r? @nikomatsakis