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

Partially move cg_ssa towards using a single builder #94123

Merged
merged 4 commits into from
Feb 24, 2022
Merged

Partially move cg_ssa towards using a single builder #94123

merged 4 commits into from
Feb 24, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Feb 18, 2022 •
edited
Loading

Not all codegen backends can handle hopping between blocks well. For example Cranelift requires blocks to be terminated before switching to building a new block. Rust-gpu requires a RefCell to allow hopping between blocks and cg_gcc currently has a buggy implementation of hopping between blocks. This PR reduces the amount of cases where cg_ssa switches between blocks before they are finished and mostly fixes the block hopping in cg_gcc. (only scalar_to_backend doesn't handle it correctly yet in cg_gcc fixed that one.)

@antoyo please review the cg_gcc changes.

@bjorn3 bjorn3 added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-gcc Things relevant to the [future] GCC backend labels Feb 18, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Feb 18, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the cg_ssa_singleton_builder branch 2 times, most recently from 9611fba to b8ff4bd Compare February 18, 2022 17:46
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 18, 2022

@bors try @rust-timer queue

just in case. Don't expect a lot of change though.

@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 Feb 18, 2022
@bors
Copy link
Contributor

bors commented Feb 18, 2022

⌛ Trying commit b8ff4bdd5cd50881c9dd47442f50c333261ba4e7 with merge f7c9ff1f47173b2462d35a513f564b40e1e0f380...

@bjorn3
Copy link
Member Author

bjorn3 commented Feb 18, 2022 •
edited
Loading

There is a small bug in the last commit only affecting cg_gcc. I will push a fix tomorrow.
Edit: I will split out the cg_gcc changes as far as possible too.

@bors
Copy link
Contributor

bors commented Feb 18, 2022

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

@rust-timer
Copy link
Collaborator

Queued f7c9ff1f47173b2462d35a513f564b40e1e0f380 with parent b8c56fa, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f7c9ff1f47173b2462d35a513f564b40e1e0f380): comparison url.

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

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 the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2022
@bjorn3 bjorn3 force-pushed the cg_ssa_singleton_builder branch 2 times, most recently from 97301ed to 5bc81b1 Compare February 19, 2022 12:27
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 19, 2022

Split out the cg_gcc changes where possible to my cg_gcc_fix_non_singleton_builder branch.

Ready for review.

antoyo
antoyo approved these changes Feb 19, 2022
View reviewed changes
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Looks good!

@bors
Copy link
Contributor

bors commented Feb 20, 2022

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

@tmiasko
Copy link
Contributor

tmiasko commented Feb 20, 2022

@bors r=antoyo,tmiasko

@bors
Copy link
Contributor

bors commented Feb 20, 2022

📌 Commit cc0bdfb1e01841994ce7554d08858b5d978c9744 has been approved by antoyo,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 Feb 20, 2022
@lcnr
Copy link
Contributor

lcnr commented Feb 21, 2022

r? @tmiasko

@rust-highfive rust-highfive assigned tmiasko and unassigned lcnr Feb 21, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Testing commit cc0bdfb1e01841994ce7554d08858b5d978c9744 with merge 8c5e5f64093e81f763582226630dd7814ea65335...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

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

tmiasko commented Feb 24, 2022

Could you squash the fix into commit that made the change? Feel free to r+ when done.

@bjorn3
Copy link
Member Author

bjorn3 commented Feb 24, 2022

@bors r=tmiasko

@bors
Copy link
Contributor

bors commented Feb 24, 2022

📌 Commit 96cf799 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 Feb 24, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Testing commit 96cf799 with merge 3d127e2...

@bors
Copy link
Contributor

bors commented Feb 24, 2022

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing 3d127e2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 24, 2022
@bors bors merged commit 3d127e2 into rust-lang:master Feb 24, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 24, 2022
@bjorn3 bjorn3 deleted the cg_ssa_singleton_builder branch February 24, 2022 17:48
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3d127e2): comparison url.

Summary: This benchmark run shows 4 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -0.8%
  • Largest improvement in instruction counts: -0.9% on incr-unchanged builds of externs opt

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

@rustbot label: -perf-regression

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 26, 2022
@bors
Auto merge of rust-lang#94123 - bjorn3:cg_ssa_singleton_builder, r=tm…
648d038 
…iasko

Partially move cg_ssa towards using a single builder

Not all codegen backends can handle hopping between blocks well. For example Cranelift requires blocks to be terminated before switching to building a new block. Rust-gpu requires a `RefCell` to allow hopping between blocks and cg_gcc currently has a buggy implementation of hopping between blocks. This PR reduces the amount of cases where cg_ssa switches between blocks before they are finished and mostly fixes the block hopping in cg_gcc. (~~only `scalar_to_backend` doesn't handle it correctly yet in cg_gcc~~ fixed that one.)

`@antoyo` please review the cg_gcc changes.
@Mark-Simulacrum
Copy link
Member

Likely just noise -- externs is currently a little noisy (and we haven't absorbed that noisiness into our statistical estimation yet). Seems to be due to #93839, which makes me suspect this is related to PGO or inlining decisions of some kind. #94373 might help.

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

@antoyo antoyo antoyo approved these changes

Assignees

@tmiasko tmiasko

Labels
A-codegen Area: Code generation A-gcc Things relevant to the [future] GCC backend 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.61.0
Development

Successfully merging this pull request may close these issues.
10 participants
@bjorn3 @rust-highfive @rust-log-analyzer @rust-timer @bors @tmiasko @lcnr @Mark-Simulacrum @antoyo @rustbot