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

Always permit ConstProp to exploit arithmetic identities #106340

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jan 1, 2023

Fixes #72751

Initially, I thought I would need to enable operand propagation then do something else, but actually #74491 already has the fix for the issue in question! It looks like this optimization was put under MIR opt level 3 due to possible soundness/stability implications, then demoted further to MIR opt level 4 when MIR opt level 2 became associated with --release.

Perhaps in the past we were doing CTFE on optimized MIR? We aren't anymore, so this optimization has no stability implications.

r? @oli-obk

@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 Jan 1, 2023
@saethlin
Copy link
Member Author

saethlin commented Jan 1, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 1, 2023
@bors
Copy link
Contributor

bors commented Jan 1, 2023

⌛ Trying commit 9d69572fb3d2e01a3416eda9c100ee8214f2bb7d with merge c4c846ffd62c360cb758c2a53e7be1d294fe4aea...

@bors
Copy link
Contributor

bors commented Jan 1, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c4c846ffd62c360cb758c2a53e7be1d294fe4aea): 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)
0.5% [0.2%, 0.9%] 19
Regressions ❌
(secondary)
0.4% [0.3%, 0.6%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-1.3%, -1.3%] 2
All ❌✅ (primary) 0.5% [0.2%, 0.9%] 19

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)
5.9% [3.0%, 9.8%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.6% [-2.2%, 9.8%] 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.9% [1.9%, 1.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-1.8%, -1.3%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.7% [-1.8%, 1.9%] 4

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 1, 2023
@saethlin
Copy link
Member Author

saethlin commented Jan 1, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 1, 2023
@bors
Copy link
Contributor

bors commented Jan 1, 2023

⌛ Trying commit e25d9701adb52d554c73beae6c9d9efc06b006f8 with merge 8ba671ff5e120f17892e0c8125ad1e347076093a...

@saethlin saethlin changed the title Propagate operands by default Unleash ConstProp a bit Jan 1, 2023
@bors
Copy link
Contributor

bors commented Jan 1, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8ba671ff5e120f17892e0c8125ad1e347076093a): comparison URL.

Overall result: ❌ regressions - no 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.

@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)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 2, 2023
@saethlin saethlin changed the title Unleash ConstProp a bit Always permit ConstProp to use arithmetic identities Jan 2, 2023
@saethlin saethlin changed the title Always permit ConstProp to use arithmetic identities Always permit ConstProp to exploit arithmetic identities Jan 2, 2023
@saethlin saethlin marked this pull request as ready for review January 2, 2023 04:10
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Jan 9, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2023

📌 Commit 82f0973 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 Jan 9, 2023
@bors
Copy link
Contributor

bors commented Jan 9, 2023

⌛ Testing commit 82f0973 with merge 89e0576...

@bors
Copy link
Contributor

bors commented Jan 9, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 9, 2023
@bors bors merged commit 89e0576 into rust-lang:master Jan 9, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 9, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (89e0576): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -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)
- - 0
Regressions ❌
(secondary)
0.8% [0.7%, 0.8%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.7%, -0.7%] 1
All ❌✅ (primary) - - 0

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)
2.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.9% [2.9%, 2.9%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

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

Suboptimal debug codegen for integer division with a constant rhs
5 participants