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

Avoid unnecessary monomorphization of inline asm related functions #92731

Merged
merged 3 commits into from
Jan 18, 2022
Merged

Avoid unnecessary monomorphization of inline asm related functions #92731

merged 3 commits into from
Jan 18, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 10, 2022

This should reduce build time for codegen backends by avoiding duplicated monomorphization of certain inline asm related functions for each passed in closure type.

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-inline-assembly Area: Inline assembly (`asm!(…)`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 10, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Jan 10, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the asm_support_changes branch from 7c17f98 to a9733d1 Compare January 10, 2022 16:13
@nagisa
Copy link
Member

nagisa commented Jan 15, 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 Jan 15, 2022
@bors
Copy link
Contributor

bors commented Jan 15, 2022

⌛ Trying commit a9733d1883d6253a6db716a6f8e73bb6e7b67217 with merge 2f5ac69469feb017b1eb5b3da1660ea337abf742...

nagisa
nagisa approved these changes Jan 15, 2022
View reviewed changes
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Feel free to r=me after fixing the nit or not, whichever you prefer.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the asm_support_changes branch from d3664fc to 93af944 Compare January 15, 2022 19:04
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 16, 2022

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

@bjorn3 bjorn3 force-pushed the asm_support_changes branch from 93af944 to f7786ba Compare January 16, 2022 12:07
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 16, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Jan 16, 2022

⌛ Trying commit f7786bacf8c28bbe43a59a9f3972efea7f58552e with merge 64e9d90e5a7e30f7a50668a017f94cb647970d18...

@bors
Copy link
Contributor

bors commented Jan 16, 2022

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

@rust-timer
Copy link
Collaborator

Queued 64e9d90e5a7e30f7a50668a017f94cb647970d18 with parent 42852d7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (64e9d90e5a7e30f7a50668a017f94cb647970d18): 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.

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 Jan 16, 2022
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 16, 2022

No perf changes and fixed the nit.

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jan 16, 2022

📌 Commit f7786bacf8c28bbe43a59a9f3972efea7f58552e has been approved by nagisa

@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 Jan 16, 2022
@bors
Copy link
Contributor

bors commented Jan 16, 2022

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

@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 Jan 16, 2022
bjorn3 added 3 commits January 17, 2022 18:06
@bjorn3
Use Symbol for target features in asm handling
991cbd1 
This saves a couple of Symbol::intern calls
@bjorn3
Pass target_features set instead of has_feature closure
042aa37 
This avoids unnecessary monomorphizations in codegen backends
@bjorn3
Fix review comment
9336fe3
@bjorn3 bjorn3 force-pushed the asm_support_changes branch from f7786ba to 9336fe3 Compare January 17, 2022 17:07
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 17, 2022

Rebased. The only conflict was in the list of pre-defined symbols.

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jan 17, 2022

📌 Commit 9336fe3 has been approved by nagisa

@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 Jan 17, 2022
@bors
Copy link
Contributor

bors commented Jan 18, 2022

⌛ Testing commit 9336fe3 with merge 9ad5d82...

@bors
Copy link
Contributor

bors commented Jan 18, 2022

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 9ad5d82 to master...

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

Finished benchmarking commit (9ad5d82): 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

@bjorn3 bjorn3 deleted the asm_support_changes branch January 19, 2022 14:32
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 19, 2022

This saved about 200KB of 11MB (2%) for librustc_codegen_cranelift.so. It looks like it also saved a second or two of the cg_clif build time, but that one is pretty noisy anyway.

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

@nagisa nagisa nagisa approved these changes

Assignees

@nagisa nagisa

Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@bjorn3 @rust-highfive @rust-log-analyzer @nagisa @rust-timer @bors @rustbot