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

MIR-opt: Generalize &(*_1) optimization to mutable references #76802

Closed

Conversation

simonvandel
Copy link
Contributor

Running RUSTC_LOG=rustc_mir::transform::instcombine ./x.py test --stage 1 src/test/mir-opt --bless | grep "replacing \`&mut*" | wc -l yields 10601, so it seems to be a fairly usable optimization on rust-std.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Sep 16, 2020
@simonvandel
Copy link
Contributor Author

Hmm I just saw 309661e where this was reverted. Not sure why yet

@jonas-schievink
Copy link
Contributor

Because of #72797

@simonvandel
Copy link
Contributor Author

So the problem is that it is only valid to optimize _2 = &mut (*_1) into _2 = move _1 iff _1 is not used later on? I'm not that familiar with the dataflow analysises. How difficult is it to check if _1 has later uses?

Alternatively, I'll close this PR - but it's more fun to drive this to something sound :)

@jonas-schievink
Copy link
Contributor

You'd have to compute liveness of locals, which isn't too difficult. But since this optimization had no measurable performance benefits anyways, I'm not sure that's worth it (running dataflow might slightly regress performance).

@simonvandel
Copy link
Contributor Author

I'm curious about the measured performance and couldn't find anything on #72093. Do you remember where the perf run was performed?

@jonas-schievink
Copy link
Contributor

That was a different issue, the fix was in #72820

@simonvandel
Copy link
Contributor Author

Thanks. The results surprise me. I'm wondering if things have changed since then - it seemed like the observed changes could be explained by different CGU partitioning. Taking your rustc experience in mind, do you think it is worth continuing with this PR, or should I just close it?

@jonas-schievink
Copy link
Contributor

Hmm, I would probably not make this use dataflow at this point. Maybe if we can use the dataflow results to do more things this would be more worthwhile.

@jyn514 jyn514 added A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2020
@simonvandel simonvandel marked this pull request as draft September 17, 2020 06:46
@bors
Copy link
Contributor

bors commented Sep 20, 2020

☔ The latest upstream changes (presumably #72632) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@Dylan-DPC-zz
Copy link

thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
@jyn514 jyn514 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-inactive labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants