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

MaybeRequiresStorage for generators is not necessary #69902

Closed
ecstatic-morse opened this issue Mar 10, 2020 · 2 comments · Fixed by #71956
Closed

MaybeRequiresStorage for generators is not necessary #69902

ecstatic-morse opened this issue Mar 10, 2020 · 2 comments · Fixed by #71956
Assignees
Labels
A-coroutines Area: Coroutines C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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 10, 2020

Continuing from discussion in #69716.

MaybeRequiresStorage (introduced in #60187 as RequiresStorage) is one of four dataflow analyses used to determine what locals are live across yield points or live at the same time as other locals during the MIR generator pass. I don't see any reason we're not using a liveness analysis exclusively for this, with an additional check against MaybeBorrowedLocals ∩ MaybeStorageLive in cases where borrows can live across suspension points (immovable generators).

MaybeRequiresStorage is basically MaybeStorageLive with some additional logic that kills locals after they are moved out of. It also incorporates the MaybeBorrowedLocals analysis as part of its transfer function, which is probably unnecessary, since we could check the results of MaybeBorrowedLocals directly at each yield point. If there is often a large gap between the point at which a local is moved out of and the point at which it is marked StorageDead, we could augment MaybeStorageLive to kill locals when they are moved out of.

The liveness analysis in librustc_mir/util/liveness.rs will implicitly handle moved-from locals as long as there are no future accesses to them, which is what causes a variable to be marked as live.

I'd like refactor the liveness parts of the MIR generator pass, but will wait for buy-in from its authors. I think that basing the current storage optimization on a combination of a few well-known dataflow analyses will make it easier to reason about and therefore fine-tune. This will involve using the liveness analysis (plus MaybeBorrowedLocals / MaybeStorageLive inside immovable generators) for compute_storage_conflicts. Does anyone see a problem with this? @tmandry @jonas-schievink

@ecstatic-morse ecstatic-morse self-assigned this Mar 10, 2020
@jonas-schievink jonas-schievink added A-coroutines Area: Coroutines C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 10, 2020
@RalfJung
Copy link
Member

we could augment MaybeStorageLive to kill locals when they are moved out of.

Note that, until we have figured out MIR semantics more precisely, this could be problematic... depending on how the analysis results are used, anyway. Moving out of a variable and then putting something back in is legal in Miri right now, and is guaranteed to keep the local at the same address, so points to it may remain valid (modulo Stacked Borrows). StorageDead on the other hand is like free for a stack variable, all old pointers to it are definitely dangling now.

If there is often a large gap between the point at which a local is moved out of and the point at which it is marked StorageDead

Maybe another option would be to improve StorageDead generation and move it closer to the move point.

@ecstatic-morse
Copy link
Contributor Author

Moving out of a variable and then putting something back in is legal in Miri right now, and is guaranteed to keep the local at the same address, so points to it may remain valid (modulo Stacked Borrows).

That's good to keep in mind. The current MaybeRequiresStorage is suitably conservative here, so it should behave the same as MaybeStorageLive for borrowed locals.

Maybe another option would be to improve StorageDead generation and move it closer to the move point.

Yeah, I had #68622 in mind when I brought this up. Specifically the hypothetical InvalidateBorrows statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.

3 participants