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

Disable nrvo mir opt #111007

Merged
merged 1 commit into from
May 8, 2023
Merged

Disable nrvo mir opt #111007

merged 1 commit into from
May 8, 2023

Conversation

JakobDegen
Copy link
Contributor

See #111005 and #110902 . The ICE can definitely be hit on stable, the miscompilation I'm not sure about. The pass makes some pretty sketchy assumptions though, and we should not have it on while that's the case.

I'm not going to work on actually fixing this, it's probably not excessively difficult though.

r? rust-lang/mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 30, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor

Should we remove this pass entirely, and let dest-prop do it instead?

@JakobDegen
Copy link
Contributor Author

If it were on by default, yes. Until then, I'm not so sure

@tmiasko
Copy link
Contributor

tmiasko commented May 2, 2023

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 2, 2023

📌 Commit 74d9567c3eb52f3cb0ba748e285eab860f507013 has been approved by tmiasko

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2023
@bors
Copy link
Contributor

bors commented May 2, 2023

⌛ Testing commit 74d9567c3eb52f3cb0ba748e285eab860f507013 with merge 76faaae3124d4f4ecf5310e32c27f65d768dbfc9...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 2, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 2, 2023
@tmiasko
Copy link
Contributor

tmiasko commented May 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2023

📌 Commit 02d4c2f4e5d7bf06f2100a2f7cc3b5f4c0828d1b has been approved by tmiasko

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2023
@bors
Copy link
Contributor

bors commented May 4, 2023

⌛ Testing commit 02d4c2f4e5d7bf06f2100a2f7cc3b5f4c0828d1b with merge 0bd1485d726d4b5f29d0ce904b3a1c7540f060ac...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 4, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 4, 2023
@tmiasko
Copy link
Contributor

tmiasko commented May 8, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2023

📌 Commit 8e2da80 has been approved by tmiasko

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2023
@bors
Copy link
Contributor

bors commented May 8, 2023

⌛ Testing commit 8e2da80 with merge dfe3188...

@bors
Copy link
Contributor

bors commented May 8, 2023

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing dfe3188 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 8, 2023
@bors bors merged commit dfe3188 into rust-lang:master May 8, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dfe3188): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.2%, 2.1%] 109
Regressions ❌
(secondary)
0.5% [0.1%, 0.9%] 55
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [0.2%, 2.1%] 109

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
1.2% [1.2%, 1.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.5% [-2.3%, -1.1%] 5
Improvements ✅
(secondary)
-3.2% [-3.8%, -2.7%] 2
All ❌✅ (primary) -1.1% [-2.3%, 1.2%] 6

Cycles

Results

This 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.

mean range count
Regressions ❌
(primary)
1.0% [0.8%, 1.7%] 7
Regressions ❌
(secondary)
2.6% [2.0%, 3.8%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [0.8%, 1.7%] 7

Binary size

Results

This 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.

mean range count
Regressions ❌
(primary)
0.3% [0.0%, 1.9%] 49
Regressions ❌
(secondary)
0.1% [0.0%, 0.2%] 13
Improvements ✅
(primary)
-0.1% [-0.3%, -0.0%] 18
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.3%, 1.9%] 67

Bootstrap: 656.767s -> 658.118s (0.21%)

@rustbot rustbot added the perf-regression Performance regression. label May 8, 2023
@JakobDegen JakobDegen deleted the nrvo branch May 9, 2023 07:23
@nnethercote
Copy link
Contributor

Lots of regressions here. It sure would be nice if these could be regained, via dest-prop or any other means.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2023
Fortify and re-enable RNVO MIR opt

Fixes rust-lang#111005
Fixes rust-lang#110902

This PR re-enables NRVO opt that had been disabled in rust-lang#111007

To look for RVO opportunities, we walk the MIR backwards from `return` terminators. In addition to the former implementation, we look at all the traversed statements and terminators for writes. If a local is written-to or moved-from, we discard it from the RVO candidates (`assigned_locals` bitset). If we see an indirect write or move, we discard all borrowed locals from candidates.

cc `@JakobDegen`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants