114-114: LGTM! Unconditional alignment sanitizer check improves safety.

The unconditional inclusion of the 'alignment' check in the sanitizer list is a positive change that aligns with the PR's objective of cleaning up unaligned memory access checks. This will help catch potential memory alignment issues across all builds.

246-254: LGTM! Well-structured architecture-specific optimizations.

The implementation correctly defines optimal comparison sizes for x86_64 (64 bytes), i386 (32 bytes), and ARM64 (64 bytes with unaligned access support). The architecture detection and conditions are accurate.

255-262: LGTM! Proper handling of ARM and PowerPC architectures.

The code correctly:

• Checks for ARM FEATURE_UNALIGNED support
• Handles PowerPC64 with appropriate endianness check
• Sets appropriate OPTIMAL_CMP values (32/64 bytes) based on architecture capabilities

264-269: LGTM! Safe fallback mechanism.

The implementation provides a robust fallback mechanism:

1. Respects NO_UNALIGNED override by unsetting OPTIMAL_CMP
2. Defaults to a safe 8-byte comparison size when no architecture-specific optimization is available

84-88: LGTM! Simplified UBSAN workflow configuration.

The changes correctly:

• Update the job name to reflect the removal of unaligned checks
• Remove the redundant WITH_UNALIGNED flag from cmake arguments
  This simplification aligns with the PR's cleanup objectives.

58-65: LGTM: Proper replacement of UNALIGNED_OK checks

The change from UNALIGNED_OK to BYTE_ORDER == LITTLE_ENDIAN is appropriate since unaligned access is primarily a concern for little-endian architectures. The nested checks for HAVE_BUILTIN_CTZ/CTZLL are also properly structured.

14-16: LGTM: Consistent with test file changes

The ZTEST_ALL workaround is consistently applied across test files.

67-74: LGTM: Proper conditional compilation structure

The conditional compilation structure correctly mirrors the test file, maintaining consistency across the test suite.

72-80: LGTM: Proper architecture-specific conditional compilation

The changes correctly:

1. Replace UNALIGNED_OK with BYTE_ORDER checks
2. Maintain proper nesting of CPU feature checks
3. Keep consistent structure with other test files

16-18: Review the direct inclusion of C implementation file

The comment acknowledges this as an "unorthodox workaround", but including a .c file directly is generally discouraged. Consider:

1. Moving the test-specific functions to a header file
2. Creating a test-specific header that exposes these functions

32-39: Verify performance impact of consistent memcpy usage

The changes improve safety and consistency by using memcpy for all cases, but we should verify there's no significant performance regression.

Consider benchmarking the new implementation against the old one, particularly on platforms where unaligned access was previously optimized.

#!/bin/bash
# Look for existing benchmarks that measure hash calculation performance
rg -l "benchmark.*hash" test/benchmarks/

#!/bin/bash
# Check content of slidehash benchmark which is most relevant to our case
rg -A 10 "benchmark.*hash" test/benchmarks/benchmark_slidehash.cc

# Also check the implementation details around HASH_CALC_READ
rg -B 5 -A 10 "HASH_CALC_READ" --type cpp

#!/bin/bash
# Check the git history for the previous implementation of HASH_CALC_READ
git log -p --no-merges -S "HASH_CALC_READ" insert_string_tpl.h

# Also check if there are any existing performance-related issues or discussions
gh issue list --search "performance hash"
gh pr list --search "performance hash"

#!/bin/bash
# Get the PR description and comments for PR #1828
gh pr view 1828 --json body,comments

# Also check for any benchmark results or performance discussions in the PR
gh pr view 1828 --json comments | jq -r '.comments[].body' | grep -i "performance\|benchmark"

Line range hint 47-74: LGTM: Safe implementation of unaligned 16-bit comparison

The implementation correctly uses memcpy for safe unaligned memory access and properly handles byte order requirements.

Line range hint 76-103: LGTM: Efficient implementation using compiler intrinsics

The implementation makes good use of __builtin_ctz for efficient comparison while maintaining safety with proper memory access patterns.

Line range hint 104-132: LGTM: Verify 64-bit alignment requirements

The implementation is efficient and safe, using appropriate compiler intrinsics for 64-bit operations.

#!/bin/bash
# Verify that 64-bit operations are only used on supported platforms
rg -l "HAVE_BUILTIN_CTZLL" | xargs rg "compare256.*64"

#!/bin/bash
# Check how HAVE_BUILTIN_CTZLL is defined and its platform dependencies
rg "HAVE_BUILTIN_CTZLL" -B 3 -A 3

# Check if there are any platform-specific checks or fallbacks
rg "compare256_rle" -B 2 -A 2

34-53: LGTM: Well-structured conditional compilation

The implementation properly handles different architectures and capabilities with a clear fallback hierarchy.

Line range hint 57-71: LGTM: Clear and maintainable implementation selection

The generic implementation selection is well-structured and maintainable.

Line range hint 62-142: LGTM: Safe and efficient unaligned comparison implementations

The implementations properly handle unaligned access and follow best practices for memory comparison.

Line range hint 143-183: LGTM: Consistent implementation for 64-bit operations

The implementation maintains consistency with other bit-width implementations while properly handling 64-bit specific requirements.

230-232: LGTM! Verify OPTIMAL_CMP usage across the codebase.

The switch from UNALIGNED checks to OPTIMAL_CMP for alignment control is a good improvement. The alignment masks (7 for 8-byte alignment, 3 for 4-byte alignment) are correctly defined based on the optimal comparison size.

Let's verify the OPTIMAL_CMP usage:

#!/bin/bash
# Description: Check OPTIMAL_CMP usage and definitions
# Verify that OPTIMAL_CMP is consistently used and properly defined

# Find OPTIMAL_CMP definition and usage
rg -n "OPTIMAL_CMP" --type c --type h

# Check if there are any remaining UNALIGNED checks that should be migrated
rg -n "UNALIGNED.*OK" --type c --type h

#!/bin/bash
# Let's examine the context around OPTIMAL_CMP definitions and usage

# Check zbuild.h definitions with context
rg -A 5 -B 5 "OPTIMAL_CMP" zbuild.h

# Check chunkset_tpl.h usage with context
rg -A 5 -B 5 "OPTIMAL_CMP" chunkset_tpl.h

# Look for any other alignment-related macros that might interact
rg -n "(ALIGN|align)" chunkset_tpl.h

73-81: Verify architectural assumptions about endianness

The unaligned benchmarks are now conditionally compiled only for little-endian systems. While this aligns with removing UNALIGNED_OK checks, we should verify:

1. That unaligned access is indeed only supported/optimized for little-endian
2. Whether we're missing test coverage for big-endian systems

#!/bin/bash
# Search for any other endianness-specific code patterns
rg "BYTE_ORDER|LITTLE_ENDIAN|BIG_ENDIAN" --type c --type cpp

# Look for other unaligned access implementations
rg "unaligned|UNALIGNED" --type c --type cpp

Line range hint 47-104: Verify BYTE_ORDER macro availability across all supported platforms.

The change from UNALIGNED_OK to BYTE_ORDER-based conditional compilation is good, but we should ensure BYTE_ORDER is consistently defined.

#!/bin/bash
# Description: Check if BYTE_ORDER is consistently defined across the codebase
# and if there are any platform-specific definitions we need to handle

# Search for BYTE_ORDER definitions or includes
rg -n "BYTE_ORDER|endian\.h" 

# Check for platform-specific handling
rg -n "#if.*BYTE_ORDER" 

#!/bin/bash
# Check the complete implementation of zendian.h
cat zendian.h

Line range hint 104-132: Verify fallback behavior when builtin functions are unavailable.

The optimized implementations correctly use memcpy for safe unaligned access. However, we should verify the fallback behavior when HAVE_BUILTIN_CTZ/CTZLL are not defined.

#if BYTE_ORDER == LITTLE_ENDIAN
#  if defined(HAVE_BUILTIN_CTZLL)
#    define compare256_rle compare256_rle_unaligned_64
#  elif defined(HAVE_BUILTIN_CTZ)
#    define compare256_rle compare256_rle_unaligned_32
#  else
#    define compare256_rle compare256_rle_unaligned_16
#  endif
#else
#  define compare256_rle compare256_rle_c
#endif

