Improved setting of OPTIMAL_CMP on ARM#1835
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Nitpick comments (2)
zbuild.h (2)
252-256: LGTM! Well-structured conditional optimization for AArch64The implementation correctly handles both unaligned access capabilities and Windows platform specifics. The OPTIMAL_CMP values are appropriately set based on the architecture's capabilities:
- 64 bytes when unaligned access is supported (optimal for modern AArch64)
- 8 bytes as a safe fallback when strict alignment is required
Consider documenting that on AArch64 platforms where strict alignment is enforced (OPTIMAL_CMP=8), the
-mstrict-aligncompiler flag should be used to ensure consistent behavior.
257-262: LGTM! Appropriate optimization values for ARM32The implementation mirrors the AArch64 structure with values adjusted for 32-bit architecture:
- 32 bytes when unaligned access is supported (optimal for ARM32)
- 8 bytes as a safe fallback when strict alignment is required
Consider documenting that on ARM32 platforms where strict alignment is enforced (OPTIMAL_CMP=8), the
-mno-unaligned-accesscompiler flag should be used to ensure consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zbuild.h(1 hunks)
🔇 Additional comments (1)
zbuild.h (1)
252-262: Verify OPTIMAL_CMP usage in alignment-sensitive operations
The changes to OPTIMAL_CMP values could affect alignment-sensitive operations throughout the codebase.
✅ Verification successful
Based on the search results, I can now make a definitive assessment. The OPTIMAL_CMP values are used carefully throughout the codebase with proper alignment handling:
- The code uses
zng_memcmp_2/4/8wrappers that handle unaligned access safely using memcpy - Memory operations in match_tpl.h check OPTIMAL_CMP values before performing operations
- There's a NO_UNALIGNED macro that forces OPTIMAL_CMP to 8 when unaligned access isn't safe
- The chunkset implementations across architectures handle alignment requirements appropriately
OPTIMAL_CMP changes are safely implemented with proper alignment handling
The codebase has robust alignment handling through:
- Safe memory access wrappers for unaligned comparisons
- Conditional compilation based on architecture capabilities
- Fallback mechanisms when unaligned access isn't supported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for alignment-sensitive operations and OPTIMAL_CMP usage
# Search for direct OPTIMAL_CMP usage
echo "=== Checking direct OPTIMAL_CMP usage ==="
rg "OPTIMAL_CMP" -B 2 -A 2
# Search for potential alignment-sensitive operations
echo "=== Checking for alignment-sensitive memory operations ==="
rg -e "memcmp|memcpy|memmove" -B 2 -A 2
# Search for alignment checks
echo "=== Checking for alignment validation code ==="
rg -e "aligned|alignment|unaligned" -B 2 -A 2
Length of output: 66498
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1835 +/- ##
========================================
Coverage 82.41% 82.41%
========================================
Files 136 136
Lines 11021 11021
Branches 2799 2799
========================================
Hits 9083 9083
- Misses 1239 1240 +1
+ Partials 699 698 -1 ☔ View full report in Codecov by Sentry. |
|
A relevant Ref: https://developer.arm.com/documentation/dui0774/l/Compiler-Command-line-Options/-munaligned-access---mno-unaligned-access This looks like a good improvement to me 👍 |
Following the suggestion in #1830 (comment), this PR explicitly sets
OPTIMAL_CMPto 8 on ARM when unaligned memory access is unavailable. I've also changed the feature checks to better cover non-GCC compilers.On ARM32,
-mno-unaligned-accesscan be used to force alignment-safe loads, while-mstrict-aligncan be used with AArch64. This should help when running benchmarks on platforms that normally have it enabled.Summary by CodeRabbit