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

Do one round of DestProp at mir-opt-level=2 #105577

Closed
wants to merge 1 commit into from
Closed

Do one round of DestProp at mir-opt-level=2 #105577

wants to merge 1 commit into from

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 11, 2022 •
edited
Loading

Based on some brief experimentation with DestinationPropagation, it seems like sometimes we run a lot of rounds, where the first round does a lot more than any of the later rounds. So it seems like doing just the first round is probably a much better compile time tradeoff than doing all of them.

@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 Dec 11, 2022
@saethlin
Copy link
Member 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 Dec 11, 2022
@bors
Copy link
Contributor

bors commented Dec 11, 2022

⌛ Trying commit a586167a54a2505492cbd6f6d66cee28d76670da with merge 8e6760e82a081fb624e777d52ef744c1def2f593...

@bors
Copy link
Contributor

bors commented Dec 12, 2022

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

1 similar comment
@bors
Copy link
Contributor

bors commented Dec 12, 2022

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8e6760e82a081fb624e777d52ef744c1def2f593): 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.0% [0.4%, 1.9%] 5
Regressions ❌
(secondary)		 0.7% [0.2%, 2.4%] 8
Improvements ✅
(primary)		 -1.2% [-3.6%, -0.2%] 124
Improvements ✅
(secondary)		 -1.3% [-6.3%, -0.1%] 75
All ❌✅ (primary) -1.1% [-3.6%, 1.9%] 129

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.9% [1.5%, 7.6%] 8
Regressions ❌
(secondary)		 2.5% [1.7%, 3.3%] 7
Improvements ✅
(primary)		 -2.3% [-16.7%, -0.5%] 48
Improvements ✅
(secondary)		 -2.0% [-2.7%, -1.2%] 14
All ❌✅ (primary) -1.4% [-16.7%, 7.6%] 56

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)		 2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 -1.7% [-3.2%, -0.8%] 78
Improvements ✅
(secondary)		 -2.9% [-5.4%, -1.2%] 52
All ❌✅ (primary) -1.7% [-3.2%, 2.1%] 79

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

@JakobDegen Based on the perf run above, are you interested in this?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@saethlin
Copy link
Member Author

FYI I have no idea why the codegen tests are failing in CI, they pass locally and I can't reproduce the CI failure. I've tried flipping quite a few options in my config and none seem to produce the test failure.

@saethlin saethlin marked this pull request as ready for review December 13, 2022 04:08
@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@saethlin
Copy link
Member Author

r? rust-lang/compiler

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned nagisa Dec 13, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

oh wow, that's an amazing find. typeck and borrowck actually get faster because this causes rustc to improve, offsetting regressions which do work in optimized_mir.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 13, 2022

📌 Commit e2bf4b9 has been approved by oli-obk

It is now in the queue for this repository.

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

@craterbot abort

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-105577 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

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

oli-obk commented Dec 13, 2022

@craterbot build

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

@craterbot run mode=build-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-105577 created and queued.
🤖 Automatically detected try build 8e6760e82a081fb624e777d52ef744c1def2f593
⚠️ Try build based on commit a586167a54a2505492cbd6f6d66cee28d76670da, but latest commit is e2bf4b9. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2022
@JakobDegen JakobDegen mentioned this pull request Dec 14, 2022
Merged
@craterbot
Copy link
Collaborator

🚧 Experiment pr-105577 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-105577 is completed!
📊 46 regressed and 15 fixed (250060 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 15, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#105683 - JakobDegen:dest-prop-storage, r=tm…
6cdc83b 
…iasko

Various cleanups to dest prop

This makes fixing the issues identified in rust-lang#105577 easier. A couple changes

 - Use an enum with names instead of a bool
 - Only call `remove_candidates_if` from one place instead of two. Doing it from two places is far too fragile, since any divergence in the behavior between those callsites is likely to be unsound.
 - Remove `is_constant`. Right now we only merge locals, so this doesn't do anything, and the logic would be wrong if it did.

r? `@tmiasko`
@saethlin
Copy link
Member Author

The crater run found nothing, as expected. We would need a way to ensure that we have -Zmir-opt-level=2 -Zvalidate-mir on in crater. I can push a hacky commit that makes those the defaults?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 15, 2022

I flipped crater to build mode, so that should have run the changes from your PR I think?

@saethlin
Copy link
Member Author

I'm pretty sure crater's build mode is a debug build, which is -Zmir-opt-level=1. Even with this PR, DestProp is still a -Zmir-opt-level=2-only pass. (and we would need -Zvalidate-mir)

It looks like craterbot supports setting rustflags: https://github.com/rust-lang/crater/blob/master/docs/bot-usage.md#experiment-requirements

@JakobDegen
Copy link
Contributor

If we do another run, let's wait for the changes I have pending to land first, specifically the fix for stack size and dealing with moves

@saethlin saethlin mentioned this pull request Dec 19, 2022
Merged
@bors
Copy link
Contributor

bors commented Dec 29, 2022

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

@saethlin saethlin mentioned this pull request Jan 7, 2023
Merged
@saethlin
Copy link
Member Author

saethlin commented Jan 9, 2023

Waiting on DestProp improvement
@rustbot label +S-blocked

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 9, 2023
@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Jan 21, 2023
@saethlin
Copy link
Member Author

Since #107449, it seems highly unlikely that any applying any kind of cleverness like this will let DestinationPropagation be a compile time improvement.

So I think the status quo is fine and there's nothing to do here: DestinationPropagation can stay opt-in, for things like cranelift.

@saethlin saethlin closed this Feb 19, 2023
@saethlin saethlin deleted the destprop-one-round branch February 19, 2023 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinerSebas MinerSebas MinerSebas left review comments

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations perf-regression Performance regression. S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.