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

rustc_codegen_llvm: make sse4.2 imply crc32 for LLVM 14 #88981

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented Sep 15, 2021

This fixes compiling things like the snap crate after
https://reviews.llvm.org/D105462. I added a test that verifies the
additional attribute gets specified, and confirmed that I can build
cargo with both LLVM 13 and 14 with this change applied.

r? @nagisa cc @nikic

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nagisa (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2021
@rust-log-analyzer

This comment has been minimized.

@durin42
Copy link
Contributor Author

durin42 commented Sep 16, 2021

Added an assembly test that covers the -Ctarget-feature=+sse4.2 case.

@rust-log-analyzer

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented Sep 16, 2021

@bors try @rust-timer queue

r=me once the commits are squashed and the perf run does not indicate any significant negative trend.

@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 Sep 16, 2021
@bors
Copy link
Contributor

bors commented Sep 16, 2021

⌛ Trying commit 28e0274cb84cda89e3f085b55060d877eeb42c21 with merge 232f9135f71fd7a09bc04b440e3cb7a1966d361a...

@bors
Copy link
Contributor

bors commented Sep 16, 2021

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

@rust-timer
Copy link
Collaborator

Queued 232f9135f71fd7a09bc04b440e3cb7a1966d361a with parent d1d8145, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (232f9135f71fd7a09bc04b440e3cb7a1966d361a): 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 Sep 16, 2021
@durin42
Copy link
Contributor Author

durin42 commented Sep 16, 2021

Now that the perf run is done, rebased and squashed.

@nagisa
Copy link
Member

nagisa commented Sep 16, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2021

📌 Commit 1c779647259894dbc52b3e571ff8642c21347a35 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 Sep 16, 2021
@bors
Copy link
Contributor

bors commented Sep 18, 2021

⌛ Testing commit 1c779647259894dbc52b3e571ff8642c21347a35 with merge 2c1f37364fd5b287f5dfb5c04e6d29cef63df49b...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 18, 2021

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 18, 2021
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2021
@durin42
Copy link
Contributor Author

durin42 commented Sep 19, 2021 via email

@nagisa
Copy link
Member

nagisa commented Sep 19, 2021

The only-x86 approach is definitely more straightforward, so its probably going to be better for you to adjust it that way.

That said, I personally prefer the no_core option. It allows for better cross-architecture coverage when developing on machines that execute a different instruction set.

@durin42
Copy link
Contributor Author

durin42 commented Sep 20, 2021

I got a little lost trying to figure out the no_core version, so I did the only-x86_64 flavor with an amend+rebase.

This fixes compiling things like the `snap` crate after
https://reviews.llvm.org/D105462. I added a test that verifies the
additional attribute gets specified, and confirmed that I can build
cargo with both LLVM 13 and 14 with this change applied.
@nagisa
Copy link
Member

nagisa commented Sep 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2021

📌 Commit 4185b76 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 Sep 21, 2021
@bors
Copy link
Contributor

bors commented Sep 21, 2021

⌛ Testing commit 4185b76 with merge 840acd3...

@bors
Copy link
Contributor

bors commented Sep 21, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 840acd3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 21, 2021
@bors bors merged commit 840acd3 into rust-lang:master Sep 21, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 21, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (840acd3): 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

@durin42 durin42 deleted the llvm-14-crc32 branch September 27, 2021 22:13
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.

7 participants