Fix native detection of ARM CRC instruction #1818
Conversation
WalkthroughThe changes in this pull request involve modifications to the conditional compilation directives in the Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)arch/arm/arm_functions.h (1)
The modification to use an OR condition Let's verify there are no conflicting CRC-related conditions elsewhere: ✅ Verification successfulThe CRC32 condition change is safe and consistent with other architecture-specific implementations The verification shows that each architecture (ARM, x86, s390, POWER) has its own unique set of conditions for enabling native CRC32 implementations, tailored to their specific features:
The change is consistent with the pattern where each architecture defines its own appropriate detection logic, and there are no conflicting conditions that would be affected by this modification. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for other CRC-related preprocessor conditions that might need similar updates
# Search for CRC-related preprocessor conditions
rg -n "defined.*CRC32|defined.*ACLE" --type c --type cpp --type h
# Search for native_crc32 usage
rg -n "native_crc32"
Length of output: 3003 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
It's unclear if raspberry pi OS's shipped GCC doesn't properly detect ACLE or not (/proc/cpuinfo claims to support AES), but in any case, the preprocessor macro for that flag is not defined with -march=native on a raspberry pi 5. Unfortunately that means when built "WITH_NATIVE", we do not get a fast CRC function. The CRC32 preprocessor macro _IS_ defined, and the auto detection when built without NATIVE support does properly get dispatched to. Since we only need the scalar CRC32 and not the polynomial stuff anyhow, let's make it be an || condition and not a && one.
37163e5 to
7339907
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1818 +/- ##
===========================================
+ Coverage 32.18% 32.21% +0.03%
===========================================
Files 67 67
Lines 5752 5752
Branches 1237 1237
===========================================
+ Hits 1851 1853 +2
Misses 3644 3644
+ Partials 257 255 -2 ☔ View full report in Codecov by Sentry. |
|
As far as I know |
It's unclear if raspberry pi OS's shipped GCC doesn't properly detect ACLE or not (/proc/cpuinfo claims to support AES), but in any case, the preprocessor macro for that flag is not defined with -march=native on a raspberry pi 5. Unfortunately that means when built "WITH_NATIVE", we do not get a fast CRC function. The CRC32 preprocessor macro IS defined, and the auto detection when built without NATIVE support does properly get dispatched to. Since we only need the scalar CRC32 and not the polynomial stuff anyhow, let's make it be an || condition and not a && one.
Summary by CodeRabbit
New Features
native_crc32feature, allowing for broader compatibility with ARM architectures.Bug Fixes