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

Move ICH to rustc_query_system #89266

Merged
merged 9 commits into from
Oct 5, 2021
Merged

Move ICH to rustc_query_system #89266

merged 9 commits into from
Oct 5, 2021

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Sep 26, 2021

Based on #89183

The StableHashingContext does not need to be in rustc_middle.

This PR moves it to rustc_query_system. This will avoid a dependency between rustc_ast_lowering and rustc_middle in #89124.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Sep 26, 2021
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 27, 2021

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

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems sensible to me, but I'll reassign to get another opinion.

@davidtwco
Copy link
Member

r? @michaelwoerister

@cjgillot cjgillot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@cjgillot
Copy link
Contributor Author

Actually, putting it in rustc_query_system would be more consistent.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot force-pushed the session-ich branch 2 times, most recently from e2a2c86 to 64a8296 Compare September 30, 2021 19:09
@michaelwoerister
Copy link
Member

Looks good to me. Thanks, @cjgillot!

Let's do a perf run for good measure:
@bors try @rust-timer queue

If that doesn't yield any surprises, r=davidtwco,michaelwoerister after rebasing and updating the PR message.

@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 Oct 1, 2021
@bors
Copy link
Contributor

bors commented Oct 1, 2021

⌛ Trying commit 64a8296404f34e1d6eb70fc8403719eca6d34843 with merge 0167f490eac64cc5491f7207f726854e923cda54...

@bors
Copy link
Contributor

bors commented Oct 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued 0167f490eac64cc5491f7207f726854e923cda54 with parent 598d89b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0167f490eac64cc5491f7207f726854e923cda54): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

  • Moderate regression in instruction counts (up to 1.1% on incr-unchanged builds of externs)

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 1, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (376d7982c5989bfb7db8ef162de7e442e7168d62): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.8% on incr-unchanged builds of externs)

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 led to changes in compiler perf.

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

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Oct 3, 2021
@cjgillot cjgillot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 3, 2021
@cjgillot cjgillot marked this pull request as ready for review October 3, 2021 18:20
@michaelwoerister
Copy link
Member

Performance looks good now 👍

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2021

📌 Commit b2ed9c4 has been approved by michaelwoerister

@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 Oct 4, 2021
@bors
Copy link
Contributor

bors commented Oct 4, 2021

⌛ Testing commit b2ed9c4 with merge 6f7dab50d103a74c5130a3ebf578ade0467ee02e...

@bors
Copy link
Contributor

bors commented Oct 4, 2021

💥 Test timed out

@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 Oct 4, 2021
@cjgillot
Copy link
Contributor Author

cjgillot commented Oct 4, 2021

@bors retry

@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 Oct 4, 2021
@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)

@bors
Copy link
Contributor

bors commented Oct 5, 2021

⌛ Testing commit b2ed9c4 with merge 55111d6...

@bors
Copy link
Contributor

bors commented Oct 5, 2021

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 55111d6 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (55111d6): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@cjgillot cjgillot deleted the session-ich branch October 5, 2021 21:34
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants