Skip to content

Handle inactive enum variants in MaybeUninitializedPlaces#73879

Merged
bors merged 2 commits intorust-lang:masterfrom
ecstatic-morse:discr-switch-uninit
Jul 5, 2020
Merged

Handle inactive enum variants in MaybeUninitializedPlaces#73879
bors merged 2 commits intorust-lang:masterfrom
ecstatic-morse:discr-switch-uninit

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

ecstatic-morse commented Jun 30, 2020

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 30, 2020

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Jun 30, 2020

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

@Aaron1011
Copy link
Contributor

Aaron1011 commented Jun 30, 2020

@ecstatic-morse: How does interact with generators?

@bors
Copy link
Collaborator

bors commented Jun 30, 2020

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

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 30, 2020

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

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 30, 2020

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

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jun 30, 2020

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 _5: isize; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2
- let mut _5: bool; // in scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:6:1: 6:2
- 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

@oli-obk oli-obk Jul 1, 2020

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
Collaborator

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

ecstatic-morse commented Jul 1, 2020

@bors rollup=never (perf)

@Manishearth
Copy link
Member

Manishearth commented Jul 1, 2020

@bors p=1

@bors
Copy link
Collaborator

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge 04c24cf3654f0427ebb9bdb5a8aaf879d169aeb7...

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2020

@bors yield retry

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2020

@bors retry yield

@bors
Copy link
Collaborator

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge 0202cee4aabe48154df4a45a442656e0598f3d9b...

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2020

@bors retry yield

@bors
Copy link
Collaborator

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge b5c3197d9d1e60bdd5ca98e8a8533a5742164739...

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2020

@bors retry yield

@bors
Copy link
Collaborator

bors commented Jul 2, 2020

⌛ Testing commit eb4d28b with merge aafc3c614dfaf82515c3c9c8f5eccd812823603c...

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2020

@bors retry yield

@bors
Copy link
Collaborator

bors commented Jul 3, 2020

⌛ Testing commit eb4d28b with merge 4a0a37a3373d93f4332f326546224e43d020d36c...

@bors
Copy link
Collaborator

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

JohnTitor commented Jul 5, 2020

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
Collaborator

bors commented Jul 5, 2020

⌛ Testing commit eb4d28b with merge 2753fab...

@bors
Copy link
Collaborator

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