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

[NLL] Remove `LiveVar` #58505

Merged
merged 4 commits into from Mar 4, 2019

Conversation

Projects
None yet
8 participants
@schomatis
Copy link
Contributor

schomatis commented Feb 16, 2019

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 Locals 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

schomatis added some commits Feb 15, 2019

nll: remove `NllLivenessMap` from `LivenessContext`
It was used in `compute_for_all_locals` to iterate only the `Local`s that need
liveness analysis (filtered through `compute`). Instead, explicitly extract that
reduced set (as `live_locals`) in `trace` and pass it to
`compute_for_all_locals`.

Change the variable type used in `compute_for_all_locals` from `LiveVar` to
`Local` and do the same for its helper functions (and the functions in
`LocalUseMap` they rely on):

* `add_defs_for`                 -> `LocalUseMap::defs`
* `compute_use_live_points_for`  -> `LocalUseMap::uses`
* `compute_drop_live_points_for` -> `LocalUseMap::drops`

Push back the use of `LiveVar` to the `LocalUseMap` (where the other
`NllLivenessMap` remains embedded) functions which internally do the
`from_local` conversion.
nll: remove `NllLivenessMap` from `LocalUseMap`
Extend `LocalUseMap`'s `IndexVec`s that track def/use/drop data to store the
original `Local` indexes and not the compacted `LiveVar` ones (favoring speed
and code simplicity over space). Remove the `NllLivenessMap` embedded inside it
since it's no longer needed to perform the `LiveVar`/`Local` conversion.
nll: remove `NllLivenessMap` and `LiveVar`
Extract the `compute` logic (now renamed `compute_live_locals`) from
`NllLivenessMap` to the `liveness` module. Remove the unused structures.
nll: remove `IdentityMap` and `LiveVariableMap`
With `NllLivenessMap` and `LiveVar` removed, the `IdentityMap` (remaining
structure implementing the `LiveVariableMap` trait) loses its meaning.

Specialize the `LiveVarSet` to a `BitSet<Local>` removing the `V` and related
parameters. The `LiveVarSet<V>` was only being used as `LiveVarSet<Local>` so
this commit doesn't bring any change to the logic, it just removes an unused
parameter (that without `LiveVar` now, it couldn't have been specialized to
anything but `Local`).
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 16, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Feb 16, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 16, 2019

⌛️ Trying commit ae5f722 with merge 0baf2b5...

bors added a commit that referenced this pull request Feb 16, 2019

Auto merge of #58505 - schomatis:fix/nll/remove-live-var, r=<try>
[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

This comment has been minimized.

Copy link
Contributor

bors commented Feb 16, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 22, 2019

@schomatis IIRC, we no longer even use the LiveVar abstraction, right? That is, I think that NLL wound up doing its own, distinct liveness computation, and hence the only consumer (generators) winds up computing liveness for all variables. If that is correct, then definitely 👍 from me to this change.

Regardless, I am going to re-assign this to @matthewjasper for review -- @matthewjasper, I hope that's ok. If not, please ping me on Zulip and I'll find someone else. I'm basically trying to work through a backlog of reviews so I fear I won't be able to get to it in a timely fashion.

r? @matthewjasper

@schomatis

This comment has been minimized.

Copy link
Contributor Author

schomatis commented Feb 22, 2019

@schomatis IIRC, we no longer even use the LiveVar abstraction, right? That is, I think that NLL wound up doing its own, distinct liveness computation, and hence the only consumer (generators) winds up computing liveness for all variables. If that is correct, then definitely from me to this change.

@nikomatsakis No, my (inexperienced) understanding is that we do use the LiveVar abstraction and we do not compute liveness for all variables. However, this PR doesn't change that last part, this still filters the variables considered "live", the objective of this PR is to remove the LiveVar abstraction itself and rather refer to that set as live_locals: Vec<Local> (not LiveVar) because there was a continuous conversion between LiveVar (many times referred to as local in the instantiated variable) and Local that seemed, in my opinion, rather confusing.

(There is a case, when we request to "dump facts from NLL analysis", which I'm not sure how frequent that is, when in fact we do compute liveness for all variables. This also made the LiveVar abstraction a bit more confusing because the functions that used it didn't actually have any guarantee that it was a "live" variable after all.)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 23, 2019

Ping from triage, @matthewjasper, we eagerly await your review :)

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Feb 23, 2019

Looks good, but I want to check how much memory this ends up using.

@rust-lang/infra can I have a perf run?

@schomatis

This comment has been minimized.

Copy link
Contributor Author

schomatis commented Mar 3, 2019

@rust-lang/infra can I have a perf run?

@matthewjasper ping :)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Mar 3, 2019

@rust-timer build 0baf2b5

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Mar 3, 2019

Success: Queued 0baf2b5 with parent eac0908, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Mar 3, 2019

Finished benchmarking try commit 0baf2b5

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Mar 3, 2019

Perf looks good

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 3, 2019

📌 Commit ae5f722 has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 3, 2019

⌛️ Testing commit ae5f722 with merge 45d015c...

bors added a commit that referenced this pull request Mar 3, 2019

Auto merge of #58505 - schomatis:fix/nll/remove-live-var, r=matthewja…
…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
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: matthewjasper
Pushing 45d015c to master...

@bors bors added the merged-by-bors label Mar 4, 2019

@bors bors merged commit ae5f722 into rust-lang:master Mar 4, 2019

1 check passed

homu Test successful
Details

@schomatis schomatis deleted the schomatis:fix/nll/remove-live-var branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.