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.

Sign up for GitHub

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

Jump to bottom

Enable DestinationPropagation by default. #115105

Merged
merged 1 commit into from
May 30, 2024
Merged

Enable DestinationPropagation by default. #115105

merged 1 commit into from
May 30, 2024

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Aug 22, 2023 •
edited
Loading

Based on #115291.

This PR proposes to enable the destination propagation pass by default.
This pass is meant to reduce the amount of copies present in MIR.

At the same time, this PR removes the RenameReturnPlace pass, as it is currently unsound.
DestinationPropagation is not limited to _0, but does not handle borrowed locals.

@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 Aug 22, 2023
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Trying commit 3a7cd3992c305fc85f47c3f8b1529a190578558f with merge bc959a66e49afdd420b5983051c6fe0004b08764...

@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Try build successful - checks-actions
Build commit: bc959a66e49afdd420b5983051c6fe0004b08764 (bc959a66e49afdd420b5983051c6fe0004b08764)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bc959a66e49afdd420b5983051c6fe0004b08764): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

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)		 1.6% [0.4%, 6.8%] 12
Regressions ❌
(secondary)		 2.2% [0.3%, 3.4%] 5
Improvements ✅
(primary)		 -0.6% [-1.8%, -0.2%] 36
Improvements ✅
(secondary)		 -0.6% [-1.1%, -0.3%] 33
All ❌✅ (primary) -0.1% [-1.8%, 6.8%] 48

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)		 4.2% [0.1%, 7.9%] 3
Regressions ❌
(secondary)		 3.0% [1.7%, 4.5%] 4
Improvements ✅
(primary)		 -2.2% [-5.3%, -0.6%] 6
Improvements ✅
(secondary)		 -2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) -0.1% [-5.3%, 7.9%] 9

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)		 3.2% [1.4%, 5.0%] 3
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.5% [-4.6%, -1.0%] 8
Improvements ✅
(secondary)		 -2.6% [-3.3%, -1.8%] 7
All ❌✅ (primary) -1.0% [-4.6%, 5.0%] 11

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.4% [0.0%, 1.2%] 23
Regressions ❌
(secondary)		 0.3% [0.1%, 2.8%] 12
Improvements ✅
(primary)		 -0.4% [-1.8%, -0.0%] 75
Improvements ✅
(secondary)		 -0.8% [-1.4%, -0.0%] 17
All ❌✅ (primary) -0.2% [-1.8%, 1.2%] 98

Bootstrap: 634.51s -> 634.058s (-0.07%)
Artifact size: 346.98 MiB -> 347.11 MiB (0.04%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 23, 2023
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 26, 2023
@bors
Copy link
Contributor

bors commented Aug 26, 2023

⌛ Trying commit 388804de0e1798da7828759ac2f0c52164e7f2b1 with merge 249071430ed663b2a6c13c118a88c73c03c35050...

@bors
Copy link
Contributor

bors commented Aug 26, 2023

☀️ Try build successful - checks-actions
Build commit: 249071430ed663b2a6c13c118a88c73c03c35050 (249071430ed663b2a6c13c118a88c73c03c35050)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (249071430ed663b2a6c13c118a88c73c03c35050): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

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)		 1.9% [0.6%, 6.1%] 13
Regressions ❌
(secondary)		 3.0% [0.2%, 11.1%] 10
Improvements ✅
(primary)		 -0.6% [-1.7%, -0.2%] 63
Improvements ✅
(secondary)		 -0.7% [-1.5%, -0.3%] 37
All ❌✅ (primary) -0.1% [-1.7%, 6.1%] 76

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)		 3.7% [0.2%, 7.3%] 4
Regressions ❌
(secondary)		 2.8% [1.9%, 3.8%] 5
Improvements ✅
(primary)		 -2.4% [-2.5%, -2.2%] 2
Improvements ✅
(secondary)		 -2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) 1.7% [-2.5%, 7.3%] 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.8% [1.1%, 3.5%] 6
Regressions ❌
(secondary)		 3.6% [3.3%, 3.8%] 2
Improvements ✅
(primary)		 -3.0% [-5.0%, -1.2%] 7
Improvements ✅
(secondary)		 -3.1% [-4.5%, -2.0%] 9
All ❌✅ (primary) -0.8% [-5.0%, 3.5%] 13

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.2%] 20
Regressions ❌
(secondary)		 0.3% [0.1%, 2.8%] 12
Improvements ✅
(primary)		 -0.4% [-1.8%, -0.0%] 75
Improvements ✅
(secondary)		 -0.8% [-1.4%, -0.0%] 17
All ❌✅ (primary) -0.2% [-1.8%, 1.2%] 95

Bootstrap: 630.77s -> 632.971s (0.35%)
Artifact size: 316.08 MiB -> 316.02 MiB (-0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 27, 2023
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 27, 2023
@bors
Copy link
Contributor

bors commented Aug 27, 2023

⌛ Trying commit 4fc796bfabd8481ec2b89224030e92f3b7346494 with merge e8ecd4bd0b0f35deb36c1d1baee03c268646c4f1...

@bors
Copy link
Contributor

bors commented Feb 22, 2024

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

@wesleywiser
Copy link
Member

r? mir-opt

@rustbot rustbot assigned oli-obk and unassigned tmiasko Mar 21, 2024
@cjgillot cjgillot force-pushed the dest-prop-default branch from 70d3cae to aca6562 Compare April 7, 2024 15:01
@cjgillot cjgillot force-pushed the dest-prop-default branch from aca6562 to 63555ae Compare May 4, 2024 19:27
@bors
Copy link
Contributor

bors commented May 13, 2024

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

@oli-obk
Copy link
Contributor

oli-obk commented May 28, 2024

logic wise this is fine. Performance wise it's not ideal, but at least we have a FIXMEs to work on and a better pass to edit. r=me after another rebase

@cjgillot cjgillot force-pushed the dest-prop-default branch from 63555ae to 5fa0ec6 Compare May 30, 2024 00:03
@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2024

📌 Commit 5fa0ec6 has been approved by oli-obk

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 30, 2024
@bors
Copy link
Contributor

bors commented May 30, 2024

⌛ Testing commit 5fa0ec6 with merge cfb7304...

@bors
Copy link
Contributor

bors commented May 30, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing cfb7304 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2024
@bors bors merged commit cfb7304 into rust-lang:master May 30, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 30, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cfb7304): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)		 1.0% [0.3%, 3.4%] 18
Regressions ❌
(secondary)		 1.3% [0.3%, 3.3%] 22
Improvements ✅
(primary)		 -0.5% [-4.0%, -0.2%] 23
Improvements ✅
(secondary)		 -0.8% [-1.6%, -0.2%] 18
All ❌✅ (primary) 0.2% [-4.0%, 3.4%] 41

Max RSS (memory usage)

Results (primary 1.2%, secondary 2.8%)

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)		 4.9% [2.6%, 8.1%] 6
Regressions ❌
(secondary)		 2.8% [1.6%, 4.8%] 3
Improvements ✅
(primary)		 -3.3% [-6.2%, -1.9%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.2% [-6.2%, 8.1%] 11

Cycles

Results (primary 1.3%, secondary 4.5%)

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.9% [1.2%, 2.6%] 8
Regressions ❌
(secondary)		 4.5% [1.8%, 6.6%] 9
Improvements ✅
(primary)		 -3.3% [-3.3%, -3.3%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.3% [-3.3%, 2.6%] 9

Binary size

Results (primary -0.1%, secondary 0.2%)

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.1% [0.0%, 0.6%] 29
Regressions ❌
(secondary)		 0.4% [0.0%, 6.5%] 54
Improvements ✅
(primary)		 -0.2% [-0.7%, -0.0%] 53
Improvements ✅
(secondary)		 -0.8% [-1.5%, -0.1%] 13
All ❌✅ (primary) -0.1% [-0.7%, 0.6%] 82

Bootstrap: 666.558s -> 669.847s (0.49%)
Artifact size: 318.78 MiB -> 318.77 MiB (-0.00%)

@ehuss
Copy link
Contributor

ehuss commented May 30, 2024

@cjgillot I believe this PR has caused the Building stage1 codegen backend gcc step in the PR builder to regress from taking about 45s to taking about 80m. Would it be possible to revert if an immediate fix isn't available.

Discussion at https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/x86_64-gnu-llvm-17.20job.20being.20really.20slow

@cjgillot cjgillot deleted the dest-prop-default branch May 31, 2024 00:20
cjgillot added a commit to cjgillot/rust that referenced this pull request May 31, 2024
@cjgillot
Revert "Auto merge of rust-lang#115105 - cjgillot:dest-prop-default, …
e110567 
…r=oli-obk"

This reverts commit cfb7304, reversing
changes made to 91c0823.
@cjgillot cjgillot mentioned this pull request May 31, 2024
Merged
@cjgillot
Copy link
Contributor Author

Posted #125794

@fmease fmease mentioned this pull request May 31, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
@bors
Auto merge of rust-lang#125794 - cjgillot:revert-115105, r=compiler-e…
366da30 
…rrors

Revert "Auto merge of rust-lang#115105 - cjgillot:dest-prop-default, r=oli-obk"

This reverts commit cfb7304, reversing changes made to 91c0823.

To address rust-lang#115105 (comment)
r? `@oli-obk`

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
@pnkfelix
Copy link
Member

pnkfelix commented Jun 4, 2024

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klensy klensy klensy left review comments

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.