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.

Sign up for GitHub

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

Jump to bottom

Handle inactive enum variants in MaybeUninitializedPlaces #73879

Merged
merged 2 commits into from
Jul 5, 2020
Merged

Handle inactive enum variants in MaybeUninitializedPlaces #73879

merged 2 commits into from
Jul 5, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 30, 2020 •
edited
Loading

Resolves the first part of #69715.

This is the equivalent of #68528 but for MaybeUninitializedPlaces. Because we now notify drop elaboration that inactive enum variants might be uninitialized, some drops get marked as "open" that were previously "static". Unlike in #69715, this isn't strictly better: An "open" drop expands to more MIR than a simple call to the drop shim. However, because drop elaboration considers each field of an "open" drop separately, it can sometimes eliminate unnecessary drops of moved-from or unit-like enum variants. This is the case for Option::unwrap, which is reflected in the mir-opt test.

cc @eddyb
r? @oli-obk

@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 30, 2020

⌛ Trying commit 7f8b034f63e2914164a1a2ef52b077272363f3fb with merge 7eb15f79d388f9a78cd1fb2e8eeb7267cf56f4a4...

@Aaron1011
Copy link
Member

@ecstatic-morse: How does interact with generators?

@bors
Copy link
Contributor

bors commented Jun 30, 2020

☀️ Try build successful - checks-azure
Build commit: 7eb15f79d388f9a78cd1fb2e8eeb7267cf56f4a4 (7eb15f79d388f9a78cd1fb2e8eeb7267cf56f4a4)

@rust-timer
Copy link
Collaborator

Queued 7eb15f79d388f9a78cd1fb2e8eeb7267cf56f4a4 with parent 0ca7f74, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7eb15f79d388f9a78cd1fb2e8eeb7267cf56f4a4): comparison url.

@jonas-schievink
Copy link
Contributor

Neat!

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jun 30, 2020 •
edited
Loading

How does interact with generators?

Hey, you're not a ghost! 👻

The generator transform doesn't use MaybeUninitializedLocals to compute generator saved locals, and it does drop elaboration with DropShimElaborator , which also doesn't use MaybeUninitializedLocals. Only Elaborator, which seems to be the "general purpose" drop elaborator uses it. Furthermore, apply_discriminant_switch_effect doesn't get called for TyKind::Generators, so I don't believe that this affects generators any more than regular functions. I don't know what the benefits may be from changing that last part, but it might be worth investigating. I'm pretty much terrified of touching the generator transform, though.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jun 30, 2020 •
edited
Loading

This is ready for review now.

r? @oli-obk
cc @eddyb

@ecstatic-morse ecstatic-morse marked this pull request as ready for review June 30, 2020 20:42
@rust-lang rust-lang deleted a comment from rust-highfive Jul 1, 2020
oli-obk
oli-obk reviewed Jul 1, 2020
View reviewed changes
...ir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff
- let mut _6: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2
- let mut _7: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god diffs of diffs XD. I love how this basically says we now have an additional variable, but it's optimized out here.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 1, 2020

📌 Commit eb4d28b has been approved by oli-obk

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 1, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jul 1, 2020

You're really raising the bar on what a high quality PR means... Readable PR diff, modular design, documentation and it all causes a perf improvement along the way.

@ecstatic-morse
Copy link
Contributor Author

@bors rollup=never (perf)

@Manishearth
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge 04c24cf3654f0427ebb9bdb5a8aaf879d169aeb7...

@Manishearth
Copy link
Member

@bors yield retry

@Manishearth
Copy link
Member

@bors retry yield

@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge 0202cee4aabe48154df4a45a442656e0598f3d9b...

@Manishearth
Copy link
Member

@bors retry yield

@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge b5c3197d9d1e60bdd5ca98e8a8533a5742164739...

@Manishearth
Copy link
Member

@bors retry yield

@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge aafc3c614dfaf82515c3c9c8f5eccd812823603c...

@Manishearth
Copy link
Member

@bors retry yield

@bors
Copy link
Contributor

bors commented Jul 3, 2020

⌛ Testing commit eb4d28b with merge 4a0a37a3373d93f4332f326546224e43d020d36c...

@bors
Copy link
Contributor

bors commented Jul 3, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 3, 2020
@JohnTitor
Copy link
Member

Another case of #73992, @bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2020
@bors
Copy link
Contributor

bors commented Jul 5, 2020

⌛ Testing commit eb4d28b with merge 2753fab...

@bors
Copy link
Contributor

bors commented Jul 5, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 2753fab to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 5, 2020
@bors bors merged commit 2753fab into rust-lang:master Jul 5, 2020
@ecstatic-morse ecstatic-morse mentioned this pull request Sep 30, 2020
Closed
@cuviper cuviper mentioned this pull request Oct 1, 2020
Closed
@ecstatic-morse ecstatic-morse deleted the discr-switch-uninit branch October 6, 2020 01:42
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
9 participants
@ecstatic-morse @rust-timer @bors @Aaron1011 @jonas-schievink @oli-obk @Manishearth @JohnTitor @cuviper