#!/bin/bash
# Description: Check how the code handles platforms without CTZ/CTZLL builtins

# Search for fallback implementations
rg -n "fallback.*ctz" 

# Check if there are platform-specific alternatives
ast-grep --pattern 'define HAVE_BUILTIN_CTZ'
ast-grep --pattern 'define HAVE_BUILTIN_CTZLL'

#!/bin/bash
# Let's check for any non-builtin implementations of compare256_rle
rg -n "compare256_rle" -A 10

# Also search for any other CTZ implementations
rg -n "ctz" -B 2 -A 2

# Check for compiler detection macros
rg -n "HAVE_BUILTIN" -B 2 -A 2

57-58: LGTM! Generic implementation selection is consistent.

The selection of generic implementations aligns well with the architecture-specific function declarations above.

34-38: LGTM! Verify function availability across architectures.

The reorganization of functions under BYTE_ORDER != LITTLE_ENDIAN improves clarity and architecture-specific handling.

Let's verify the function declarations match their implementations:

#!/bin/bash
# Description: Verify function declarations match implementations
# Search for function implementations in source files
rg -l "^(uint32_t |void |static.*)(longest_match_c|longest_match_slow_c|compare256_unaligned_c)\b" arch/

#!/bin/bash
# Search for the function implementations with context
rg -A 5 "^(uint32_t |void |static.*)(longest_match_c|longest_match_slow_c|compare256_unaligned_c)\b" --type c

# Also check for any potential references to these functions
rg "(longest_match_c|longest_match_slow_c|compare256_unaligned_c)" --type c

#!/bin/bash
# Get the content of compare256_c.c to verify implementations
cat arch/generic/compare256_c.c

Line range hint 62-142: LGTM! Safe unaligned access implementation.

The use of zng_memcmp_2 ensures safe 16-bit comparisons without alignment requirements.

143-144: Verify memory safety for 64-bit comparisons.

The removal of UNALIGNED64_OK check relies on memcpy for safe access, but we should verify this is sufficient across all platforms.

Let's check for potential alignment issues:

#!/bin/bash
# Description: Search for alignment-related issues or discussions
# Look for alignment-related comments or issues
rg -i "align|unaligned" --type md
rg -i "align|unaligned" --type cpp arch/ include/ -C2

#!/bin/bash
# Let's check the implementation details of compare256 functions
ast-grep --pattern 'compare256_unaligned_64($$$)'
ast-grep --pattern 'compare256_unaligned_32($$$)'
ast-grep --pattern 'compare256_unaligned_16($$$)'

# Also check for memcpy usage in these functions
rg "memcpy" arch/generic/compare256_c.c -C3

13-21: LGTM: Well-structured function selection hierarchy

The changes implement a clear and logical selection of comparison functions:

1. 64-bit variant when CTZLL is available
2. 32-bit variant when CTZ is available
3. 16-bit variant as a fallback for little-endian
4. Basic C implementation for big-endian

This aligns with the PR objectives by removing UNALIGNED_OK checks while maintaining optimal function selection based on available CPU features.

Line range hint 11-45: LGTM: Well-implemented basic comparison function

The C implementation is well-structured with:

• Proper bounds checking (len < 256)
• Efficient unrolled loop for byte comparisons
• Clear fallback for big-endian systems

Line range hint 47-75: LGTM: Safe 16-bit comparison implementation

The 16-bit comparison function:

• Uses safe memory access through memcpy
• Properly handles partial matches
• Maintains correct alignment requirements

34-51: LGTM! Well-structured function declarations based on CPU capabilities.

The hierarchical organization (CTZLL > CTZ > fallback) provides optimal function selection for each architecture, with a clean separation between LITTLE_ENDIAN and BIG_ENDIAN paths.

56-69: Verify implementation and usage of generic functions.

The macro definitions look good, maintaining a clean mapping to the declared functions. Let's verify:

1. Implementation of all referenced functions
2. Usage of these macros in dependent code

67-74: LGTM! Clean conditional compilation structure

The changes improve the code by:

1. Replacing UNALIGNED_OK checks with direct endianness checks
2. Using consistent defined() checks for builtin functions
3. Simplifying function names

Let's verify the consistency of these changes across the codebase:

#!/bin/bash
# Check for any remaining UNALIGNED_OK checks that might need cleanup
rg "UNALIGNED_OK" 

# Verify the new function names are used consistently
rg "compare256_rle_(16|32|64)"

# Check for consistent endianness checks
rg "BYTE_ORDER.*LITTLE_ENDIAN"

13-15: Consider documenting the ZTEST_ALL workaround more thoroughly

While workarounds in test code are sometimes necessary, it would be helpful to document:

1. Why this workaround is needed
2. What functions it provides access to
3. Whether this could be solved through a better design

Let's check if this workaround is consistently used across other test files:

1-4: License header looks fine and consistent with project standards.
No further action needed.

6-10: Includes are appropriate and minimal.
These header inclusions provide the necessary dependencies without apparent redundancies.

64-90: 64-bit mismatch detection logic is consistent with 32-bit version.
The approach extends naturally to 64-bit words. The cast of match_byte to uint32_t is correct, ensuring the correct return type. Good job retaining a consistent pattern.

36-62: Verify usage of built-in functions for mismatch detection.
The code uses __builtin_ctz for determining the first differing byte. This is a good optimization, but confirm that all target compilers and architectures support it correctly (especially cross-compiling scenarios).

#!/bin/bash
# Searching for usage of __builtin_ctz in the codebase to verify compatibility across files.
ast-grep --pattern $'__builtin_ctz($_)' 

#!/bin/bash
# Let's check for HAVE_BUILTIN_CTZ macro definition and its usage
rg "HAVE_BUILTIN_CTZ" -B 2 -A 2

# Also search for any configure/build system checks for this builtin
fd -e configure -e m4 -e cmake -X rg "builtin_ctz|__builtin_ctz"

# Check if there are any other similar built-in function checks
rg "__builtin_(clz|ctz)" -B 2 -A 2

10-10: Include for compare256_p.h ensures the inline functions are accessible.
This is an appropriate place to include the new header.

13-18: Dynamic selection of comparison variant is clear.
The logic uses #if checks around HAVE_BUILTIN_CTZLL, HAVE_BUILTIN_CTZ, and a fallback for 16-bit variant. This is concise and helps maintain a smaller code footprint.

22-44: Macro definitions for generating longest_match variants.
The approach is consistent and maintains similar selection logic for different built-ins. No immediate issues here. Just ensure match_tpl.h references the correct macros.

62-67: Benchmark registration is concise; watch for unknown macro warnings.
Cppcheck warns about potential unknown macros (e.g., BENCHMARK_DEFINE_F). If that macro is from a recognized library (Google Benchmark), this is typically harmless, but consider configuring the static analyzer to avoid false positives.

#!/bin/bash
# Attempt to locate definition of BENCHMARK_DEFINE_F in the codebase
rg -A 5 'BENCHMARK_DEFINE_F'

#!/bin/bash
# Check the includes and macro definition in the file
cat test/benchmarks/benchmark_compare256_rle.cc

15-15: Including "arch/generic/compare256_p.h" directly.
This can sometimes raise concerns about mixing test code with private/internal headers. However, the comment in past reviews clarifies this is intentional for direct testing.

67-72: Tests for 16, 32, and 64-bit variants.
All three coverage targets look correct. This ensures thorough testing of each specialized function.

15-15: LGTM: Header inclusion aligns with architectural changes

The addition of compare256_p.h is consistent with the move towards aligned comparisons and the removal of unaligned variants.

68-74: Verify architecture-specific benchmark registration

The replacement of unaligned benchmarks with aligned versions is good, but we should verify the architecture-specific conditions:

1. Base compare256_16 is unconditional
2. compare256_32 requires HAVE_BUILTIN_CTZ
3. compare256_64 requires HAVE_BUILTIN_CTZLL

#!/bin/bash
# Description: Verify that the compare256 functions are properly defined in the header

# Check if the compare256 functions are properly defined
ast-grep --pattern 'uint32_t compare256_$_(const uint8_t* src0, const uint8_t* src1)' arch/generic/compare256_p.h

#!/bin/bash
# Let's try a different approach to verify the architecture-specific conditions

# 1. Search for compare256 function declarations
rg -n "compare256_(16|32|64)" --type cpp -A 2

# 2. Check the conditional compilation directives around these functions
rg -n "HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CTZLL" --type cpp -B 2 -A 2

64-66: Verify function implementations exist

The replacement of generic implementations with architecture-specific ones looks good, but we should verify the existence of these functions.

1077-1079: LGTM: Header additions support architectural changes

The addition of these headers to ZLIB_PRIVATE_HDRS is appropriate:

1. chunk_permute_table.h likely contains permutation tables for chunk operations
2. compare256_p.h contains the new aligned comparison functions
3. generic_functions.h provides architecture-independent implementations

13-19: LGTM! Well-structured function selection based on CPU capabilities.

The hierarchical fallback from 64-bit to 32-bit to 16-bit operations is well-designed, ensuring optimal performance across different architectures.

22-40: Verify performance impact of function selection.

Given the PR objectives mentioning potential performance concerns on platforms without unaligned access, we should verify the performance impact of these changes.

Run the following script to identify platforms that might be affected:

62-67: LGTM! Comprehensive benchmark coverage for all variants.

The benchmarks are properly structured to test all comparison variants based on available CPU capabilities:

• Base 16-bit version is always available
• 32-bit version when CTZ is available
• 64-bit version when CTZLL is available

36-36: LGTM! Function declaration aligns with implementation.

The addition of compare256_c declaration is consistent with the PR's objective of cleaning up and consolidating comparison functions.

15-15: LGTM: Header inclusion aligns with new architecture

The inclusion of compare256_p.h is appropriate as it provides the new comparison function implementations.

67-72: Verify test coverage for all comparison functions

The conditional compilation ensures tests are only built when the required CPU features are available. However, we should verify that all code paths are tested.

#!/bin/bash
# Description: Check test coverage for comparison functions
# Look for untested comparison functions

# Find all compare256 function declarations
rg -p "static\s+inline\s+uint32_t\s+compare256_\w+" --type cpp

# Find corresponding test cases
rg -p "TEST_COMPARE256\(" --type cpp

#!/bin/bash
# Search for RLE comparison function tests
rg -p "TEST.*compare256_rle" --type cpp

# Get more context around TEST_COMPARE256 macro to understand test coverage
rg -p "TEST_COMPARE256" -B 5 -A 10 test/test_compare256.cc

Line range hint 69-94: Verify 64-bit comparison edge cases

The 64-bit comparison function handles endianness correctly but should be tested with various alignment scenarios.

15-15: LGTM: Header inclusion aligns with compare256 reorganization

The addition of compare256_p.h from the generic architecture directory is appropriate for the benchmark reorganization.

68-73: LGTM: Well-structured benchmark registration with proper guards

The new benchmark registrations are properly guarded with appropriate compiler feature checks:

• compare256_16 is always available
• compare256_32 requires HAVE_BUILTIN_CTZ
• compare256_64 requires HAVE_BUILTIN_CTZLL

1077-1079: LGTM: Properly added new generic architecture headers

The new headers are correctly added to the private headers list:

• chunk_permute_table.h
• compare256_p.h
• generic_functions.h

12-34: Consider bounds checking for memory access

While the function implementation is efficient, it lacks parameter validation.

The unrolled loop implementation looks good

The manual loop unrolling with 4 iterations per cycle is a good optimization technique. The implementation correctly handles both the 2-byte comparisons and single-byte fallback on mismatch.

68-73: LGTM! Benchmark coverage aligns with PR objectives

The changes correctly:

1. Remove unaligned comparison benchmarks
2. Add benchmarks for new aligned variants (16/32/64-bit)
3. Use appropriate conditional compilation based on CPU features

62-67: LGTM! Consistent with non-RLE benchmark changes

The benchmark registration follows the same pattern as benchmark_compare256.cc, maintaining consistency across the codebase.

1077-1079: New generic architecture headers align with cleanup objectives.

The addition of these header files in the generic architecture directory supports the PR's goal of cleaning up alignment-related code:

• chunk_permute_table.h: Likely contains permutation tables for chunk operations
• compare256_p.h: New implementation for 256-byte comparisons
• generic_functions.h: Common function declarations for generic architecture

Let's verify the integration of these headers:

#!/bin/bash
# Check if the new headers are properly referenced in the codebase
echo "Checking usage of new headers..."
rg -l "chunk_permute_table\.h|compare256_p\.h|generic_functions\.h" --type c

# Verify if these headers replace any old alignment-related code
echo "Checking for old alignment-related code..."
rg -l "UNALIGNED_OK" --type c

#!/bin/bash
# Let's check the content and relationships of these headers

# Check content of generic_functions.h
echo "=== Content of generic_functions.h ==="
cat arch/generic/generic_functions.h

# Check content of compare256_p.h
echo -e "\n=== Content of compare256_p.h ==="
cat arch/generic/compare256_p.h

# Check content of chunk_permute_table.h
echo -e "\n=== Content of chunk_permute_table.h ==="
cat arch/generic/chunk_permute_table.h

# Check how these are included in architecture-specific implementations
echo -e "\n=== Usage in arch_functions.h ==="
cat arch_functions.h

# Check for any related changes in the implementation files
echo -e "\n=== Usage in SSSE3 implementation ==="
cat arch/x86/chunkset_ssse3.c

echo -e "\n=== Usage in NEON implementation ==="
cat arch/arm/chunkset_neon.c

20-22: Clean implementation using macro-based dispatch.

The simplified implementation improves maintainability while allowing platform-specific optimizations through the COMPARE256 macro.

7-18: LGTM! Verify platform-specific performance impact.

The macro-based selection of comparison functions with priority for byte version when OPTIMAL_CMP=8 aligns well with the previous discussions about performance on platforms without unaligned reads.

Run this script to analyze platform-specific configurations:

#!/bin/bash
# Description: Check which platforms might benefit from OPTIMAL_CMP=8

# Search for platform-specific configurations and CPU feature checks
rg -l "HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CTZLL|UNALIGNED_OK" arch/

#!/bin/bash
# Check the content of platform-specific files to understand the configuration logic

# Check generic implementation header
echo "=== Generic implementation header (compare256_p.h) ==="
cat arch/generic/compare256_p.h

# Check ARM-specific configurations
echo -e "\n=== ARM configurations ==="
rg "UNALIGNED_OK|HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CTZLL|OPTIMAL_CMP" arch/arm/arm_functions.h

# Check x86-specific configurations
echo -e "\n=== x86 configurations ==="
rg "UNALIGNED_OK|HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CTZLL|OPTIMAL_CMP" arch/x86/x86_functions.h

#!/bin/bash
# Check for unaligned memory access handling and platform-specific optimizations

# Check memory access functions
echo "=== Memory access functions ==="
rg "zng_memread|zng_memcmp" arch/generic/zmemory.h || rg "zng_memread|zng_memcmp" zmemory.h

# Check for NEON implementation details
echo -e "\n=== ARM NEON implementation ==="
head -n 30 arch/arm/compare256_neon.c

# Check for SSE2/AVX2 implementation details
echo -e "\n=== x86 SSE2/AVX2 implementations ==="
head -n 30 arch/x86/compare256_sse2.c
head -n 30 arch/x86/compare256_avx2.c

68-74: Maintain consistency with RLE benchmark error messages.

The benchmark structure is good, but error messages should be consistent with any improvements made to the RLE benchmarks.

Update the error message to be more descriptive, maintaining consistency with RLE benchmarks:

