-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Unable to optimize out branches in drop function in unwind landing pad #87055
Comments
I'd say the main problem here is that drop_in_place isn't inlined. If it were, it would be optimized away. |
#69715 tracks alternative MIR solution which is to track active enum variants during drop elaboration. |
After taking a closer look, the reason it doesn't get inlined is that we explicitly mark it as noinline here:
Though argument promotion doesn't happen either, not sure why that is. |
It's aiming at |
Apparently because a bitcast is sitting before the load.
|
The idea is to solve a data flow problem that answers which enum variants might reach a drop terminator. In the case here, we know that Option doesn't implement Drop, and that the only variant that might reach drop is None which doesn't contain anything that needs to be dropped either. Thus it would be valid to remove the drop terminator. This is de-facto implemented as part of const validation which statically ensures that there are no live drops. Following const variant already compiles successfully (even though destructors cannot be evaluated at compile-time): pub const fn foo() {
let _var: Option<Box<i32>> = None;
g();
}
pub const fn g() {} |
Seems this issue has the same root cause as #46515. And the LLVM issue of heavily inlining, mentioned in #41696, which let us mark drop in unwinding noinline, is still NOT fixed. https://bugs.llvm.org/show_bug.cgi?id=33072
My point is that the MIR approach cannot handle manual drop impl which conditional does nothing. This code cannot have pub fn foo(g: fn()) {
let _var = String::new(); // This does not alloc.
g();
} |
Revert "Auto merge of rust-lang#92419 - erikdesjardins:coldland, r=nagisa" Should fix (untested) rust-lang#94390 Reopens rust-lang#46515, rust-lang#87055 r? `@ehuss`
bors also closing this issue in #95338 was unexpected, cc @nagisa @erikdesjardins. |
…=nikic Rebased: Mark drop calls in landing pads cold instead of noinline I noticed that certain inlining optimizations were missing while staring at some compiled code output. I'd like to see this relanded, so I rebased the PR from ``@erikdesjardins`` (PR rust-lang#94823). This PR reapplies rust-lang#92419, which was reverted in rust-lang#94402 due to rust-lang#94390. Fixes rust-lang#46515, fixes rust-lang#87055. Update: fixes rust-lang#97217.
…ikic Rebased: Mark drop calls in landing pads cold instead of noinline I noticed that certain inlining optimizations were missing while staring at some compiled code output. I'd like to see this relanded, so I rebased the PR from `@erikdesjardins` (PR rust-lang#94823). This PR reapplies rust-lang#92419, which was reverted in rust-lang#94402 due to rust-lang#94390. Fixes rust-lang#46515, fixes rust-lang#87055. Update: fixes rust-lang#97217.
Rebased: Mark drop calls in landing pads cold instead of noinline I noticed that certain inlining optimizations were missing while staring at some compiled code output. I'd like to see this relanded, so I rebased the PR from `@erikdesjardins` (PR #94823). This PR reapplies rust-lang/rust#92419, which was reverted in rust-lang/rust#94402 due to rust-lang/rust#94390. Fixes rust-lang/rust#46515, fixes rust-lang/rust#87055. Update: fixes #97217.
In this code:
Assembly:
_var
is known to beNone
so the deallocation is optimized away in the normal flow. But in landing pad for unwinding, it still generates a branch to check if_var
is null. But it's a local variable which is not able to be modified in anywhere else and should be known to be null.I'm expecting the
drop
of_var
should be completely elided so thatg()
can be a tail call.The text was updated successfully, but these errors were encountered: