MIR pass to remove unneeded drops on types not needing drop #76673

simonvandel · 2020-09-13T14:07:35Z

This is heavily dependent on MIR inlining running to actually see the drop statement.

Do we want to special case replacing a call to std::mem::drop with a goto aswell?

rust-highfive · 2020-09-13T14:07:38Z

r? @petrochenkov

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

petrochenkov · 2020-09-13T18:36:12Z

r? @oli-obk (the first reviewer suggested by github)

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs

oli-obk · 2020-09-17T11:31:34Z

Do we want to special case replacing a call to std::mem::drop with a goto aswell?

No, we'll want inlining to take care of this at some point

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs

oli-obk · 2020-09-17T11:36:31Z

one thing I'm wondering is whether we should/could just rerun drop elaboration after inlining... That would catch even more cases, but may be expensive.

simonvandel · 2020-09-19T13:25:31Z

Hi @oli-obk
I pushed some cleanup and your suggestion for running simplify-cfg after the optimization.

Note that on mir-opt-level=1 this pass really does nothing as the inliner is not running.

one thing I'm wondering is whether we should/could just rerun drop elaboration after inlining... That would catch even more cases, but may be expensive.

I suggest handling that in another issue. Do you want to elaborate on it yourself in another issue?

oli-obk · 2020-09-19T13:33:07Z

I suggest handling that in another issue

yes let's handle that in the future. It was just random musing, I don't know at all whether any of that is actionable

@bors r+

bors · 2020-09-19T13:33:09Z

📌 Commit 804f673 has been approved by oli-obk

…r=oli-obk MIR pass to remove unneeded drops on types not needing drop This is heavily dependent on MIR inlining running to actually see the drop statement. Do we want to special case replacing a call to std::mem::drop with a goto aswell?

simonvandel · 2020-09-22T21:17:09Z

Rebased

ecstatic-morse · 2020-09-22T22:15:58Z

@bors r=oli-obk

bors · 2020-09-22T22:15:59Z

📌 Commit ff24163 has been approved by oli-obk

ecstatic-morse · 2020-09-22T23:56:39Z

@bors p=15 (queue fairness)

bors · 2020-09-23T01:11:46Z

⌛ Testing commit ff24163 with merge 1fbc1c2c0ab3279483232bf44b400d18a3275b0e...

bors · 2020-09-23T01:39:18Z

💔 Test failed - checks-actions

simonvandel · 2020-09-23T05:22:03Z

Hmm seems like the same error as in the rollup. When I did the rebase I also reblessed but that didn't change any files. Not sure what is wrong at this point.

RalfJung · 2020-09-23T08:06:23Z

Maybe the target (wasm32-unknown-unknown) is relevant to reproduce the problem?

oli-obk · 2020-09-23T10:07:52Z

Yea I'd also guess that it's related to the target, as that specific drop impl doesn't seem to have unwinding activated. Check the other mir-opt tests to see if any of them have // ignore-foo flags that relate to unwind, abort or wasm32

simonvandel · 2020-09-23T16:26:28Z

Let's try again with the test ignored on wasm

RalfJung · 2020-09-23T16:33:00Z

@bors r=oli-obk

bors · 2020-09-23T16:33:02Z

📌 Commit 05f84c6 has been approved by oli-obk

bors · 2020-09-23T20:13:57Z

⌛ Testing commit 05f84c6 with merge 8b40853...

bors · 2020-09-23T22:34:37Z

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

tmiasko · 2020-09-24T15:48:40Z

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs

+    fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
+        match terminator.kind {
+            TerminatorKind::Drop { place, target, .. }
+            | TerminatorKind::DropAndReplace { place, target, .. } => {


The DropAndReplace terminator should never occur in an input to this pass, since it should be removed during drop elaboration. Additionally, its semantics is to drop and assign a new value, so the transformation wouldn't be correct without additional assignment. Either way, I would just remove it from here.

Hi @tmiasko, thanks! See PR #77165

…as-schievink Rollup of 15 pull requests Successful merges: - rust-lang#75438 (Use adaptive SVG favicon for rustdoc like other rust sites) - rust-lang#76304 (Make delegation methods of `std::net::IpAddr` unstably const) - rust-lang#76724 (Allow a unique name to be assigned to dataflow graphviz output) - rust-lang#76978 (Documented From impls in std/sync/mpsc/mod.rs) - rust-lang#77044 (Liballoc bench vec use mem take not replace) - rust-lang#77050 (Typo fix: "satsify" -> "satisfy") - rust-lang#77074 (add array::from_ref) - rust-lang#77078 (Don't use an if guard to check equality with a constant) - rust-lang#77079 (Use `Self` in docs when possible) - rust-lang#77081 (Merge two almost identical match arms) - rust-lang#77121 (Updated html_root_url for compiler crates) - rust-lang#77136 (Suggest `const_mut_refs`, not `const_fn` for mutable references in `const fn`) - rust-lang#77160 (Suggest `const_fn_transmute`, not `const_fn`) - rust-lang#77164 (Remove workaround for deref issue that no longer exists.) - rust-lang#77165 (Followup to rust-lang#76673) Failed merges: r? `@ghost`

rust-highfive assigned petrochenkov Sep 13, 2020

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

simonvandel force-pushed the remove-unneeded-drops branch from 1657e01 to 9c5d0c1 Compare September 13, 2020 14:28

rust-highfive assigned oli-obk and unassigned petrochenkov Sep 13, 2020

jyn514 added A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 13, 2020

andjo403 reviewed Sep 14, 2020

View reviewed changes

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs Outdated Show resolved Hide resolved

oli-obk reviewed Sep 17, 2020

View reviewed changes

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs Show resolved Hide resolved

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 Sep 19, 2020

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 17 pull requests #76944

Closed

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 18 pull requests #76945

Closed

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 18 pull requests #76946

Closed

Dylan-DPC-zz mentioned this pull request Sep 21, 2020

Rollup of 14 pull requests #76987

Closed

Dylan-DPC-zz mentioned this pull request Sep 21, 2020

Rollup of 17 pull requests #76988

Closed

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 Sep 22, 2020

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 Sep 23, 2020

Ignore test on wasm as it does not unwind

05f84c6

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 Sep 23, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2020

bors merged commit 8b40853 into rust-lang:master Sep 23, 2020

rustbot added this to the 1.48.0 milestone Sep 23, 2020

tmiasko reviewed Sep 24, 2020

View reviewed changes

simonvandel added a commit to simonvandel/rust that referenced this pull request Sep 24, 2020

Resolve rust-lang#76673 (comment)

7dec440

simonvandel mentioned this pull request Sep 24, 2020

Followup to #76673 #77165

Merged

jonas-schievink mentioned this pull request Sep 25, 2020

Rollup of 15 pull requests #77172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR pass to remove unneeded drops on types not needing drop #76673

MIR pass to remove unneeded drops on types not needing drop #76673

simonvandel commented Sep 13, 2020

rust-highfive commented Sep 13, 2020

petrochenkov commented Sep 13, 2020

oli-obk commented Sep 17, 2020

oli-obk commented Sep 17, 2020

simonvandel commented Sep 19, 2020

oli-obk commented Sep 19, 2020

bors commented Sep 19, 2020

simonvandel commented Sep 22, 2020

ecstatic-morse commented Sep 22, 2020

bors commented Sep 22, 2020

ecstatic-morse commented Sep 22, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

simonvandel commented Sep 23, 2020

RalfJung commented Sep 23, 2020

oli-obk commented Sep 23, 2020

simonvandel commented Sep 23, 2020

RalfJung commented Sep 23, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

tmiasko Sep 24, 2020

simonvandel Sep 24, 2020

MIR pass to remove unneeded drops on types not needing drop #76673

MIR pass to remove unneeded drops on types not needing drop #76673

Conversation

simonvandel commented Sep 13, 2020

rust-highfive commented Sep 13, 2020

petrochenkov commented Sep 13, 2020

oli-obk commented Sep 17, 2020

oli-obk commented Sep 17, 2020

simonvandel commented Sep 19, 2020

oli-obk commented Sep 19, 2020

bors commented Sep 19, 2020

simonvandel commented Sep 22, 2020

ecstatic-morse commented Sep 22, 2020

bors commented Sep 22, 2020

ecstatic-morse commented Sep 22, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

simonvandel commented Sep 23, 2020

RalfJung commented Sep 23, 2020

oli-obk commented Sep 23, 2020

simonvandel commented Sep 23, 2020

RalfJung commented Sep 23, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

tmiasko Sep 24, 2020

Choose a reason for hiding this comment

simonvandel Sep 24, 2020

Choose a reason for hiding this comment