-            state.SkipWithError("CPU does not support " #name);
+            state.SkipWithError("CPU does not support " #name "-bit comparisons (missing required CPU feature)");

11-42: LGTM! Efficient byte-by-byte comparison implementation.

The unrolled loop design optimizes performance by reducing branch predictions.

45-67: LGTM! Well-implemented 16-bit comparison with proper partial match handling.

The implementation correctly handles both full 16-bit matches and partial byte matches.

97-123: LGTM! 64-bit comparison implementation with proper type handling.

The implementation correctly handles 64-bit comparisons with appropriate type casting for the return value.

69-95: Verify compiler support for CTZ/CLZ built-ins.

The implementation efficiently uses built-in functions for trailing/leading zero counts. Let's verify compiler support across different platforms.

#!/bin/bash
# Description: Check compiler support for CTZ/CLZ built-ins
# Test: Search for compiler checks and definitions
rg -A 5 "HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CLZ"

246-271: Well-structured architecture-specific optimizations.

The OPTIMAL_CMP definitions are properly organized with:

• 64-bit optimization for x86_64 and ARM64 with unaligned access
• 32-bit optimization for i386 and ARM with unaligned access
• Safe fallback to 8-bit for architectures without unaligned access
• Default to 16-bit when architecture is unknown

76-76: Clarify the boolean return logic in zng_memcmp_2.

This function returns 1 if there is a difference, 0 otherwise, which is an unconventional usage of “memcmp” semantics. Ensure all call sites correctly interpret this boolean-like return value.

92-92: Confirm usage alignment in zng_memcmp_8.

Returning 1 or 0 instead of a signed integer might be intentional, but verify that it aligns with downstream usage and avoids confusion.

62-63: Confirm coverage and naming consistency.

Adding benchmarks for compare256_rle_8 and compare256_rle_16 looks fine. Ensure that all relevant compare sizes are tested if needed, and confirm the naming scheme remains consistent across the codebase.

64-66: Conditionally compiled benchmark for 32-byte comparisons.

Having this block guarded by HAVE_BUILTIN_CTZ suggests 32-byte RLE comparisons might be unavailable on some platforms. Verify that fallback or alternative benchmarks exist if HAVE_BUILTIN_CTZ is not defined.

15-15: Include path verification.

#include "arch/generic/compare256_p.h" is newly added. Confirm that the path aligns with the repository’s structure and that no duplication occurs with other headers.

68-74: Expansion of aligned comparison benchmarks.

Benchmarks for 8, 16, 32, and 64 bytes are conditionally compiled. This is consistent with the removal of unaligned comparisons. Just ensure runtimes on platforms where some macros (e.g. HAVE_BUILTIN_CTZ, HAVE_BUILTIN_CTZLL) aren’t defined still receive the intended fallback coverage.

11-11: New include for compare256_rle.h.

Ensure that only what is needed from this header is included and that the dependency is minimal. If usage expands, a forward declaration might be insufficient. Current approach looks fine.

13-20: Macro-based selection approach for compare256_rle.

Selecting a compare function based on OPTIMAL_CMP or builtins is a neat optimization. However, confirm that this logic does not break on unusual compiler or platform configurations that might define multiple or no macros. Additional fallback logic or explicit checks can help maintain portability.

If you need a script to verify coverage for each macro condition, let me know.

246-254: LGTM! Well-documented optimization strategy.

The comment block provides clear and comprehensive documentation about the OPTIMAL_CMP values and their intended use cases, with an important clarification about unaligned access safety.

255-278: LGTM! Comprehensive architecture-specific optimizations.

The implementation provides well-structured architecture detection and appropriate OPTIMAL_CMP values based on platform capabilities and word sizes.

279-281: Verify performance impact of default OPTIMAL_CMP change.

The default OPTIMAL_CMP value has been changed from 8 to 16. While this aligns with the documentation, we should verify that this change doesn't negatively impact performance on platforms without unaligned access support.

#!/bin/bash
# Search for any existing performance tests or benchmarks related to compare functions
rg -l "compare.*benchmark|benchmark.*compare" --type c --type cpp

# Look for any platform-specific implementations that might be affected
rg -l "OPTIMAL_CMP.*8|compare.*8" --type c --type h

# Search for any existing issues or discussions about OPTIMAL_CMP
gh issue list --search "OPTIMAL_CMP"
gh pr list --search "OPTIMAL_CMP"

#!/bin/bash
# Check the content of benchmark files to understand performance implications
rg -A 5 "OPTIMAL_CMP" test/benchmarks/benchmark_compare256*.cc

# Look at platform-specific implementations to understand unaligned access handling
cat arch/generic/generic_functions.h
cat arch/generic/compare256_p.h

# Check the PR that's related to UNALIGNED_OK checks
gh pr view 1830 --json body

#!/bin/bash
# Check the generic compare256 implementation and its usage of OPTIMAL_CMP
cat arch/generic/compare256_c.c

# Look for any platform-specific OPTIMAL_CMP definitions
rg "define.*OPTIMAL_CMP" --type c --type h

62-63: LGTM: New benchmarks for 8-bit and 16-bit comparisons

The addition of benchmarks for 8-bit and 16-bit comparisons aligns with the PR objectives of consolidating comparison functions. These variants are unconditionally available, which matches the architectural independence of byte-level comparisons.

64-68: Verify CPU feature detection for conditional benchmarks

The conditional compilation for 32-bit and 64-bit comparisons is correctly gated behind CPU feature checks (HAVE_BUILTIN_CTZ and HAVE_BUILTIN_CTZLL).

Let's verify the CPU feature detection macros are consistently defined across the codebase:

#!/bin/bash
# Description: Verify CPU feature detection consistency
# Check if HAVE_BUILTIN_CTZ and HAVE_BUILTIN_CTZLL are consistently defined

# Search for CTZ-related macro definitions and usage
echo "=== Searching for CTZ macro definitions ==="
rg -n "HAVE_BUILTIN_CTZ|HAVE_BUILTIN_CTZLL" --type c --type cpp

# Search for other comparison function implementations that might need similar checks
echo -e "\n=== Searching for related comparison functions ==="
ast-grep --pattern 'compare256_rle_$_'

68-74: LGTM! Well-structured benchmark organization.

The new benchmarks are properly organized with appropriate CPU feature guards. The progression from 8-bit to 64-bit comparisons allows for comprehensive performance testing across different architectures.

71-94: Verify endianness handling in builtin_ctz usage.

The endianness handling for finding the first differing byte is crucial for correctness. Please verify that the calculation __builtin_ctz(diff) / 8 and __builtin_clz(diff) / 8 correctly identifies the byte position on all supported platforms.

#!/bin/bash
# Search for any endianness-related test cases
rg -l "BYTE_ORDER|__BYTE_ORDER" test/

#!/bin/bash
# Search for endianness-related code and tests in the entire codebase
rg -l "BYTE_ORDER|__BYTE_ORDER|endian"

# Look for test files that might verify memory comparison functions
fd -e c -e h test

#!/bin/bash
# Check the endianness handling in compare functions across architectures
rg -A 5 -B 5 "builtin_[cl]tz.*diff.*8" 

# Check test files for memory comparison tests
cat test/test_shared.h test/test_shared_ng.h

# Check endianness definitions and handling
cat zendian.h

#!/bin/bash
# Check the implementation of zng_memread functions
rg -A 5 "zng_memread_[48]" zbuild.h

246-254: LGTM! Well-documented OPTIMAL_CMP values.

The comments clearly explain the rationale behind each OPTIMAL_CMP value and their relationship to unaligned access capabilities.

255-280: Verify architecture detection completeness.

The architecture detection appears comprehensive but should be verified against all supported platforms.

#!/bin/bash
# Search for architecture-specific code that might need OPTIMAL_CMP
rg -l "__x86_64__|__i386__|__aarch64__|__arm__|__powerpc" arch/

#!/bin/bash
# Check content of architecture-specific files for any additional architecture macros
echo "=== Content of x86_functions.h ==="
cat arch/x86/x86_functions.h

echo -e "\n=== Content of arm_features.c ==="
cat arch/arm/arm_features.c

# Search for any other architecture-related macros in the codebase
echo -e "\n=== Other architecture macros in codebase ==="
rg -n "(defined\(__[a-zA-Z0-9_]+\)|defined\(_M_[a-zA-Z0-9_]+\))" --type c --type h