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

Drop with guaranteed move elision #62508

Open
tmandry opened this issue Jul 8, 2019 · 4 comments
Open

Drop with guaranteed move elision #62508

tmandry opened this issue Jul 8, 2019 · 4 comments
Labels
A-destructors Area: Destructors (`Drop`, …) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jul 8, 2019

Today, calling drop(_1) results in the following MIR:

_2 = move _1;
std::mem::drop::<T>(_2) -> [return .., unwind ..];

We could instead inline this directly to a MIR drop, with no move. This makes it simpler to do some optimizations in MIR.

This issue specifically tracks having such a drop available in HIR lowering; whether or not to bless std::mem::drop with this guarantee is up for discussion.

cc @cramertj @withoutboats

@tmandry
Copy link
Member Author

tmandry commented Jul 8, 2019

await is my primary motivation here, but I'm sure there are others. Today, foo.await; translates roughly to

{
  let pinned = foo;
  // use `Pin::new_unchecked(&mut pinned)`
}  // `pinned` implicitly dropped here

Implicit drops at the end of scope are lowered directly to MIR drop. Because this does not involve moving, the invariants around Pin::new_unchecked are satisfied. However, if we could write it like this:

{
  // use `Pin::new_unchecked(&mut foo)`
  drop(foo);
}

and calling std::mem::drop(foo) was guaranteed not to move foo, then we would get rid of all moves. This would solve #59087 and #62321 with respect to await.

We also need to emit StorageDead on both the return and unwind edges of our MIR drop, or we'll break generator optimizations merged in #60187. (See #61015)

@tmandry
Copy link
Member Author

tmandry commented Jul 9, 2019

I would guess that adding this guarantee to std::mem::drop is going to require an RFC (cc @rust-lang/lang). If so, let's define a separate unstable function that we can make the guarantee for. As long as we can invoke it from HIR lowering, that would unblock the case I care most about.

@jonas-schievink jonas-schievink added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 10, 2019
@scottmcm
Copy link
Member

How close would it get if drop were just marked [inline(always)]? (Assuming the perf of running the mir inliner for that is acceptable, since there's no body so it'd be hopefully easy.)

tmandry added a commit to tmandry/rust that referenced this issue Jul 22, 2019
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.
@cramertj
Copy link
Member

@scottmcm I definitely wouldn't consider the possibility of inlining and optimization enough to be relied on for soundness of any async fn, especially in debug mode.

@jonas-schievink jonas-schievink added A-destructors Area: Destructors (`Drop`, …) C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 22, 2020
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants