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

feat: stabilization for stdarch_arm_crc32 #1573

Merged
merged 2 commits into from
May 14, 2024

Conversation

ssukanmi
Copy link

@ssukanmi ssukanmi commented May 6, 2024

Stabilization for CRC32 intrinsics on ARM, for which the FCP has been completed
rust-lang/rust#117215

@rustbot
Copy link
Collaborator

rustbot commented May 6, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

@@ -58,7 +58,7 @@ use stdarch_test::assert_instr;
#[target_feature(enable = "crc")]
#[cfg_attr(target_arch = "arm", target_feature(enable = "v8"))]
#[cfg_attr(test, assert_instr(crc32b))]
#[unstable(feature = "stdarch_arm_crc32", issue = "117215")]
#[stable(feature = "stdarch_arm_crc32", since = "1.80.0")]
Copy link
Member

Choose a reason for hiding this comment

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

Since target features for 32-bit ARM are not stabilized yet, we only want to stabilize these on AArch64 for now. You can do this by using cfg_attr like this:

#[cfg_attr(not(target_arch = "arm"), stable(feature = "stdarch_arm_crc32", since = "1.80.0"))]

Copy link
Author

Choose a reason for hiding this comment

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

Ok just to confirm, I should revert all the #[unstable()] back to #[stable()] and add a new line for the
#[cfg_attr(not(target_arch = "arm"), stable(feature = "stdarch_arm_crc32", since = "1.80.0"))].
Also do I need to delete any of the #![cfg_attr()] in the crates/intrinsic-test/src/main.rs file

Copy link
Member

Choose a reason for hiding this comment

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

Right, you actually do need to include both, like this:

#[cfg_attr(target_arch = "arm", unstable(feature = "stdarch_aarch32_crc32", issue = "XXXX"))]
#[cfg_attr(not(target_arch = "arm"), stable(feature = "stdarch_aarch64_crc32", since = "1.80.0"))]

You will need to create a new tracking issue for the stabilization of the intrinsics on 32-bit ARM, blocked on the target features being stabilized.

@ssukanmi ssukanmi force-pushed the stdarch_arm_crc32 branch 2 times, most recently from 804455d to 1e0ac7b Compare May 13, 2024 18:44
@ssukanmi
Copy link
Author

Added new PR with suggested change and created a new issue for intrinsics on 32 bit ARM rust-lang/rust#125085

@Amanieu Amanieu merged commit df3618d into rust-lang:master May 14, 2024
29 checks passed
@Amanieu
Copy link
Member

Amanieu commented May 14, 2024

Thanks! Now you just need to send a PR to rust-lang/rust to update the library/stdarch submodule.

AlexTMjugador added a commit to ComunidadAylas/rust-crc32fast that referenced this pull request May 19, 2024
rust-lang/stdarch#1573 stabilized the
`stdarch_arm_crc32` feature, meaning that it's no longer necessary to
enable it to get ARM CRC intrinsics, and doing so in the latest
nightlies causes compile errors to happen.

Let's get rid of that removed unstable feature and update the
documentation accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants