-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Generate StorageLive after DropAndReplace for locals #61060
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
@bors try (for perf) @RalfJung WRT correctness of this change, is it guaranteed that a place that is treated as Do we have any kind of documentation for these sorts of guarantees on MIR? |
⌛ Trying commit 201b464 with merge 8044de50b3d0a2ec814b74c2c61416e712b4ba9c... |
💥 Test timed out |
also cc @pnkfelix @nikomatsakis @arielb1 |
@bors rollup |
No. It will be at a different location in memory, in general (and in Miri). It is quite explicitly the goal of the Without this, your generator optimization would be unsound (as the location could be revived after the So, this change is incorrect and does not help to solve the issue, I am afraid. |
Closing in favor of #61373. |
Emit StorageDead along unwind paths for generators Completion of the work done in #60840. That PR made a change to implicitly consider a local `StorageDead` after Drop, but that was incorrect for DropAndReplace (see also #61060 which tried to fix this in a different way). This finally enables the optimization implemented in #60187. r? @eddyb cc @Zoxc @cramertj @RalfJung
Emit StorageDead along unwind paths for generators Completion of the work done in #60840. That PR made a change to implicitly consider a local `StorageDead` after Drop, but that was incorrect for DropAndReplace (see also #61060 which tried to fix this in a different way). This finally enables the optimization implemented in #60187. r? @eddyb cc @Zoxc @cramertj @RalfJung
…nd, r=eddyb Emit StorageDead along unwind paths for generators Completion of the work done in rust-lang#60840. That PR made a change to implicitly consider a local `StorageDead` after Drop, but that was incorrect for DropAndReplace (see also rust-lang#61060 which tried to fix this in a different way). This finally enables the optimization implemented in rust-lang#60187. r? @eddyb cc @Zoxc @cramertj @RalfJung
Emit StorageDead along unwind paths for generators Completion of the work done in #60840. That PR made a change to implicitly consider a local `StorageDead` after Drop, but that was incorrect for DropAndReplace (see also #61060 which tried to fix this in a different way). This finally enables the optimization implemented in #60187. r? @eddyb cc @Zoxc @cramertj @RalfJung
Emit StorageDead along unwind paths for generators Completion of the work done in #60840. That PR made a change to implicitly consider a local `StorageDead` after Drop, but that was incorrect for DropAndReplace (see also #61060 which tried to fix this in a different way). This finally enables the optimization implemented in #60187. r? @eddyb cc @Zoxc @cramertj @RalfJung
Fixes a problem in #60840 where I forgot to consider DropAndReplace.
That PR assumes that a MIR Drop terminator implies StorageDead along both edges for locals. During drop elaboration, we turn DropAndReplace into a Drop followed by an assignment, which would incorrectly then be seen as an assignment to a local that's StorageDead. To compensate, we declare that the local is StorageLive again before assigning.
I did some local benchmarking on this change and didn't see any regressions. I wasn't running the full suite, however.
I have a test which reproduces the problem. It will be included in #60187, since I need that optimization in place for the problem to actually manifest.