[configure] Add support for RISC-V ZBC extension#1917
Conversation
|
""" WalkthroughThe build system was updated to add support for the RISC-V ZBC (Bitmanip) instruction set extension. This includes new Makefile rules and object files for ZBC-specific code, new compiler flag variables, and enhancements to the configure script to detect ZBC support, control its inclusion, and propagate relevant flags and files into the build process. Additionally, runtime RVV feature detection code is now conditionally compiled only when RVV support is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigureScript
participant Compiler
participant Makefile
User->>ConfigureScript: Run ./configure [--without-zbc]
ConfigureScript->>Compiler: Test for ZBC support (-march=rv64gc_zbc, clmul)
Compiler-->>ConfigureScript: Compilation success/failure
alt ZBC supported and enabled
ConfigureScript->>Makefile: Set ZBC flags, add crc32_zbc objects
else ZBC not supported or disabled
ConfigureScript->>Makefile: Do not set ZBC flags or objects
end
User->>Makefile: Run make
Makefile->>Compiler: Build with/without ZBC objects and flags
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
arch/riscv/Makefile.in (1)
18-20: Flag explosion – consider consolidating RVV/ZBC flags
RVVZBCFLAGandZBCFLAGare useful, but we now have three almost-identical “‐march” variables (RVVFLAG,RVVZBCFLAG,ZBCFLAG).
Keeping separate variables multiplies maintenance work and increases the risk that one of them gets out of sync withconfigure.If the only current user of
RVVZBCFLAGisriscv_features.c, you could save complexity by:
- Dropping
RVVZBCFLAG.- Building
riscv_features.ctwice (once withRVVFLAG, once withZBCFLAG) and selecting at link-time, or- Using a single flag variable that
configuremutates in place (RVVFLAG+="…_zbc").Not mandatory, but worth weighing.
configure (3)
1750-1756: Quote variables in numeric testsBare variables can expand to the empty string and break
test.
Use the POSIX-safe pattern:-if test $buildrvv -eq 1 -a $HAVE_RVV_INTRIN -eq 1; then +if test "$buildrvv" -eq 1 && test "$HAVE_RVV_INTRIN" -eq 1; thenThe same quoting fix is needed for every occurrence of
$HAVE_ZBC_EXT,$HAVE_RVV_INTRIN, etc.
952-966: Cross-platform header probe could poison CFLAGSIncluding
<linux/auxvec.h>unconditionally adds-DHAVE_LINUX_AUXVEC_Hto every build, even non-Linux cross-targets (e.g.riscv64-elf).
That header is Linux-only and will not be found by bare-metal or BSD tool-chains, leading to later compile errors.Consider:
- Guarding the test with
if test "$LINUX" -eq 1; then ….- Resetting the macro in non-Linux paths.
2434-2437: Sed replacement will silently drop empty flagsThe two new substitutions:
/^RVVZBCFLAG *=/s#=.*#=$rvvzbcflag# /^ZBCFLAG *=/s#=.*#=$zbcflag#emit an empty right-hand side when the flag variable is unset, resulting in lines like
RVVZBCFLAG =(note the trailing space).
Some Make versions treat “variable with just spaces” as defined, alteringifneq ($(RVVZBCFLAG),)logic.Safer:
sed … -e "/^RVVZBCFLAG *=/s#=.*#=${rvvzbcflag:-}#" …or remove the line entirely when the feature is disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
arch/riscv/Makefile.in(2 hunks)configure(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (123)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win64 Native Instructions (AVX)
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win64 Native Instructions (AVX)
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS Clang (Target 10.10)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows MSVC ARM No Test
- GitHub Check: Windows MSVC 2022 v143 Win64 Native Instructions (AVX)
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No ARMv8 UBSAN
- GitHub Check: Ubuntu GCC Native Instructions
- GitHub Check: Ubuntu GCC MIPS
- GitHub Check: Ubuntu GCC
- GitHub Check: Ubuntu GCC ARM HF
- GitHub Check: Ubuntu GCC -m32
🔇 Additional comments (2)
arch/riscv/Makefile.in (2)
26-27: Indentation may break Makefile parsingThe two recipe lines that were re-indented (
slide_hash_rvv.*) show leading spaces instead of a TAB in the diff.
Make(1) requires a literal TAB before every recipe line; spaces result in “*** missing separator” build failures.Please confirm those characters are still TABs in the committed file.
59-63: Missing dependency glue for the new ZBC objects
crc32_zbc.o/.loare listed here, but nothing in this Makefile declares how they are used (e.g. they aren’t added to anylibzarchive or shared-object rule inside this file).Double-check that the top-level or arch-specific Makefiles append them to
ARCH_STATIC_OBJS/ARCH_SHARED_OBJS– otherwise the objects will be built but never linked.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1917 +/- ##
===========================================
- Coverage 81.20% 80.40% -0.81%
===========================================
Files 147 137 -10
Lines 13304 12991 -313
Branches 2948 2889 -59
===========================================
- Hits 10804 10445 -359
- Misses 1559 1654 +95
+ Partials 941 892 -49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
configure (2)
124-125:⚠️ Potential issueVariables need default initialization
The variables
rvvzbcflagandzbcflagare initialized as empty strings, butHAVE_RVV_INTRINandHAVE_ZBC_EXTare used in conditionals without initialization.Add safe defaults right after these variables:
rvvzbcflag= zbcflag= +HAVE_RVV_INTRIN=0 +HAVE_ZBC_EXT=0This prevents
testcommands from failing with "unary operator expected" errors when these variables are unset.
1743-1776: 🛠️ Refactor suggestionTest for missing quotes in conditional test
In the conditional test at line 1750, variables should be quoted to avoid syntax errors when empty.
- if test $buildrvv -eq 1 -a "$HAVE_RVV_INTRIN" -eq 1; then + if test "$buildrvv" -eq 1 && test "$HAVE_RVV_INTRIN" -eq 1; thenAlso, prefer
&&over-afor better portability and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
arch/riscv/Makefile.in(2 hunks)configure(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- arch/riscv/Makefile.in
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows GCC
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
🔇 Additional comments (6)
configure (6)
103-103: Looks good: Added ZBC build flagThis correctly initializes the
buildzbcflag to 1 (enabled by default), following the pattern of other similar feature flags.
180-180: Command-line flag implementation looks goodThe
--without-zbcflag follows the same pattern as other feature flags in the configure script.Also applies to: 218-218
952-966: Well-implemented check for linux/auxvec.hThis check follows the established pattern for header checks in the configure script.
1761-1768: Good test for ZBC extensionThe test for the ZBC extension using the
clmulassembly instruction is well implemented.
2169-2179: RISC-V ZBC support properly integratedThis section correctly adds ZBC support to the RISC-V architecture section, following the pattern of other extensions.
2435-2436: Makefile variable replacement correctly implementedThe ZBC flags are correctly propagated to the architecture-specific Makefile.
f927880 to
0acad89
Compare
This should allow pkgcheck CI to pass for RISC-V
-marchif enabledSummary by CodeRabbit
New Features
--without-zbc).Chores