Conversation
WalkthroughThis pull request introduces multiple updates across build configuration and CRC computation. It adds a new GitHub Actions job for Ubuntu GCC builds with Chorba disabled, enhances CMake configuration options and Makefile object listings, and updates various CRC and Adler-32 implementations. Macro names and constant definitions have been changed for clarity and consistency. Additionally, conditional compilation is employed for tests and benchmarks (based on the Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crc32_braid_p.h (1)
6-7: Remove or clarify commented-out prototype
Ifcrc32_braid_internalis no longer used, consider removing or explaining it to keep the header clean and avoid confusion.arch/generic/crc32_c.c (1)
9-38: Consider adding a comment explaining the Chorba optimization thresholds.The code correctly implements different Chorba optimization paths based on buffer size and alignment, but it would be helpful to document the threshold values and their significance.
arch/generic/generic_functions.h (1)
37-40: Consider grouping related functions with descriptive comments.While the organization is good, adding descriptive comments for each group of related functions would improve readability.
arch/generic/crc32_chorba_c.c (1)
27-1421: LGTM! Robust implementation of the Chorba algorithm.The implementation provides multiple variants of the Chorba algorithm with proper:
- Documentation of minimum input size requirements
- Handling of endianness
- Memory alignment
- Error handling
Consider adding performance benchmarks to compare different variants and help users choose the most appropriate one for their use case.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 638-638: Uninitialized variable
(uninitvar)
crc32_braid_tbl.h (1)
62-9421: Consider adding documentation for BRAID_W and BRAID_N values.While the macro names are now more descriptive, it would be helpful to add documentation explaining:
- The purpose of these macros
- Their valid values (4 or 8 for BRAID_W, 1-6 for BRAID_N)
- Their impact on the CRC computation
CMakeLists.txt (1)
1096-1104: Review Header File Organization:arch_functions.hThe PR objectives indicate that
arch_functions.hshould be relocated from the private headers to the public headers. However, it still appears inZLIB_PRIVATE_HDRS(line 1101). Please confirm if this is intentional or if it should be moved toZLIB_PUBLIC_HDRSto correctly expose the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/cmake.yml(1 hunks)CMakeLists.txt(7 hunks)Makefile.in(2 hunks)adler32_p.h(2 hunks)arch/generic/adler32_c.c(1 hunks)arch/generic/crc32_braid_c.c(4 hunks)arch/generic/crc32_c.c(2 hunks)arch/generic/crc32_chorba_c.c(1 hunks)arch/generic/generic_functions.h(1 hunks)arch/s390/crc32-vx.c(2 hunks)arch/x86/crc32_pclmulqdq_tpl.h(3 hunks)crc32.h(1 hunks)crc32_braid_p.h(1 hunks)crc32_braid_tbl.h(15 hunks)crc32_c.h(0 hunks)functable.c(1 hunks)test/benchmarks/benchmark_crc32.cc(1 hunks)test/test_crc32.cc(1 hunks)tools/makecrct.c(4 hunks)
💤 Files with no reviewable changes (1)
- crc32_c.h
✅ Files skipped from review due to trivial changes (1)
- adler32_p.h
🧰 Additional context used
🧠 Learnings (3)
arch/generic/crc32_chorba_c.c (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
arch/generic/crc32_c.c (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
CMakeLists.txt (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
🪛 Cppcheck (2.10-2)
test/benchmarks/benchmark_crc32.cc
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (34)
arch/generic/adler32_c.c (1)
41-41: LGTM! Improved macro naming clarity.The renaming of
DO16toADLER_DO16andDO8toADLER_DO8enhances code clarity by making the macros' purpose explicit. This change aligns well with the PR's cleanup objectives and reduces potential naming conflicts.Also applies to: 44-44
tools/makecrct.c (7)
18-20: Consistent definitions for CRC generation
These definitions correctly establish the 64-bit context for generating CRC tables. No immediate concerns.
159-160: Conditional compilation forBRAID_W == 8
These lines properly introduce the 64-bit branch of table definitions. Looks good.
167-167: Handling the 32-bit branch and closing directives
These lines clearly separate the 32-bit alternative and close the conditional blocks. No issues found.Also applies to: 172-173
177-177: Iterating overBRAID_N
Printing the preprocessor line for each possibleBRAID_Nvalue is aligned with the existing loop logic.
183-183: Nested conditional forBRAID_W == 8
This nested check is consistent with the pattern used above.
204-204: Else branch forBRAID_W == 4
No concerns. Maintains symmetry with the 64-bit path.
219-220: Final closing ofBRAID_WandBRAID_N
These directives appropriately conclude the nested conditional sections.crc32.h (1)
9-16: Introduction of Chorba thresholds
Defines buffer size and thresholds for different Chorba algorithm variants. This addition is clear and consistent with the CRC32 folding interface.crc32_braid_p.h (6)
9-10: Fixed definition ofBRAID_N
SettingBRAID_Nto 5 by default is coherent with the usage shown in the table-generation code.
12-13: Documentation forBRAID_W
These lines provide a clear statement that sets expectations for how the braided CRC calculation is compiled.
15-17: DefiningBRAID_Was 8 on 64-bit architectures
Choosinguint64_tforz_word_tis appropriate whenBRAID_Wis 8.
19-20: Fallback to 32-bit word size
Definesz_word_tasuint32_tfor non-64-bit architectures. No issues here.
27-27: Byte order checks for big-endian
Ensures the correct swapping functions and table references are used for big-endian systems.Also applies to: 29-29
37-38: Renamed CRC macros
Renaming DO1/DO8 to CRC_DO1/CRC_DO8 provides clarity. The logic is unchanged and remains correct.arch/generic/crc32_c.c (1)
6-7: LGTM!The function signature is correctly marked as internal, and the CRC initialization is properly handled with pre-conditioning.
test/benchmarks/benchmark_crc32.cc (1)
59-64: LGTM!The conditional compilation correctly handles the benchmark registration based on the
WITHOUT_CHORBAflag, maintaining consistency with the implementation.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
arch/generic/generic_functions.h (1)
30-35: LGTM!The Chorba function declarations are properly guarded with
#ifndef WITHOUT_CHORBAand follow a consistent naming pattern.arch/generic/crc32_braid_c.c (2)
14-35: LGTM!The documentation clearly explains the braided CRC implementation and the rationale behind BRAID_N and BRAID_W parameters. The processor-specific optimizations are well-documented.
215-222: LGTM!The function correctly handles pre and post-conditioning of the CRC value and delegates the core computation to crc32_braid_internal.
arch/s390/crc32-vx.c (1)
204-205: LGTM! Function calls updated correctly.The changes to use direct
crc32_cfunction calls instead ofPREFIX(crc32_c)are consistent with marking the function as internal.Also applies to: 210-210, 219-219
functable.c (1)
57-57: LGTM! Function pointer assignment updated correctly.The change to use direct
crc32_cfunction pointer instead ofPREFIX(crc32_c)is consistent with marking the function as internal.Makefile.in (1)
107-107: LGTM! Object files added correctly.The addition of
crc32_chorba_c.oandcrc32_chorba_c.loto the object lists is consistent with the new Chorba configuration.Also applies to: 149-149
test/test_crc32.cc (1)
227-233: LGTM! Conditional compilation added correctly.The conditional compilation based on
WITHOUT_CHORBAflag is correctly implemented to test either the generic or Chorba implementation.arch/x86/crc32_pclmulqdq_tpl.h (2)
171-213: LGTM! Well-structured implementation of fold_12.The implementation follows the established pattern of other fold functions while introducing a new folding constant specific to this implementation. The conditional compilation ensures proper integration with the Chorba algorithm.
20-33: LGTM! Well-organized header includes.The header includes are properly organized with appropriate conditional compilation for vpclmulqdq support. All necessary dependencies for CRC32 computation are included.
crc32_braid_tbl.h (2)
62-63: LGTM! Improved macro naming clarity.The renaming from
WtoBRAID_Wmakes the purpose of the macro more explicit and self-documenting.
153-154: LGTM! Consistent macro renaming throughout the file.The changes systematically rename macros from:
WtoBRAID_WNtoBRAID_NThis improves code readability by making the purpose of these macros more explicit. The updated
#endifcomments also help with tracking preprocessor directives.Also applies to: 209-213, 323-324, 745-748, 858-859, 1280-1281, 1393-1394, 1745-1748, 2858-2859, 3280-3283, 4393-4394, 4815-4818, 5928-5929, 6350-6353, 7463-7464, 7885-7888, 8998-8999, 9420-9421
.github/workflows/cmake.yml (1)
100-106: New Matrix Job for Ubuntu GCC No ChorbaThe new job configuration disables the Chorba feature (using
-DWITH_CHORBA=OFF) for Ubuntu builds. This addition aligns perfectly with the PR’s objective of testing builds with Chorba disabled.CMakeLists.txt (5)
88-90: Add Option for Optimized CRC32 (Chorba)A new option
WITH_CRC32_CHORBAhas been introduced with a default value of ON. This enables the optimized CRC32 algorithm (Chorba) and is consistent with the intended enhancements described in the PR.
175-178: Conditional Macro for Disabling ChorbaThe conditional block correctly defines
WITHOUT_CHORBAwhenWITH_CRC32_CHORBAis not enabled. This ensures that the code can differentiate based on whether the Chorba optimization is active.
1124-1127: New Private Headers AddedThe additions of
zmemory.handzutil_p.hintoZLIB_PRIVATE_HDRSare implemented here as expected. These headers enhance internal functionality and better manage dependency separation.
1160-1163: Integrating Optimized CRC32 Chorba SourceThe conditional appending of
arch/generic/crc32_chorba_c.ctoZLIB_SRCSwhenWITH_CRC32_CHORBAis enabled ensures that the optimized Chorba algorithm is built when desired. This implementation is correct and well integrated.
1406-1406: Feature Summary Entry for Chorba OptimizationAdding a feature summary entry for
WITH_CRC32_CHORBAhelps in clearly reporting the build configuration. This aids users in verifying whether the optimized CRC32 (Chorba) is enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
CMakeLists.txt (1)
1096-1127:⚠️ Potential issueRelocate
arch_functions.hto Public Headers
According to the PR objectives,arch_functions.hshould no longer be part of the private header list and instead should be declared as public. Currently, the file appears in the private header list (line marked with~next toarch_functions.h). To align with the objectives, please remove it fromZLIB_PRIVATE_HDRSand add it toZLIB_PUBLIC_HDRS. For example:--- a/CMakeLists.txt @@ set(ZLIB_PUBLIC_HDRS - ${CMAKE_CURRENT_BINARY_DIR}/zconf${SUFFIX}.h - ${CMAKE_CURRENT_BINARY_DIR}/zlib_name_mangling${SUFFIX}.h - ${CMAKE_CURRENT_BINARY_DIR}/zlib${SUFFIX}.h + ${CMAKE_CURRENT_BINARY_DIR}/zconf${SUFFIX}.h + ${CMAKE_CURRENT_BINARY_DIR}/zlib_name_mangling${SUFFIX}.h + ${CMAKE_CURRENT_BINARY_DIR}/zlib${SUFFIX}.h + arch_functions.h )and in the private section:
--- a/CMakeLists.txt @@ set(ZLIB_PRIVATE_HDRS - arch_functions.hThis change will ensure that
arch_functions.his treated as a public interface as intended.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
175-177: Conditionally Define WITHOUT_CHORBA Macro
This conditional block:if(NOT WITH_CRC32_CHORBA) add_definitions(-DWITHOUT_CHORBA) endif()ensures that the macro
WITHOUT_CHORBAis defined when the Chorba option is disabled. While the implementation is correct, note that using modern CMake practices, it might be preferable to scope such definitions to specific targets viatarget_compile_definitionsinstead of the globaladd_definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/cmake.yml(1 hunks)CMakeLists.txt(7 hunks)Makefile.in(2 hunks)adler32_p.h(2 hunks)arch/generic/adler32_c.c(1 hunks)arch/generic/crc32_braid_c.c(4 hunks)arch/generic/crc32_c.c(2 hunks)arch/generic/crc32_chorba_c.c(1 hunks)arch/generic/generic_functions.h(1 hunks)arch/s390/crc32-vx.c(2 hunks)arch/x86/crc32_pclmulqdq_tpl.h(3 hunks)crc32.h(1 hunks)crc32_braid_p.h(1 hunks)crc32_braid_tbl.h(15 hunks)crc32_c.h(0 hunks)functable.c(1 hunks)test/benchmarks/benchmark_crc32.cc(1 hunks)test/test_crc32.cc(1 hunks)tools/makecrct.c(4 hunks)
💤 Files with no reviewable changes (1)
- crc32_c.h
🚧 Files skipped from review as they are similar to previous changes (13)
- functable.c
- Makefile.in
- arch/generic/adler32_c.c
- crc32_braid_tbl.h
- test/test_crc32.cc
- arch/s390/crc32-vx.c
- arch/generic/crc32_chorba_c.c
- crc32.h
- adler32_p.h
- crc32_braid_p.h
- .github/workflows/cmake.yml
- arch/generic/crc32_braid_c.c
- arch/generic/generic_functions.h
🧰 Additional context used
🧠 Learnings (2)
arch/generic/crc32_c.c (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
CMakeLists.txt (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
🪛 Cppcheck (2.10-2)
test/benchmarks/benchmark_crc32.cc
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (10)
arch/generic/crc32_c.c (3)
2-4: LGTM! Header includes are well-organized.The includes are properly ordered from most specific to most general, and all necessary headers are present.
6-7: LGTM! Function signature change improves visibility control.The change from
PREFIX(crc32_c)toZ_INTERNAL uint32_t crc32_cmakes the internal nature of this function explicit.
9-38: LGTM! Well-structured conditional compilation for Chorba optimization.The
#ifndef WITHOUT_CHORBAblock cleanly separates the optimized path from the fallback implementation, making it easy to build without Chorba optimization when needed.test/benchmarks/benchmark_crc32.cc (1)
59-64: LGTM! Clear conditional benchmarking based on Chorba availability.The benchmarks are appropriately named and conditionally registered based on the
WITHOUT_CHORBAflag, ensuring accurate performance comparison.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
tools/makecrct.c (1)
18-20: LGTM! Clear type and constant definitions.The changes improve code clarity by:
- Explicitly defining the polynomial constant
- Using a descriptive name
BRAID_Wfor the word size- Explicitly defining
z_word_tasuint64_tarch/x86/crc32_pclmulqdq_tpl.h (2)
171-213: LGTM! Well-implemented fold_12 function for Chorba optimization.The function follows the established pattern of other fold functions and is properly guarded by
#ifndef WITHOUT_CHORBA. The implementation is clean and efficient, using PCLMULQDQ instructions appropriately.
403-403: LGTM! Improved macro naming.The change from
DO1toCRC_DO1makes the macro's purpose clearer and follows better naming conventions.CMakeLists.txt (3)
89-89: Add New Chorba Optimization Option
The new optionoption(WITH_CRC32_CHORBA "Enable optimized CRC32 algorithm Chorba" ON)is introduced to control the inclusion of the optimized Chorba-based CRC32 functionality. The default is set to ON, which appears consistent with enabling this optimization by default.
1160-1163: Includecrc32_chorba_c.cWhen Chorba is Enabled
The following block:if(WITH_CRC32_CHORBA) list(APPEND ZLIB_SRCS arch/generic/crc32_chorba_c.c) endif()correctly appends the source file for the Chorba optimization to the build when enabled. This aligns with the PR objectives and build configuration.
1406-1406: Integrate Feature Summary for Chorba Option
Adding this feature information:add_feature_info(WITH_CRC32_CHORBA WITH_CRC32_CHORBA "Use optimized CRC32 algorithm Chorba")ensures that the build summary clearly reflects the configuration state of the Chorba optimization. This improves overall project documentation and is implemented correctly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1873 +/- ##
===========================================
- Coverage 82.44% 82.39% -0.05%
===========================================
Files 141 141
Lines 12558 12556 -2
Branches 2894 2894
===========================================
- Hits 10353 10346 -7
- Misses 1232 1239 +7
+ Partials 973 971 -2 ☔ View full report in Codecov by Sentry. |
|
LGTM |
- Rename N and W to BRAID_N and BRAID_W - Remove override capabilities for BRAID_N and BRAID_W - Fix formatting in crc32_braid_tbl.h - Make makecrct not rely on crc32_braid_p.h
Mark crc32_c and crc32_braid functions as internal, and remove prefix. Reorder contents of generic_functions, and remove Z_INTERNAL hints from declarations. Add test/benchmark output to indicate whether Chorba is used.
…config. Add CI run without chorba enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/makecrct.c (1)
18-20: LGTM! Consider enhancing the POLY constant documentation.The constant definitions and type declaration are correct. The POLY constant represents the standard CRC32 polynomial, and BRAID_W is appropriately set for 64-bit word size.
Consider adding a more detailed comment for the POLY constant to explain its derivation:
-#define POLY 0xedb88320 /* p(x) reflected, with x^32 implied */ +#define POLY 0xedb88320 /* p(x) = x^32+x^26+x^23+x^22+x^16+x^12+x^11+x^10+x^8+x^7+x^5+x^4+x^2+x+1 reflected */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/cmake.yml(1 hunks)CMakeLists.txt(7 hunks)Makefile.in(2 hunks)adler32_p.h(2 hunks)arch/generic/adler32_c.c(1 hunks)arch/generic/crc32_braid_c.c(4 hunks)arch/generic/crc32_c.c(2 hunks)arch/generic/crc32_chorba_c.c(1 hunks)arch/generic/generic_functions.h(1 hunks)arch/s390/crc32-vx.c(2 hunks)arch/x86/crc32_pclmulqdq_tpl.h(3 hunks)crc32.h(1 hunks)crc32_braid_p.h(1 hunks)crc32_braid_tbl.h(15 hunks)crc32_c.h(0 hunks)functable.c(1 hunks)test/benchmarks/benchmark_crc32.cc(1 hunks)test/test_crc32.cc(1 hunks)tools/makecrct.c(4 hunks)
💤 Files with no reviewable changes (1)
- crc32_c.h
🚧 Files skipped from review as they are similar to previous changes (13)
- Makefile.in
- arch/generic/adler32_c.c
- crc32_braid_tbl.h
- arch/generic/crc32_braid_c.c
- arch/s390/crc32-vx.c
- crc32.h
- .github/workflows/cmake.yml
- adler32_p.h
- arch/generic/crc32_chorba_c.c
- functable.c
- crc32_braid_p.h
- CMakeLists.txt
- arch/generic/generic_functions.h
🧰 Additional context used
🧠 Learnings (1)
arch/generic/crc32_c.c (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
🪛 Cppcheck (2.10-2)
test/benchmarks/benchmark_crc32.cc
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (40)
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
🔇 Additional comments (9)
tools/makecrct.c (2)
159-173: LGTM! Clean preprocessor directive structure.The conditional compilation logic using BRAID_W is well-structured and correctly handles both 32-bit and 64-bit cases.
177-220: LGTM! Well-structured table generation with clear preprocessor boundaries.The nested preprocessor directives for BRAID_N and BRAID_W are well-organized, with proper comments marking the boundaries of each conditional block.
arch/x86/crc32_pclmulqdq_tpl.h (2)
171-213: LGTM! Well-structured implementation of the fold_12 function.The implementation follows the established pattern of other fold functions and is properly guarded by the
WITHOUT_CHORBAdirective.
403-403:Details
✅ Verification successful
Verify the CRC_DO1 macro definition.
The
DO1macro has been replaced withCRC_DO1. Let's verify that this macro is properly defined.
🏁 Script executed:
#!/bin/bash # Description: Search for the CRC_DO1 macro definition # Expected: Find the macro definition in header files # Search for the macro definition rg -A 2 '#define\s+CRC_DO1'Length of output: 227
Verification: CRC_DO1 macro definition confirmed
The CRC_DO1 macro is correctly defined in
crc32_braid_p.has:#define CRC_DO1 c = crc_table[(c ^ *buf++) & 0xff] ^ (c >> 8)This confirms that the macro replacement (from
DO1toCRC_DO1) is valid and being used correctly inarch/x86/crc32_pclmulqdq_tpl.h(line 403).arch/generic/crc32_c.c (3)
1-6: LGTM!The function signature change to mark
crc32_cas internal aligns with the PR objectives, and all necessary headers are included.
40-42: LGTM!The post-conditioning of the CRC value is correctly implemented.
9-38:Details
✅ Verification successful
LGTM! Verify Chorba thresholds.
The Chorba implementation is well-structured with clear thresholds and proper conditional compilation. The use of
OPTIMAL_CMPfor testing optimal size aligns with the PR objectives.Run the following script to verify the threshold values:
🏁 Script executed:
#!/bin/bash # Description: Verify Chorba threshold values across the codebase. # Search for threshold definitions rg -A 1 'CHORBA_.*_THRESHOLD'Length of output: 1158
LGTM! Threshold values verified.
The script output confirms that the threshold definitions in
crc32.h(e.g.,
•CHORBA_LARGE_THRESHOLDdefined as(sizeof(z_word_t) * 64 * 1024)
•CHORBA_MEDIUM_LOWER_THRESHOLDat 8192 &CHORBA_MEDIUM_UPPER_THRESHOLDat 32768
•CHORBA_SMALL_THRESHOLD_64BITat 72 andCHORBA_SMALL_THRESHOLD_32BITat 80)
are correctly referenced inarch/generic/crc32_c.cas per the conditional logic. The usage ofOPTIMAL_CMPfor selecting the appropriate Chorba variant further aligns with the objectives. No further changes are necessary.test/benchmarks/benchmark_crc32.cc (1)
59-65: LGTM!The conditional compilation for benchmarks correctly handles both Chorba and non-Chorba builds, aligning with the PR objectives. The benchmark registration is well-structured.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 60-60: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
test/test_crc32.cc (1)
227-233: LGTM!The conditional compilation for tests correctly handles both Chorba and non-Chorba builds, aligning with the PR objectives. The test registration is well-structured.
| if(aligned_len > CHORBA_LARGE_THRESHOLD) | ||
| c = crc32_chorba_118960_nondestructive(c, (z_word_t*) aligned_buf, aligned_len); | ||
| # if W == 8 | ||
| # if OPTIMAL_CMP == 64 |
There was a problem hiding this comment.
It might be a good idea to split this into two defines - one to reflect the size of the registers, and one to reflect the availability of unaligned loads and stores.
Summary by CodeRabbit
New Features
generic_chorbaCRC32 processing.Tests
Chores