-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Enable instcombine for mutable reborrows #105274
Conversation
r? @fee1-dead (rustbot has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit d966a3b69baf8b6a60918783f95170f552b79db2 with merge ad3c92c23998265b250e84072921bcacbe08907e... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (ad3c92c23998265b250e84072921bcacbe08907e): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
The regressions are in |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Hmm. Can we confirm that this generates better assembly? Otherwise LLVM might be doing unnecessary work. |
Let's get a second opinion on this. I would approve it if it did not regress, but now I don't know if it is the most optimal thing to do.. r? compiler |
To your previous comment, I have looked through things as best I can. A diff of I also looked around the ecosystem at a few other crates. In a few cases I found microbenchmarks whose runtimes seem to have changed with this PR, but if I inspect the assembly for the benchmark, I see no changes at all. I wouldn't be surprised if these changes are wall time changes due to code layout shifts in the criterion or libtest runtime, perhaps perturbing the alignment of the benchmark loop. The rustc-perf runtime benchmarks are exactly the same before and after this PR. |
Is this compatible with stacked borrows? AFAIK reborrows have a semantic meaning. |
That sounds like a good question for t-opsem 😉 (I am resisting the urge to put together an informal proof that this is valid, there is much else I would like to do) The fact that the aliasing model cares about this doesn't necessarily mean we can't remove it in a MIR optimization. These optimizations can and do rely on the input program not executing UB, and are only obligated to not add UB. I would be surprised if this optimization were legal for non-mutable reborrows but not mutable reborrows. Especially considering the comment this PR removes. |
d966a3b
to
f946ab9
Compare
Rebased away merge conflict, changing reviewer to Oli because you keep r?'ing yourself on my other MIR opt PRs. r? @oli-obk |
Failed to set assignee to
|
⌛ Trying commit 1409cb5 with merge c5bb1d862806b46d162d2c26fc57fc5f2ef20fc8... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c5bb1d862806b46d162d2c26fc57fc5f2ef20fc8): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Hmm, that's a lot more LLVM work in cranelift-codegen and some regressions in check builds. I think the regressions in opt-full builds are due to extra MIR inlining. Probably the LLVM inlining got bumped as well. I'll study the cachegrind diffs and MIR diffs about 7 hours from now. But I think there may be an argument for merging as-is, it seems unlikely that the big regression is actionable. |
I cannot find any MIR inlining differences in cachegrind diffs for the check regressions point at a smattering of functions. I've looked into |
@cjgillot With this PR, there are some copies of
|
@saethlin which function's MIR are you showing? I can't read the span nor its name, truncated by the copy-paste. |
Wow yeah I really truncated that didn't I. I updated the comment, and the function is |
It's a shortcoming of the SSA analysis. Based on a visitor, it checks for |
That makes sense. I've always though the |
I agree with merging this PR as is. In addition, we have a gain up to 3% in terms of binary size and metadata size. I'll propose some improvement to @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (231bcd1): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Do not consider `&mut *x` as mutating `x` in `CopyProp` This PR removes an unfortunate overly cautious case from the current implementation. Found by rust-lang#105274 cc `@saethlin`
Do not consider `&mut *x` as mutating `x` in `CopyProp` This PR removes an unfortunate overly cautious case from the current implementation. Found by rust-lang/rust#105274 cc `@saethlin`
instcombine
used to contain this comment, which is no longer accurate because there it is fine to copy&mut _
in MIR:// The dereferenced place must have type `&_`, so that we don't copy `&mut _`.
So let's try replacing that check with something much more permissive...