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

Generate StorageLive after DropAndReplace for locals #61060

Closed
wants to merge 1 commit into from

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented May 22, 2019

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.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2019
@cramertj
Copy link
Member

cramertj commented May 23, 2019

@bors try (for perf)

@RalfJung WRT correctness of this change, is it guaranteed that a place that is treated as StorageDead followed by StorageLive won't be moved to another location in memory? That is, I'd expect assignment to result in a variable that has the same address as the previous place prior to the assignment.

Do we have any kind of documentation for these sorts of guarantees on MIR?

@bors
Copy link
Contributor

bors commented May 23, 2019

⌛ Trying commit 201b464 with merge 8044de50b3d0a2ec814b74c2c61416e712b4ba9c...

@bors
Copy link
Contributor

bors commented May 23, 2019

💥 Test timed out

@Centril
Copy link
Contributor

Centril commented May 23, 2019

Do we have any kind of documentation for these sorts of guarantees on MIR?

also cc @pnkfelix @nikomatsakis @arielb1

@Centril
Copy link
Contributor

Centril commented May 23, 2019

@bors rollup

@RalfJung
Copy link
Member

RalfJung commented May 23, 2019

is it guaranteed that a place that is treated as StorageDead followed by StorageLive won't be moved to another location in memory?

No. It will be at a different location in memory, in general (and in Miri). It is quite explicitly the goal of the Storage* annotations that when a variable is live again, it might be at a different location. This is the only reason why you can assume that, after a StorageDead, all pointers to the location are and always will be unusable.

Without this, your generator optimization would be unsound (as the location could be revived after the drop).

So, this change is incorrect and does not help to solve the issue, I am afraid.

@tmandry
Copy link
Member Author

tmandry commented May 30, 2019

Closing in favor of #61373.

@tmandry tmandry closed this May 30, 2019
@tmandry tmandry deleted the fix-drop-and-replace branch May 30, 2019 20:48
@tmandry tmandry restored the fix-drop-and-replace branch May 30, 2019 20:48
bors added a commit that referenced this pull request May 30, 2019
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
bors added a commit that referenced this pull request May 31, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Jun 4, 2019
…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
bors added a commit that referenced this pull request Jun 5, 2019
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
bors added a commit that referenced this pull request Jun 6, 2019
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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants