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

MIR: split Operand::Consume into Copy and Move. #46142

Merged
merged 5 commits into from Nov 28, 2017

Conversation

Projects
None yet
7 participants
@eddyb
Member

eddyb commented Nov 21, 2017

By encoding the choice of leaving the source untouched (Copy) and invalidating it (Move) in MIR, we can express moves of copyable values and have MIR borrow-checking enforce them, including ownership transfer of stack locals in calls (when the ABI passes by indirection).

Optimizations could turn a "last-use" Copy into a Move, and the MIR borrow-checker, at least within the confines of safe code, could even do this when the underlying lvalue was borrowed.
(However, that last part would be the first time lifetime inference affects code generation, AFAIK).

Furthermore, as Moves invalidate borrows as well, for any local that is initialized only once, we can ignore borrows that happened before a Move and safely reuse/replace its memory storage.
This will allow us to perform NRVO in the presence of short-lived borrows, unlike LLVM (currently), and even compute optimal StorageLive...StorageDead ranges instead of discarding them.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 21, 2017

Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Nov 21, 2017

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb
Member

eddyb commented Nov 21, 2017

@nikomatsakis

r=me modulo nits

Show outdated Hide outdated src/librustc/mir/mod.rs Outdated
}
_ => {}

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 21, 2017

Contributor

Personally, I'd rather this exhaustive (and perhaps move the if on the arm above inside the arm. But I rate the probability of overlooking this match after some future change to Operand as fairly low, so it's not that important.

@nikomatsakis

nikomatsakis Nov 21, 2017

Contributor

Personally, I'd rather this exhaustive (and perhaps move the if on the arm above inside the arm. But I rate the probability of overlooking this match after some future change to Operand as fairly low, so it's not that important.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Nov 21, 2017

Member

FYI: If PR #46093 goes in first, I think it will conflict with this in a way that git won't notice. This line will need to become a Move: https://github.com/rust-lang/rust/pull/46093/files#diff-69bf2a484158ebf56322b7ffb03601cdR108

Member

scottmcm commented Nov 21, 2017

FYI: If PR #46093 goes in first, I think it will conflict with this in a way that git won't notice. This line will need to become a Move: https://github.com/rust-lang/rust/pull/46093/files#diff-69bf2a484158ebf56322b7ffb03601cdR108

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 22, 2017

Contributor

☔️ The latest upstream changes (presumably #45879) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 22, 2017

☔️ The latest upstream changes (presumably #45879) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Nov 27, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 27, 2017

Contributor

📌 Commit cd99774 has been approved by nikomatsakis

Contributor

bors commented Nov 27, 2017

📌 Commit cd99774 has been approved by nikomatsakis

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Nov 27, 2017

Member

Looks like this might be real?

[00:19:46] error[E0599]: no associated item named `Consume` found for type `rustc::mir::Operand<'_>` in the current scope
[00:19:46]    --> /checkout/src/librustc_mir/transform/add_moves_for_packed_drops.rs:135:34
[00:19:46]     |
[00:19:46] 135 |                      Rvalue::Use(Operand::Consume(location.clone())));
[00:19:46]     |                                  ^^^^^^^^^^^^^^^^
Member

scottmcm commented Nov 27, 2017

Looks like this might be real?

[00:19:46] error[E0599]: no associated item named `Consume` found for type `rustc::mir::Operand<'_>` in the current scope
[00:19:46]    --> /checkout/src/librustc_mir/transform/add_moves_for_packed_drops.rs:135:34
[00:19:46]     |
[00:19:46] 135 |                      Rvalue::Use(Operand::Consume(location.clone())));
[00:19:46]     |                                  ^^^^^^^^^^^^^^^^
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 28, 2017

Member

@bors r=nikomatsakis

Member

eddyb commented Nov 28, 2017

@bors r=nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

📌 Commit 56fb59c has been approved by nikomatsakis

Contributor

bors commented Nov 28, 2017

📌 Commit 56fb59c has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

☔️ The latest upstream changes (presumably #46312) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 28, 2017

☔️ The latest upstream changes (presumably #46312) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 28, 2017

Member

@bors r=nikomatsakis

Member

eddyb commented Nov 28, 2017

@bors r=nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

📌 Commit 919ed40 has been approved by nikomatsakis

Contributor

bors commented Nov 28, 2017

📌 Commit 919ed40 has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

⌛️ Testing commit 919ed40 with merge 7745a7a...

Contributor

bors commented Nov 28, 2017

⌛️ Testing commit 919ed40 with merge 7745a7a...

bors added a commit that referenced this pull request Nov 28, 2017

Auto merge of #46142 - eddyb:even-mirer-2, r=nikomatsakis
MIR: split Operand::Consume into Copy and Move.

By encoding the choice of leaving the source untouched (`Copy`) and invalidating it (`Move`) in MIR, we can express moves of copyable values and have MIR borrow-checking enforce them, *including* ownership transfer of stack locals in calls  (when the ABI passes by indirection).

Optimizations could turn a "last-use" `Copy` into a `Move`, and the MIR borrow-checker, at least within the confines of safe code, could even do this when the underlying lvalue was borrowed.
(However, that last part would be the first time lifetime inference affects code generation, AFAIK).

Furthermore, as `Move`s invalidate borrows as well, for any local that is initialized only once, we can ignore borrows that happened before a `Move` and safely reuse/replace its memory storage.
This will allow us to perform NRVO in the presence of short-lived borrows, unlike LLVM (currently), and even compute optimal `StorageLive...StorageDead` ranges instead of discarding them.

bors added a commit that referenced this pull request Nov 28, 2017

Auto merge of #46321 - eddyb:even-mirer-3, r=<try>
 rustc_mir: implement an "lvalue reuse" optimization (aka destination propagation aka NRVO).

Replaces a chain of moves, such as `a = ...; ... b = move a; ... f(&mut b) ... c = move b`, with the final destination, i.e. only `c = ...; ... f(&mut c); ...` remains (note that borrowing is allowed).

**DO NOT MERGE** only for testing atm. Based on #46142.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 28, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7745a7a to master...

Contributor

bors commented Nov 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7745a7a to master...

@bors bors merged commit 919ed40 into rust-lang:master Nov 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@eddyb eddyb deleted the eddyb:even-mirer-2 branch Nov 28, 2017

dwrensha added a commit to dwrensha/seer that referenced this pull request Nov 29, 2017

@eddyb eddyb referenced this pull request Jan 30, 2018

Open

MIR optimization tracking issue. #44285

3 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment