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

Make needs_drop less pessimistic on generators #70015

Merged
merged 3 commits into from
Apr 19, 2020
Merged

Make needs_drop less pessimistic on generators #70015

merged 3 commits into from
Apr 19, 2020

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Mar 15, 2020

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

This builds off of #69814 since that contains some fixes that are made relevant by this PR (see #69814 (comment)). (this has been merged)

@rust-highfive
Copy link
Collaborator

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2020

let witness = substs.witness(def_id, tcx);
let interior_tys = match &witness.kind {
ty::GeneratorWitness(tys) => tcx.erase_late_bound_regions(tys),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure the erase_late_bound_regions is completely correct here

@Centril
Copy link
Contributor

Centril commented Mar 15, 2020

r? @matthewjasper

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2020

📌 Commit 489d79d has been approved by matthewjasper

@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 Mar 23, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
@Centril
Copy link
Contributor

Centril commented Mar 23, 2020

Failed in #70325 (comment), @bors r-
(Needs rebase)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2020
@jonas-schievink
Copy link
Contributor Author

@bors r=matthewjasper

@bors
Copy link
Contributor

bors commented Mar 23, 2020

📌 Commit 1df7641 has been approved by matthewjasper

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 23, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…atthewjasper

Make `needs_drop` less pessimistic on generators

Generators only have non-trivial drop logic when they may store (in upvars or across yields) a type that does.

This prevents generation of some unnecessary MIR in simple generators. There might be some impact on compile times, but this is probably limited in real-world applications.

~~This builds off of rust-lang#69814 since that contains some fixes that are made relevant by *this* PR (see rust-lang#69814 (comment) (this has been merged)
@jonas-schievink
Copy link
Contributor Author

That's weird. They pass locally even after a rebase and I'm pretty sure CI also runs mir-opt tests.

@matthewjasper
Copy link
Contributor

Some CI builders have -C panic=abort

@jonas-schievink
Copy link
Contributor Author

Ah, you're right. Can reproduce.

@jonas-schievink
Copy link
Contributor Author

I wonder why this wasn't hit before though – the failing test checks for cleanup blocks, which should be removed by the time the generator transform runs when -Cpanic=abort.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@crlf0710
Copy link
Member

@jonas-schievink Ping from triage, would you mind rebasing and resolve the merge conflicts? Thanks.

@jonas-schievink
Copy link
Contributor Author

Rebased, and ignored the test on wasm32, like some other mir-opt tests.

@bors r=matthewjasper rollup=never

@bors
Copy link
Contributor

bors commented Apr 17, 2020

📌 Commit ae53315 has been approved by matthewjasper

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2020
@@ -99,6 +99,23 @@ where
}
}

ty::Generator(_, substs, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have assertions somewhere that could catch bugs in needs_drop (i.e., catch cases where a type has drop glue but needs_drop says no)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of, but we have several mir-opt tests for generators, as well as diagnostics tests, which were (correctly) affected by this change.

@bors
Copy link
Contributor

bors commented Apr 18, 2020

⌛ Testing commit ae53315 with merge ae96d19171e368760d20cfd76399a4e85b07f5d6...

@bors
Copy link
Contributor

bors commented Apr 18, 2020

💔 Test failed - checks-azure

@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 Apr 18, 2020
@jonas-schievink
Copy link
Contributor Author

@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 Apr 18, 2020
@bors
Copy link
Contributor

bors commented Apr 19, 2020

⌛ Testing commit ae53315 with merge 36b1a92...

@bors
Copy link
Contributor

bors commented Apr 19, 2020

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 36b1a92 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 19, 2020
@bors bors merged commit 36b1a92 into rust-lang:master Apr 19, 2020
@jonas-schievink jonas-schievink deleted the gen-needs-drop branch April 19, 2020 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants