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

[WIP] Lower core::ops::drop directly to MIR drop #62864

Closed
wants to merge 1 commit into from

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Jul 22, 2019

This change causes drop() to always drop in-place. This is meant to
allow internal optimizations (see #62508).

This does not change the documented contract for drop(). Only internal
compiler code is allowed to rely on this for now.

This is WIP still because it breaks the optimization implemented in #53080.

Closes #62508.

r? @cramertj
cc @arielb1 @eddyb

This change causes drop() to always drop in-place. This is meant to
allow internal optimizations (see rust-lang#62508).

This does _not_ change the documented contract for drop(). Only internal
compiler code is allowed to rely on this for now.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2019
@eddyb
Copy link
Member

eddyb commented Jul 22, 2019

cc @matthewjasper @pnkfelix

@eddyb
Copy link
Member

eddyb commented Jul 22, 2019

@tmandry I would be far more comfortable if this was an intrinsic, in case there are some weird interactions with callers of mem::drop itself.

@Centril
Copy link
Contributor

Centril commented Jul 22, 2019

I would like clarification on "guarantee" here. It would be one thing for this to be a pure non-guaranteed optimization that rustc has and quite another for this to be a guarantee that users can rely on for all time.

If we're going to do this I would also like to see some tests checking that the static semantics of fn drop<T>(x: T) {} as defined previously are preserved in terms of the binding "moved" into drop not being available and other properties of calling a function.

@cramertj
Copy link
Member

This PR as-implemented is what I think we should do, and the implementation looks like what I would expect. Nominating for the next lang team meeting so we can discuss exactly what guarantees we do or don't want to offer outside of the compiler/stdlib. IMO we do need to be getting into the business of guaranteeing some move elisions both for the purpose of pinning and for systems that can't afford multiple copies of large stack variables, but this PR likely isn't the time/place to begin doing that.

@cramertj cramertj added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Jul 22, 2019
@tmandry
Copy link
Member Author

tmandry commented Jul 22, 2019

I would like clarification on "guarantee" here. It would be one thing for this to be a pure non-guaranteed optimization that rustc has and quite another for this to be a guarantee that users can rely on for all time.

Right now it is internal-only. This is in the PR description. But I guess the lang team can decide whether to expand this.

@@ -53,7 +53,7 @@ impl Drop for S {
// StorageLive(_3);
// StorageLive(_4);
// _4 = move _1;
// _3 = const std::mem::drop::<std::boxed::Box<S>>(move _4) -> [return: bb5, unwind: bb7];
// drop(_4) -> [return: bb5, unwind: bb7];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change actually result in generators being smaller? I was under the impression that the move on the line above was the problem.

Copy link
Member

Choose a reason for hiding this comment

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

No, this change does not change the size of generators, but it allows removing the let binding inside the .await expansion, which would reduce the size of generators.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to solve that problem either, there's still the move on the line above that needs to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch, that move will need to be eliminated as well.

@tmandry
Copy link
Member Author

tmandry commented Jul 22, 2019

Okay, after some further discussion with @cramertj, I realized there are a couple problems with this approach to optimizing await.

For one, the goal (see #62508 (comment)) was to be able to desugar let y = x.await; to roughly:

let y = {
  loop {
    match poll(Pin::new_unchecked(&mut x)) { .. }
  }
  drop(x);
};

And thus eliminate the let mut pinned = x; statement which is there at the beginning today. This would eliminate a move out of x, which makes it much easier to optimize common cases.

But, this won't work at all unless x was a mutable binding to begin with. We would need a way around this.

A basic NRVO pass would eliminate this move while allowing us to keep the desugaring the same. That might be a better approach here.


The second issue is easier to fix: Eliminating all moves from MIR actually breaks the existing generator optimizations. The RequiresStorage analysis does not handle drop specially, which is another can of worms (#61015).

One way to fix this I mentioned in #62508 (comment) is to emit StorageDead along both return edges of our drop. Another way is to lower directly to a special intrinsic used only from await (which I think is what @eddyb was asking for), and give that the semantics we want.

@tmandry tmandry closed this Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop with guaranteed move elision
6 participants