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

Only enable ConstProp on opt level >= 1 #98961

Merged
merged 2 commits into from
Jul 9, 2022
Merged

Conversation

zeevm
Copy link

@zeevm zeevm commented Jul 5, 2022

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@zeevm zeevm changed the title only enable ConstProp on opt level 2 Only enable ConstProp on opt level 2 Jul 5, 2022
@zeevm
Copy link
Author

zeevm commented Jul 5, 2022

Fixes #98958

@rust-log-analyzer

This comment has been minimized.

@JakobDegen
Copy link
Contributor

Thanks for doing this!

Implementation looks good, only thing I'd suggest is running this on opt level 1 and above instead of 2. This is in line with what we currently do and const prop actually usually helps debug info I believe (see the ConstDebugInfo pass or whatever it's called).

r? @oli-obk as well

If you're interested in some more things to do, following up on this by doing some deduplication between the lint and non-lint versions of this pass is definitely a good next step. There's also some good opportunities around adding support for not invalidating CFG caches to VisitorMut and MirPatch - open a topic on Zulip if you're interested.

@zeevm
Copy link
Author

zeevm commented Jul 5, 2022

Thanks Jakob, changed to opt level 1.

Will look into those 2 issues next.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Jul 6, 2022

⌛ Trying commit 728fb05 with merge 1099b7f9b713a844ae847af1c5268131abfa9745...

@bors
Copy link
Contributor

bors commented Jul 6, 2022

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

@rust-timer
Copy link
Collaborator

Queued 1099b7f9b713a844ae847af1c5268131abfa9745 with parent f342bea, future comparison URL.

@klensy
Copy link
Contributor

klensy commented Jul 6, 2022

Current title is wrong, it should be >= 1, not == 2?

@zeevm zeevm changed the title Only enable ConstProp on opt level 2 Only enable ConstProp on opt level >= 1 Jul 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1099b7f9b713a844ae847af1c5268131abfa9745): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
0.6% 0.6% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 0.6% 0.6% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.1% -0.1% 1
Improvements 🎉
(secondary)
-3.2% -3.2% 1
All 😿🎉 (primary) -0.1% -0.1% 1

Cycles

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

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 6, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2022

📌 Commit 728fb05 has been approved by oli-obk

@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 Jul 6, 2022
@zeevm
Copy link
Author

zeevm commented Jul 6, 2022

Should I squash the commits or will it happen when the PR completes?

@bjorn3
Copy link
Member

bjorn3 commented Jul 6, 2022

Bors doesn't squash commits. Squashing is fine. I can re-approve it after you squash if you do it within the next 5 min or so. Otherwise keeping the commits separate is fine.

@klensy
Copy link
Contributor

klensy commented Jul 6, 2022

Should I squash the commits or will it happen when the PR completes?

There is no auto squash.

@zeevm
Copy link
Author

zeevm commented Jul 6, 2022

Bors doesn't squash commits. Squashing is fine. I can re-approve it after you squash if you do it within the next 5 min or so. Otherwise keeping the commits separate is fine.

that's fine, I'll leave it as is.

I thought I had to squash it to complete the PR.

@bjorn3
Copy link
Member

bjorn3 commented Jul 6, 2022

Once a reviewer posts an r+ comment, your PR is added to the bors queue (https://bors.rust-lang.org/queue/rust) Bors takes the first PR in the queue, merges it into a side branch and once CI passes it merges into the master branch. This ensures that CI always passes for the master branch and there are no unintended interactions between PR's that are merged close to each other.

@bors
Copy link
Contributor

bors commented Jul 9, 2022

⌛ Testing commit 728fb05 with merge db78ab7...

@bors
Copy link
Contributor

bors commented Jul 9, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2022
@bors bors merged commit db78ab7 into rust-lang:master Jul 9, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 9, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (db78ab7): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-1.7% -1.9% 4
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-1.2% -2.3% 2
Improvements 🎉
(secondary)
-3.6% -4.2% 2
All 😿🎉 (primary) -1.2% -2.3% 2

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
2.3% 2.6% 2
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.3% 2.6% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

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.

9 participants