Skip to content

Visiting a MIR return should visit the return place too #72032

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

Closed
jonas-schievink opened this issue May 9, 2020 · 1 comment · Fixed by #72048
Closed

Visiting a MIR return should visit the return place too #72032

jonas-schievink opened this issue May 9, 2020 · 1 comment · Fixed by #72048
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations 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

@jonas-schievink
Copy link
Contributor

The return terminator conceptually moves out of the return place _0, but this is not currently reflected by the visitor infrastructure.

This can lead to incorrect dataflow solutions for _0. For example, MaybeLiveLocals reports that _0 is dead before a return, which is wrong (it must always be live there). These don't currently cause any harm, since use of _0 is fairly limited, but they are an issue for destination propagation / NRVO, which can propagate _0 to arbitrary places in the function and needs to have correct information to do so soundly.

The simple fix of calling visit_local from within super_terminator_kind has the problem that the MutVisitor receives a &mut Local, but the local return reads from can not be changed. This can be caught with an assertion, but maybe there is a better solution for this?

cc @rust-lang/wg-mir-opt

@jonas-schievink jonas-schievink added 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. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations labels May 9, 2020
@oli-obk
Copy link
Contributor

oli-obk commented May 9, 2020

This can be caught with an assertion, but maybe there is a better solution for this?

I think any alternative will be more complex than that simple assertion. Conceptually the TerminatorKind::Return should contain a Local which is always _0, but then we'd need to have assertions keeping that in place, so you don't gain anything.

I don't think we can generalize returning to allow using different places than _0, which is the only way we'd get around having to assert something somewhere.

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 A-mir-opt Area: MIR optimizations 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.

2 participants