only compute liveness for variables whose types include regions #52115
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Jul 6, 2018
There was a problem hiding this comment.
I would probably not parameterize this function...
There was a problem hiding this comment.
...but instead here have LivenessResults<Local> (at least to start -- later we'll change this I think)
There was a problem hiding this comment.
Similarly, for this first refactoring, we would have LocalSet<Local> here -- the idea is that the liveness computation is generic over the index, but we as the users of it still want to compute the liveness of every MIR local variable to start
There was a problem hiding this comment.
Again, this would be LivenessResults<Local> to start
There was a problem hiding this comment.
Again, no need for V here, as we are consuming the results
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
We should probably add some kind of trait like LiveVariableMap, which would look kind of like so:
trait LiveVariableMap {
type LiveVar;
fn from_local(&self, local: Local) -> Option<Self::LiveVar>;
fn from_live_var(&self, local: Self::LiveVar) -> Local;
}We would also make an "identity map" type that we use for now:
struct IdentityMap;
impl LiveVariableMap for IdentityMap {
type LiveVar = Local;
fn from_local(&self, local: Local) -> Option<Local> {
Some(local)
}
fn from_live_var(&self, local: Self::LiveVar) -> Local {
local
}
}
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
This struct is going to need to take the &impl LiveVariableMap and store it in a field (map, say)
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
This would also take a &impl LiveVariableMap<LiveVar = V>
src/librustc_mir/util/liveness.rs
Outdated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/librustc_mir/util/liveness.rs
Outdated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Jul 13, 2018
There was a problem hiding this comment.
I don't think this file should be generic over V...
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
is this giving a warning? If so, change V::LiveVar to <V as LiveVariableMap>::LiveVar and remove the bound (for better or worse, this compiles)
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
I think I intended this to be just <V>...
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
...and here we would have impl<V, M: LiveVariableMap<LiveVar = V>...
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
and here
pub fn compute<'tcx>(mir: &Mir<'tcx>, map: &M) -> LivenessResults<V> {
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
this can just be V then, I think
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #51987) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
requested changes
Jul 13, 2018
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
I would probably make this map: &impl LiveVariableMap<LiveVar = V> and remove the explicit M parameter altogether
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
this will presumably need the map to be given to it as well
src/librustc_mir/util/liveness.rs
Outdated
There was a problem hiding this comment.
this variable represents the total number of variables we are computing liveness for. We may need to extend the LiveVariableMap trait to include a num_variables(mir: &Mir<'tcx>) method -- for the identity case, it would return mir.local_decls.len().
nikomatsakis
force-pushed
the
feature/nll-liveness-regions
branch
from
9ae372e to
8ac931d
Compare
src/librustc/mir/mod.rs
Outdated
There was a problem hiding this comment.
This should remain Local! This is the full set of local declarations -- LocalWithRegion is meant to be a subset of the full set of locals.
There was a problem hiding this comment.
Ah, I suspect this is the problem -- this should not be IdentityMap, right? But rather LocalWithRegionMap or whatever that type is called
We used to hardcode that we wanted the liveness of *all* variables. This can now be configured by selecting an alternative index type V and providing a (partial) map from locals to that new type V.
Dylan-DPC-zz
force-pushed
the
feature/nll-liveness-regions
branch
from
7147292 to
2255786
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Jul 19, 2018
src/librustc/mir/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| newtype_index!(LocalWithRegion); |
There was a problem hiding this comment.
I would expect this to live in the borrow_check somewhere -- probably nll/liveness_map.rs (See below)
src/librustc_mir/util/liveness.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| crate struct NllLivenessMap { |
There was a problem hiding this comment.
I would expect this to live in the nll code -- I'd probably put it in a new module nll/liveness_map.rs along with the LocalWithRegion index declaration.
|
Lots of interesting failures: https://travis-ci.org/rust-lang/rust/jobs/405945256#L1943 =) |
nikomatsakis
requested changes
Jul 20, 2018
src/librustc_mir/util/liveness.rs
Outdated
| use util::pretty::{dump_enabled, write_basic_block, write_mir_intro}; | ||
|
|
||
| pub type LocalSet = IdxSetBuf<Local>; | ||
| pub type LocalSet<V> = IdxSetBuf<V>; |
There was a problem hiding this comment.
oh, let's rename this to LiveVarSet<V> -- it'd be nice to distinguish locals (all variables) from live variables (the set of variables we are computing liveness on). Maybe not the best name, but it works (and fits with e.g. LiveVariableMap etc)
|
@bors try |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@rust-timer build e5906f3e09c74cf6bd6fe86851a3bacd75fc1a42 |
|
Success: Queued e5906f3e09c74cf6bd6fe86851a3bacd75fc1a42 with parent c7cba3d, comparison URL. |
|
@Dylan-DPC those travis errors are basically a case of "working as expected". TBH, I suspect we could just remove those tests. They were early "bootstrap" tests of the liveness code but I think that the full set of ui/compile-fail tests are more interesting. The tests are failing though because the variables used in them do not have any regions in their types, and hence they no longer appear in the liveness results. Still, I'd suggest just removing |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Looks like a win, perf wise =) -80% for tuple-stress :) |
nikomatsakis
requested changes
Jul 21, 2018
| // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
There was a problem hiding this comment.
We should add a module doc comment here explaining what is going on. Something like this:
For the NLL computation, we need to compute liveness, but only for those local variables whose types contain regions. The others are not of interest to us. This file defines a new index type (LocalWithRegion) that indexes into a list of "variables whose type contain regions". It also defines a map from Local to LocalWithRegion and vice versa -- this map can be given to the liveness code so that it only operates over variables with regions in their types, instead of all variables.
| use rustc_data_structures::indexed_vec::Idx; | ||
| use rustc::ty::TypeFoldable; | ||
|
|
||
| crate struct NllLivenessMap { |
There was a problem hiding this comment.
Add comment:
--
Map between Local and LocalWithRegion indices: this map is supplied to the liveness code so that it will only analyze those variables whose types contain regions.
| use rustc::ty::TypeFoldable; | ||
|
|
||
| crate struct NllLivenessMap { | ||
| pub from_local: IndexVec<Local, Option<LocalWithRegion>>, |
There was a problem hiding this comment.
Comment:
For each local variable, contains either None (if the type has no regions) or Some(i) with a suitable index.
|
|
||
| crate struct NllLivenessMap { | ||
| pub from_local: IndexVec<Local, Option<LocalWithRegion>>, | ||
| pub to_local: IndexVec<LocalWithRegion, Local>, |
There was a problem hiding this comment.
Comment:
For each LocalWithRegion, maps back to the original Local index.
| } | ||
|
|
||
| impl NllLivenessMap { | ||
| pub fn compute(mir: &Mir) -> Self { |
There was a problem hiding this comment.
Comment:
Iterates over the variables in Mir and assigns each Local whose type contains regions a LocalWithRegion index. Returns a map for converting back and forth.
| } | ||
| } | ||
|
|
||
| newtype_index!(LocalWithRegion); No newline at end of file |
There was a problem hiding this comment.
Newline at end of file, probably, and a comment:
Index given to each local variable whose type contains a region.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r+ p=1 High-priority NLL fix |
|
📌 Commit 0770ff0 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=20 |
bors
added a commit
that referenced
this pull request
Jul 21, 2018
…matsakis only compute liveness for variables whose types include regions Closes #52034 r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
Merged
bors
added a commit
that referenced
this pull request
Feb 16, 2019
[NLL] Remove `LiveVar` The `LiveVar` type (and related) made it harder to reason about the code. It seemed as an abstraction that didn't bring any useful concept to the reader (when transitioning from the RFC theory to the actual implementation code). It achieved a compactness in the vectors storing the def/use/drop information that was related only to the `LocalUseMap`. This PR went in the other direction and favored time over memory (but this decision can be easily reverted to the other side without reintroducing `LiveVar`). What this PR aims at is to clarify that there's no significant transformation between the MIR `Local` and the `LiveVar` (now refactored as `live_locals: Vec<Local>`): we're just filtering (not mapping) the entire group of `Local`s into a meaningful subset that we should perform the liveness analysis on. As a side note, there is no guarantee that the liveness analysis is performed only on (what the code calls) "live" variables, if the NLL facts are requested it will be performed on *any* variable so there can't be any assumptions on that regard. (Still, this PR didn't change the general naming convention to reduce the number of changes here and streamline the review process). **Acceptance criteria:** This PR attempts to do only a minor refactoring and not to change the logic so it can't have any performance impact, particularly, it can't lose any of the significant performance improvement achieved in the great work done in #52115. r? @nikomatsakis
bors
added a commit
that referenced
this pull request
Mar 3, 2019
…sper [NLL] Remove `LiveVar` The `LiveVar` type (and related) made it harder to reason about the code. It seemed as an abstraction that didn't bring any useful concept to the reader (when transitioning from the RFC theory to the actual implementation code). It achieved a compactness in the vectors storing the def/use/drop information that was related only to the `LocalUseMap`. This PR went in the other direction and favored time over memory (but this decision can be easily reverted to the other side without reintroducing `LiveVar`). What this PR aims at is to clarify that there's no significant transformation between the MIR `Local` and the `LiveVar` (now refactored as `live_locals: Vec<Local>`): we're just filtering (not mapping) the entire group of `Local`s into a meaningful subset that we should perform the liveness analysis on. As a side note, there is no guarantee that the liveness analysis is performed only on (what the code calls) "live" variables, if the NLL facts are requested it will be performed on *any* variable so there can't be any assumptions on that regard. (Still, this PR didn't change the general naming convention to reduce the number of changes here and streamline the review process). **Acceptance criteria:** This PR attempts to do only a minor refactoring and not to change the logic so it can't have any performance impact, particularly, it can't lose any of the significant performance improvement achieved in the great work done in #52115. r? @nikomatsakis
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #52034
r? @nikomatsakis