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

Reduce duplication of RPO calculation of mir #96500

Merged
merged 1 commit into from
Apr 30, 2022
Merged

Reduce duplication of RPO calculation of mir #96500

merged 1 commit into from
Apr 30, 2022

Conversation

SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented Apr 28, 2022 •
edited
Loading

Computing the RPO of mir is not a low-cost thing, but it is duplicate in many places. In particular the iterate_to_fixpoint method which is called multiple times when computing the data flow.
This PR reduces the number of times the RPO is recalculated as much as possible, which should save some compile time.

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

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2022
@fee1-dead
Copy link
Member

r? rust-lang/compiler

rust-lang/highfive#399

@tmiasko
Copy link
Contributor

tmiasko commented Apr 28, 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 Apr 28, 2022
@bors
Copy link
Contributor

bors commented Apr 28, 2022

⌛ Trying commit e302207a194f1a8ceb6092d5732911ebabebcbd5 with merge fe490c7fc5b49eedb0d44da3e67d066f001aa4e7...

@bors
Copy link
Contributor

bors commented Apr 28, 2022

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

@rust-timer
Copy link
Collaborator

Queued fe490c7fc5b49eedb0d44da3e67d066f001aa4e7 with parent 71cd460, future comparison URL.

@petrochenkov
Copy link
Contributor

I don't know anything about this code, so rerolling.
r? rust-lang/compiler

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fe490c7fc5b49eedb0d44da3e67d066f001aa4e7): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 10 7 0 0 10
mean2 0.4% 0.8% N/A N/A 0.4%
max 0.7% 1.0% N/A N/A 0.7%

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.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 28, 2022
@tmiasko
Copy link
Contributor

tmiasko commented Apr 28, 2022

The MIR Body API computes some information on demand and caches the result, e.g., predecessors, switch_sources, and is_cfg_cyclic. I wonder if that would be a better implementation strategy, especially if we could avoid making the copy of this data each time it is used.

@SparrowLii
Copy link
Member Author

The MIR Body API computes some information on demand and caches the result, e.g., predecessors, switch_sources, and is_cfg_cyclic. I wonder if that would be a better implementation strategy, especially if we could avoid making the copy of this data each time it is used.

That makes sense! I thought about this before too, but didn't try given that mir would be changed frequently. But with reference to these implementation, invalidating them when getting the mutable reference of mir is a good solution.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@SparrowLii
Copy link
Member Author

There is a logical problem with the current implementation.(If the root parameter is not START_BLOCK, it will get incorrect results). But I think we can do a perf test first and then fix this.

@lqd
Copy link
Member

lqd commented Apr 29, 2022

@bors try @rust-timer queue

@SparrowLii
Copy link
Member Author

SparrowLii commented Apr 29, 2022 •
edited
Loading

@bors r=tmiasko

1 similar comment
@SparrowLii
Copy link
Member Author

@bors r=tmiasko

@bors
Copy link
Contributor

bors commented Apr 29, 2022

📌 Commit 78e969e275b82c8a5ef5fbfc2d78544dd9a6b8f3 has been approved by tmiasko

@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 Apr 29, 2022
@tmiasko
Copy link
Contributor

tmiasko commented Apr 29, 2022

Could you also squash all the commits? To avoid having commits with changes that are immediately reverted or fixed by subsequent commit.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 29, 2022
@SparrowLii
Eliminate duplication of RPO calculation for mir
7149bbc 
add `postorder_cache` to mir Body

add `ReversePostorderCache` struct

correct struct name and comments
@SparrowLii
Copy link
Member Author

sure

@tmiasko
Copy link
Contributor

tmiasko commented Apr 29, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2022

📌 Commit 7149bbc has been approved by tmiasko

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2022
@bors
Copy link
Contributor

bors commented Apr 30, 2022

⌛ Testing commit 7149bbc with merge c88089caaff7dfe5766a8326eb562e7c6aa1ba57...

@bors
Copy link
Contributor

bors commented Apr 30, 2022

💔 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 Apr 30, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@tmiasko
Copy link
Contributor

tmiasko commented Apr 30, 2022

@bors retry Could not resolve host: ci-mirrors.rust-lang.org

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

bors commented Apr 30, 2022

⌛ Testing commit 7149bbc with merge 9a98c63...

@bors
Copy link
Contributor

bors commented Apr 30, 2022

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

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

Finished benchmarking commit (9a98c63): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 43 17 43
mean2 N/A N/A -0.5% -0.7% -0.5%
max N/A N/A -0.9% -1.4% -0.9%

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

Assignees

@wesleywiser wesleywiser

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
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.