port: Use __cpuid only, when available.#1887
Conversation
WalkthroughThe changes introduce improved detection and handling of x86 CPU feature intrinsics in the build system and runtime code. Specifically, the build configuration (both CMake and configure script) now checks for the availability of GNU-style ( Changes
Sequence Diagram(s)sequenceDiagram
participant BuildSystem as Build System (CMake/configure)
participant Compiler as Compiler
participant Source as x86_features.c
BuildSystem->>Compiler: Test for __cpuid_count (GNU)
alt Supported
BuildSystem->>BuildSystem: Define HAVE_CPUID_GNU
else Not Supported
BuildSystem->>Compiler: Test for __cpuidex (MSVC)
alt Supported
BuildSystem->>BuildSystem: Define HAVE_CPUID_MS
else Not Supported
BuildSystem->>BuildSystem: No CPUID macro defined
end
end
Source->>Source: Use HAVE_CPUID_GNU or HAVE_CPUID_MS for feature detection
Source->>Source: Fallback to zeroed registers if neither defined
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (70)
🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1887 +/- ##
============================================
+ Coverage 40.29% 81.68% +41.39%
============================================
Files 73 145 +72
Lines 7684 13067 +5383
Branches 1281 2914 +1633
============================================
+ Hits 3096 10674 +7578
+ Misses 4363 1444 -2919
- Partials 225 949 +724 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CMakeLists.txt (1)
493-500: Robust Check for GNU-style cpuid SupportThe new hunk clearly checks for the availability of the GNU-specific
__cpuid_countincpuid.h. The comment explains that even though gcc/clang and MSVC have a built-in__cpuid, this check ensures that the proper GNU extension is available before definingHAVE_CPUID_GNU. Also, resettingCMAKE_REQUIRED_DEFINITIONSafter the check is a good practice to avoid side effects in subsequent tests.Consider adding a brief inline note on when this macro will be used in the source code (i.e., in place of the traditional MSVC check) for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)arch/x86/x86_features.c(2 hunks)configure(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (40)
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: Windows GCC Native Instructions (AVX)
- GitHub Check: EL9 GCC S390X DFLTCC ASAN
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt
- GitHub Check: macOS GCC Symbol Prefix & Compat
🔇 Additional comments (9)
CMakeLists.txt (1)
502-510: Robust Check for MSVC-style cpuid SupportThis block mirrors the GNU check for the MSVC-specific
__cpuidexusingintrin.h. It correctly definesHAVE_CPUID_MSif the symbol is available. The comments and the structure make it easy to understand that the fallback mechanism now distinguishes between GNU and MSVC extensions.arch/x86/x86_features.c (5)
13-17: Improved preprocessor conditionals for better portability.This change replaces direct compiler checks with feature-based macros, improving portability across different compilers.
31-42: Refactored implementation selection for better maintainability.This change uses feature-based macros instead of compiler-specific checks, making the code more maintainable and easier to extend for additional compilers.
43-44: Enhanced fallback mechanism with clear performance implications.The added comment provides important context about performance implications when falling back to the non-optimized path.
61-62: Consistent error handling in cpuidex function.Good addition of the comment explaining performance implications, consistent with other similar changes in the file.
74-75: Improved xgetbv fallback with explicit return value.The fallback now returns 0 instead of no operation, and includes a comment about performance implications, making the code more robust and clear.
configure (3)
810-837: Added GNU-style __cpuid detection.This new block adds runtime detection of GNU compiler's __cpuid support, enhancing compatibility with various compilers by conditionally defining HAVE_CPUID_GNU when available.
838-865: Added Microsoft-style __cpuid detection.This block implements detection for Microsoft's __cpuid intrinsic, which complements the GNU-style detection and properly defines HAVE_CPUID_MS when available.
867-870: Well-structured separation markers.These separators maintain the script's readability with clear section boundaries between the new cpuid detection blocks and the rest of the configure checks.
|
Detection doesn't seem to work most of the time with Visual C++ and breaks build... |
Upps, sorry. I have no MSVC installed had hoped, everything was ok. I will take a look next weekend. |
One can only use CMake with Visual C++. It fails while configuring the build in CI... The error while configuring is: |
|
Thanks, that helps. (I don't have Windows with Visual Studio running yet...) I found a list of MSVC x86 intinsics, I have to find a different way. |
Best solution I think is to just try to compile small snippet of code using the intrinsic like we do in other places. When I want to make sure nothing gets optimized away when compiler does dead code elimination, I just coarse the result of intrinsic call to fit in the return value of "main" function. Because the snippet isn't (doesn't need to) actually run, not returning |
|
That's what i prepared (similar code, wht is used in configure) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)arch/x86/x86_features.c(2 hunks)configure(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- arch/x86/x86_features.c
- CMakeLists.txt
🔇 Additional comments (2)
configure (2)
835-862:Details
❓ Verification inconclusive
Check for MS-style __cpuid function support added
This block adds detection of Microsoft-style
__cpuidsupport by testing compilation of code that uses the function from<intrin.h>, settingHAVE_CPUID_MSwhen available.Based on the PR comments, there might be issues with MSVC and intrinsic functions. This detection mechanism might fail because intrinsic functions don't have addresses. Run this script to verify behavior with different compilers:
🏁 Script executed:
#!/bin/bash # Create test file for MS-style __cpuid cat > test_ms_cpuid.c << 'EOF' #include <intrin.h> #include <stdio.h> static inline void cpuid_ms(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { unsigned int registers[4]; __cpuid((int *)registers, info); *eax = registers[0]; *ebx = registers[1]; *ecx = registers[2]; *edx = registers[3]; } int main(void) { unsigned int eax, ebx, ecx, edx; cpuid_ms(0, &eax, &ebx, &ecx, &edx); printf("%d\n", eax); return 0; } EOF # Check which compilers are available and test compilation for compiler in gcc clang cl cc; do if command -v $compiler > /dev/null 2>&1; then echo "Testing MS-style __cpuid with $compiler:" if [ "$compiler" = "cl" ]; then $compiler test_ms_cpuid.c -c 2>&1 | grep -i cpuid else $compiler -c test_ms_cpuid.c 2>&1 | grep -i cpuid fi echo "Exit code: $?" fi doneLength of output: 209
Action Required: Manually Verify MS-style
__cpuidDetectionThe added code block correctly tests for the availability of Microsoft-style
__cpuidby attempting to compile a snippet that uses the intrinsic from<intrin.h>. However, the automated test script produced no output or errors, leaving uncertainty as to whether the detection might fail (particularly with MSVC, where intrinsic functions might not behave as expected with regards to addressability).
- Verify manually with MSVC and other supported compilers to ensure that the intrinsic behaves as assumed and that
HAVE_CPUID_MSis correctly set.- Check that the detection logic accounts for potential issues with intrinsic function addresses.
810-833:Details
✅ Verification successful
Check for GNU-style __cpuid function support added
This block adds detection of GNU-style
__cpuidsupport by testing compilation of code that uses the function, settingHAVE_CPUID_GNUwhen available.Verify that this detection works consistently across different compilers:
🏁 Script executed:
#!/bin/bash # Check how GNU-style __cpuid is implemented in cpuid.h across different compilers if [ -f /usr/include/cpuid.h ]; then echo "Found cpuid.h in system includes. Examining its contents:" grep -A 10 "__cpuid" /usr/include/cpuid.h else echo "cpuid.h not found in standard location" fi # Check which compilers are available and how they handle __cpuid for compiler in gcc clang cc; do if command -v $compiler > /dev/null 2>&1; then echo "Testing $compiler:" $compiler -dM -E -x c /dev/null | grep -i cpuid fi doneLength of output: 265
GNU-style __cpuid Detection Block Verified
The detection code inconfigure(lines 810–833) successfully compiles the test snippet and correctly sets theHAVE_CPUID_GNUflag when GNU-style__cpuidsupport is available. Note that on the test system the script did not find acpuid.hin/usr/include, which can be expected in environments where the header is provided via alternate include paths. Please ensure that your target environments have the appropriate include paths so that the compile test inconfigureworks reliably across different compilers.
| # check for cpuid support: GNU extensions | ||
| cat > $test.c <<EOF | ||
| #include <cpuid.h> | ||
| #include <stdio.h> | ||
| static inline void cpuid_gnu(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { | ||
| *eax = *ebx = *ecx = *edx = 0; | ||
| __cpuid(info, *eax, *ebx, *ecx, *edx); | ||
| } | ||
|
|
||
| int main(void) { | ||
| unsigned int eax, ebx, ecx, edx; | ||
| cpuid_gnu(0, &eax, &ebx, &ecx, &edx); | ||
| printf("%d\n", eax); | ||
| return eax; | ||
| } | ||
| EOF | ||
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | ||
| echo "Checking for GNU style __cpuid... Yes." | tee -a configure.log | ||
| CFLAGS="${CFLAGS} -DHAVE_CPUID_GNU" | ||
| SFLAGS="${SFLAGS} -DHAVE_CPUID_GNU" | ||
| else | ||
| echo "Checking for GNU style __cpuid... No." | tee -a configure.log | ||
| fi | ||
| echo >> configure.log | ||
|
|
||
| # check for cpuid support: MSVC extensions | ||
| cat > $test.c <<EOF | ||
| #include <intrin.h> | ||
| #include <stdio.h> | ||
| static inline void cpuid_ms(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { | ||
| unsigned int registers[4]; | ||
| __cpuid((int *)registers, info); | ||
| *eax = registers[0]; | ||
| *ebx = registers[1]; | ||
| *ecx = registers[2]; | ||
| *edx = registers[3]; | ||
| } | ||
|
|
||
| int main(void) { | ||
| unsigned int eax, ebx, ecx, edx; | ||
| cpuid_ms(0, &eax, &ebx, &ecx, &edx); | ||
| printf("%d\n", eax); | ||
| return 0; | ||
| } | ||
| EOF | ||
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | ||
| echo "Checking for MS style __cpuid... Yes." | tee -a configure.log | ||
| CFLAGS="${CFLAGS} -DHAVE_CPUID_MS" | ||
| SFLAGS="${SFLAGS} -DHAVE_CPUID_MS" | ||
| else | ||
| echo "Checking for MS style __cpuid... No." | tee -a configure.log | ||
| fi | ||
| echo >> configure.log | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative detection approach for MSVC
The detection methods for both GNU and MSVC-style __cpuid functions rely on successful compilation of test programs. However, according to the PR comments, this may not work reliably with Visual C++ due to intrinsic functions not having addresses.
Consider implementing an alternative detection approach for MSVC similar to mtl1979's suggestion in the PR comments - try compiling a small code snippet that uses the intrinsic and coerces the result into the return value of the main function to prevent the compiler from optimizing it away. This would be more reliable for detecting intrinsic function availability.
-cat > $test.c <<EOF
-#include <intrin.h>
-#include <stdio.h>
-static inline void cpuid_ms(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) {
- unsigned int registers[4];
- __cpuid((int *)registers, info);
- *eax = registers[0];
- *ebx = registers[1];
- *ecx = registers[2];
- *edx = registers[3];
-}
-
-int main(void) {
- unsigned int eax, ebx, ecx, edx;
- cpuid_ms(0, &eax, &ebx, &ecx, &edx);
- printf("%d\n", eax);
- return 0;
-}
+cat > $test.c <<EOF
+#include <intrin.h>
+int main(void) {
+ int cpu_info[4] = {0};
+ __cpuid(cpu_info, 0);
+ return cpu_info[0] & 0x1; /* Just to prevent optimization, actual value doesn't matter */
+}
EOF📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # check for cpuid support: GNU extensions | |
| cat > $test.c <<EOF | |
| #include <cpuid.h> | |
| #include <stdio.h> | |
| static inline void cpuid_gnu(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { | |
| *eax = *ebx = *ecx = *edx = 0; | |
| __cpuid(info, *eax, *ebx, *ecx, *edx); | |
| } | |
| int main(void) { | |
| unsigned int eax, ebx, ecx, edx; | |
| cpuid_gnu(0, &eax, &ebx, &ecx, &edx); | |
| printf("%d\n", eax); | |
| return eax; | |
| } | |
| EOF | |
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | |
| echo "Checking for GNU style __cpuid... Yes." | tee -a configure.log | |
| CFLAGS="${CFLAGS} -DHAVE_CPUID_GNU" | |
| SFLAGS="${SFLAGS} -DHAVE_CPUID_GNU" | |
| else | |
| echo "Checking for GNU style __cpuid... No." | tee -a configure.log | |
| fi | |
| echo >> configure.log | |
| # check for cpuid support: MSVC extensions | |
| cat > $test.c <<EOF | |
| #include <intrin.h> | |
| #include <stdio.h> | |
| static inline void cpuid_ms(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { | |
| unsigned int registers[4]; | |
| __cpuid((int *)registers, info); | |
| *eax = registers[0]; | |
| *ebx = registers[1]; | |
| *ecx = registers[2]; | |
| *edx = registers[3]; | |
| } | |
| int main(void) { | |
| unsigned int eax, ebx, ecx, edx; | |
| cpuid_ms(0, &eax, &ebx, &ecx, &edx); | |
| printf("%d\n", eax); | |
| return 0; | |
| } | |
| EOF | |
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | |
| echo "Checking for MS style __cpuid... Yes." | tee -a configure.log | |
| CFLAGS="${CFLAGS} -DHAVE_CPUID_MS" | |
| SFLAGS="${SFLAGS} -DHAVE_CPUID_MS" | |
| else | |
| echo "Checking for MS style __cpuid... No." | tee -a configure.log | |
| fi | |
| echo >> configure.log | |
| # check for cpuid support: GNU extensions | |
| cat > $test.c <<EOF | |
| #include <cpuid.h> | |
| #include <stdio.h> | |
| static inline void cpuid_gnu(int info, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx) { | |
| *eax = *ebx = *ecx = *edx = 0; | |
| __cpuid(info, *eax, *ebx, *ecx, *edx); | |
| } | |
| int main(void) { | |
| unsigned int eax, ebx, ecx, edx; | |
| cpuid_gnu(0, &eax, &ebx, &ecx, &edx); | |
| printf("%d\n", eax); | |
| return eax; | |
| } | |
| EOF | |
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | |
| echo "Checking for GNU style __cpuid... Yes." | tee -a configure.log | |
| CFLAGS="${CFLAGS} -DHAVE_CPUID_GNU" | |
| SFLAGS="${SFLAGS} -DHAVE_CPUID_GNU" | |
| else | |
| echo "Checking for GNU style __cpuid... No." | tee -a configure.log | |
| fi | |
| echo >> configure.log | |
| # check for cpuid support: MSVC extensions | |
| cat > $test.c <<EOF | |
| #include <intrin.h> | |
| int main(void) { | |
| int cpu_info[4] = {0}; | |
| __cpuid(cpu_info, 0); | |
| return cpu_info[0] & 0x1; /* Just to prevent optimization, actual value doesn't matter */ | |
| } | |
| EOF | |
| if try $CC $CFLAGS -o $test $test.c $LDSHAREDLIBC; then | |
| echo "Checking for MS style __cpuid... Yes." | tee -a configure.log | |
| CFLAGS="${CFLAGS} -DHAVE_CPUID_MS" | |
| SFLAGS="${SFLAGS} -DHAVE_CPUID_MS" | |
| else | |
| echo "Checking for MS style __cpuid... No." | tee -a configure.log | |
| fi | |
| echo >> configure.log |
There was a problem hiding this comment.
The comment from coderabbitai
uses shorter code to test for "__cpuid" availability on MSVC,
but my test code is similar to the code, what "zlib-ng" is using.
There was a problem hiding this comment.
@winspool I understand your reasoning. Using test code that closely resembles how zlib-ng actually uses the __cpuid function in the codebase is a valid approach. This helps ensure that detection works correctly for the specific usage patterns in the project. Your implementation will likely be more robust for your specific use case since it tests the function in a way that matches its actual usage.
|
Missing |
Thanks for the hint. |
|
All the relevant CI runs are now passing, so last remaining check is verifying the tests give correct results... |
|
What else can i do to get this change accepted? |
|
@winspool Sorry, this needs a rebase now. |
Add a fallback, when __cpuid is not available
np. Rebased to current head. |
|
|
||
| # | ||
| # Check for x86 __cpuid / __cpuidex support: MSVC extensions | ||
| # gcc/clang and MSVC have __cpuid, so check __cpuidex instead |
There was a problem hiding this comment.
so check __cpuidex
but the code bellow checks for __cpuid? Kind of confusing
Add a fallback, when __cpuid is not available.
This allows other compiler to build zlib-ng.
tcc as example
(compiling zlib-ng with OpenWatcom needs a C99 feature, which has issues in OW)
Regards ... Detlef
Summary by CodeRabbit