[WIP] Don't drop an enum after all its fields have been moved from #68493
Conversation
This file contains 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
|
r? @zackmdavis (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Jan 23, 2020
[WIP] Don't `drop` enum if all fields are moved out of
Previously, drop elaboration is smart enough to realize when all fields of a struct or tuple are moved out of along a given code path and will remove drop terminators on that code path. However, if all fields of an *enum* with multiple variants are moved out of, drop terminators are still emitted. After this PR, drop elaboration will eliminate drops of an `Option` after the value in the `Some` variant is moved out of. `Option::unwrap` is an example of code whose MIR should be a bit better optimized as a result of this.
```rust
fn unwrap<T>(opt: Option<T>) -> T {
match opt {
Some(inner) => inner,
None => panic!("unwrap"),
}
}
```
When I say all fields of an enum, I mean all fields across all variants, so this optimization does not apply when, e.g., the value in `Result::Ok` is moved out of. To improve this case, we would need to incorporate information about the variant that is currently live into the `{Un,}InitializedPlaces` dataflow analyses. This is more involved, but I plan to give it a try.
ecstatic-morse
changed the title
drop enum if all fields are moved out of
ecstatic-morse
changed the title
|
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 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 |
|
@bors retry |
bors
added
S-waiting-on-bors
|
@Mark-Simulacrum This one's not quite right yet unfortunately. So much for my one line change. There's a bit of discussion on Zulip. Edit: just saw the discord discussion |
|
This optimization will require additional information about the liveness of enum variants. Closing for now. |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Drop elaboration is smart enough to realize when all fields of a struct or tuple are moved out of along a given code path and will remove drop terminators on that code path. However, if all fields of an enum with multiple variants are moved out of, drop terminators are still emitted. After this PR, drop elaboration will eliminate drops of an
Optionafter the value in theSomevariant is moved out of.Option::unwrapis an example of code whose MIR should be a bit better optimized as a result.When I say all fields of an enum, I mean all fields across all variants, so this optimization does not apply when, e.g., the value in
Result::Okis moved out of. To improve this case, we would need to incorporate information about the variant that is currently live into the{Un,}InitializedPlacesdataflow analyses. This is more involved, but I plan to give it a try.This is a WIP pending perf results and the addition of a test.