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

More precise drop elaboration #69715

Open
1 of 2 tasks
ecstatic-morse opened this issue Mar 4, 2020 · 1 comment
Open
1 of 2 tasks

More precise drop elaboration #69715

ecstatic-morse opened this issue Mar 4, 2020 · 1 comment
Assignees
Labels
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. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Mar 4, 2020

#68943 and #68528 changed drop elaboration to reduce the number of Drop terminators emitted for enums. Originally, these PRs were intended only to solve #66753, but they also resulted in significant perf improvements for codegen-ed builds (look only at the "clean" runs).

It may be possible to find more perf wins by making drop elaboration more precise. Notably, #68528 added a discriminant_switch_effect to MaybeInitializedPlaces but did not make the equivalent change to MaybeUninitializedPlaces. As a result, some frivolous drop terminators remain after drop elaboration (e.g., bb5 in this recreation of Option::unwrap). This occurs because the move path for the local containing the Option is calculated to be "maybe live" by drop elaboration, but is determined not to be "maybe dead" since MaybeUninitializedLocals does not mark the move path for the Option::Some(_) variant as uninitialized. As a result, elaboration considers the drop of Option<T> along the unwind edge as "static" instead of "open". Only "open" drops trigger the new code introduced in #68943 for fieldless enum variants such as Option::None.

On the other hand, marking more drops of enums as "open" ones could increase the amount of MIR we need to codegen. If we can't prove that they are frivolous, open drops cause drop flags to be created within a function's MIR, while static drops simply invoke the drop shim. I'm not confident enough in my understanding of drop elaboration to be sure of this, however.

MaybeUninitializedLocals is also used in the borrow-checker, so care should be taken when modifying it. In the worst case, we could use a different version of that analysis for drop elaboration than for borrow-checking. However, using a more precise variant of the analysis during borrow-checking may have positive effects.

@eddyb also suggested incorporating information about the active enum variant during drop elaboration, similar to what NeedsDrop does for const-checking. If this were implemented, we could eliminate all drops of an enum that occur after it is assigned a variant that needs no drop glue (e.g. Option::None) with no intervening assignment. I'm more skeptical of this approach than the first, since it becomes pretty useless as soon as a pointer exists that could mutate the place (unless we are willing to do a much more complex alias analysis). Perhaps they disagree?

In summary, I think we (myself and possibly @rust-lang/wg-mir-opt) should investigate the following ideas, with the first one given higher priority:

@ecstatic-morse ecstatic-morse added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Mar 4, 2020
@jonas-schievink jonas-schievink added the I-compiletime Issue: Problems and improvements with respect to compile times. label Mar 4, 2020
@ecstatic-morse ecstatic-morse self-assigned this Mar 4, 2020
@rodrimati1992

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 5, 2020
…=oli-obk

Handle inactive enum variants in `MaybeUninitializedPlaces`

Resolves the first part of rust-lang#69715.

This is the equivalent of rust-lang#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"](https://github.com/rust-lang/rust/blob/e0e5d82e1677c82d209b214bbfc2cc5705c2336a/src/librustc_mir/transform/elaborate_drops.rs#L191). Unlike in rust-lang#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants