Replace stdlib keccak with XKCP for ~1.4x speedup - #28
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR integrates the XKCP Keccak Code Package: adds CPU-optimized KeccakP-1600 backends (AVX‑512, AVX2, ARMv8‑A NEON, plain64), high-level sponge/hash C code, Zig FFI bindings, build wiring to select per-CPU sources, and a Zig benchmark CLI to exercise backends. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Benchmark CLI
participant Zig as Zig Keccak Module
participant XKCP as XKCP C API
participant Backend as CPU Backend
User->>Zig: run benchmark(hash data, backend)
activate Zig
alt backend == "xkcp"
Zig->>XKCP: KeccakWidth1600_Sponge / Keccak_HashInitialize/Update/Final
activate XKCP
XKCP->>Backend: permutation/absorb/extract (arch-optimized)
Backend-->>XKCP: processed blocks (hash state)
deactivate XKCP
XKCP-->>Zig: 32-byte hash
else backend == "stdlib"
Zig->>Zig: std.crypto.hash.sha3.Keccak256.hash(data)
Zig-->>Zig: 32-byte hash
end
Zig-->>User: return 32-byte hash
deactivate Zig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s (1)
454-704: Add CI coverage for AVX512/AVX2 backend execution paths.Given this backend is assembly-heavy and x86_64 Linux validation is still pending, a dedicated CI matrix for AVX2, AVX512, and fallback would reduce regression risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s` around lines 454 - 704, Add CI jobs to exercise the AVX512/AVX2/fallback assembly paths by adding a Linux x86_64 matrix (entries: baseline/fallback, avx2, avx512) that builds and runs the test suite with appropriate compiler/assembler flags; for avx2 set CFLAGS/ASFLAGS to include -mavx2 and for avx512 include -mavx512f/-mavx512dq and use a runner or Docker image on x86_64. Ensure each job builds the target that includes the KeccakP1600_AVX512_Permute_24rounds, KeccakP1600_AVX512_Permute_12rounds and KeccakP1600_AVX512_Permute_Nrounds symbols and runs the unit/integration tests that exercise those symbols (or a small smoke test invoking the permute functions). Add conditional skipping or feature-detection (grep /proc/cpuinfo or use gcc -march checks) so jobs fail fast when the host lacks required ISA support, and document the matrix entries in CI config.src/keccak.zig (1)
12-18: API design clarification: directKeccak256usage won't benefit from XKCP speedup.The
hash()function correctly uses the XKCP backend, butKeccak256is aliased toStdlibKeccak256for backward compatibility. Code that directly instantiatesKeccak256for incremental hashing will use the slower stdlib implementation.Consider adding a doc comment clarifying this tradeoff, or exposing
xkcp.Hasheras a recommended alternative for incremental hashing:📝 Proposed documentation enhancement
-/// For API compatibility: expose the stdlib type for code that uses the hasher directly. +/// For API compatibility: expose the stdlib type for code that uses the hasher directly. +/// Note: For best performance with incremental hashing, use `hashConcat()` or access +/// `xkcp.Hasher` directly. Direct `Keccak256` usage falls back to the stdlib implementation. pub const Keccak256 = StdlibKeccak256;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keccak.zig` around lines 12 - 18, The Keccak256 alias points to StdlibKeccak256 so callers that instantiate Keccak256 for incremental hashing won't get XKCP's faster implementation; update the API by either (a) adding a concise doc comment on the Keccak256 declaration explaining that hash() uses the XKCP backend and that incremental usage of Keccak256 will use the slower StdlibKeccak256, or (b) export the XKCP incremental hasher (e.g., expose xkcp.Hasher under a public name) and add a doc comment on Keccak256 pointing users to xkcp.Hasher for high-performance incremental hashing; reference the symbols Keccak256, StdlibKeccak256, hash(), xkcp, and xkcp.Hasher when making the changes.src/crypto/xkcp/high/KeccakHash.c (1)
17-18: Static analysis false positive:string.his a standard C header.The Clang error is due to the analyzer running without the system include paths. This is a standard C header that will be available when building with
link_libc = true.Note: The
string.hinclude appears unused in this file (nomemcpy,memset, etc. calls visible). It may be a remnant from the upstream XKCP source or included for consistency. Consider removing if build testing confirms it's unnecessary, but this is low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/crypto/xkcp/high/KeccakHash.c` around lines 17 - 18, Remove the unnecessary `#include` <string.h> from KeccakHash.c (it appears unused relative to the functions implemented in that file, e.g., those declared in KeccakHash.h); delete the include line and run full build/tests (with link_libc = true) to confirm nothing breaks, and if any string functions are actually needed later, re-add the include or add a brief comment explaining why it's retained.src/keccak_xkcp.zig (1)
40-45: Consider handling empty input explicitly.When
datais an empty slice,data.ptrbehavior in Zig is well-defined (returns a non-null but invalid pointer), but passing it to C code that may dereference it could be problematic depending on the XKCP implementation.While XKCP likely handles zero-length inputs correctly, consider an explicit check for safety:
🔧 Optional: Add explicit empty input handling
pub fn hash(data: []const u8) Hash { var result: Hash = undefined; + if (data.len == 0) { + // Handle empty input explicitly + const ret = KeccakWidth1600_Sponge(rate, capacity, `@as`([*]const u8, &[_]u8{}), 0, delimited_suffix, &result, 32); + std.debug.assert(ret == 0); + return result; + } const ret = KeccakWidth1600_Sponge(rate, capacity, data.ptr, data.len, delimited_suffix, &result, 32); std.debug.assert(ret == 0); return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keccak_xkcp.zig` around lines 40 - 45, The hash function should explicitly handle empty input before calling KeccakWidth1600_Sponge: detect when data.len == 0 and pass a safe null pointer (or short-circuit to produce the known zero-length hash) instead of blindly using data.ptr, then call KeccakWidth1600_Sponge with that safe pointer and the same parameters (rate, capacity, delimited_suffix, &result, 32) and assert ret == 0; update pub fn hash and reference KeccakWidth1600_Sponge, data.ptr, data.len, delimited_suffix, and result when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bench/keccak_bench_cli.zig`:
- Around line 21-34: The benchmark currently ignores invalid backend_str values
(only "xkcp" and "stdlib" handled) which yields silent success; update the
control flow in bench/keccak_bench_cli.zig so that after the existing if/else-if
branches you add an else branch that reports an error and exits non‑zero (e.g.,
print an error mentioning backend_str and valid options) so users are informed
when an unknown backend is passed—refer to backend_str, the eth.keccak.hash
branch and std.crypto.hash.sha3.Keccak256.hash branch to locate the decision
point and add the error handling there.
In `@bench/rust-keccak/src/lib.rs`:
- Around line 5-9: The FFI functions (rust_keccak_asm and rust_tiny_keccak)
currently create slices from raw pointers without null/validity checks and risk
undefined behavior and aliasing; fix by first validating pointers with
ptr::is_null() and input_len bounds, returning early (or doing nothing) on
invalid inputs, then only call slice::from_raw_parts/from_raw_parts_mut after
those checks; also validate the output pointer is non-null and that output
buffer is at least 32 bytes; to prevent aliasing between input and output,
detect overlap (compare pointer ranges) and if they overlap, copy the digest
into a local 32-byte temporary buffer (or use ptr::copy_nonoverlapping from a
non-overlapping source) before writing to the output pointer; apply the same
pattern to both rust_keccak_asm and rust_tiny_keccak.
In `@src/crypto/xkcp/armv8a/KeccakP-1600-armv8a-neon.s`:
- Around line 496-501: KeccakP1600_Permute_Nrounds can compute a pointer before
KeccakP1600_Permute_RoundConstants0 when nrounds > 24; clamp the rounds value
before doing the pointer math by bounding the rounds in x2 to a maximum of 24
(e.g., compute x2 = min(x1, `#24`) or otherwise limit the value used for lsl `#3`)
and then use that clamped x2 for lsl x3 and the subsequent sub against
KeccakP1600_Permute_RoundConstants0 so the pointer never moves before the table;
keep the rest of the flow (branch to KeccakP1600_Permute) unchanged and only
modify how x2/x3 are derived.
In `@src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s`:
- Around line 619-624: nrounds is not being bounded before computing the iota
table pointer (iotas_end - nrounds*8), allowing reads before iotas for
nrounds>24; clamp %esi (nrounds) to a maximum of 24 before the lea/shl/sub
sequence (i.e., insert a compare/cap: cmp $24,%esi; cmovae or use ja/ mov
$24,%esi) so the following shl $3,%rsi and sub %rsi,%r10 cannot underflow the
iotas pointer; keep the following labels/regs intact (iotas_end, %r10, %rsi/%rax
used for nrounds) and perform the clamp immediately after moving nrounds into
%rax/%rsi and before computing the pointer.
In `@src/crypto/xkcp/common/brg_endian.h`:
- Around line 133-139: The code currently forces PLATFORM_BYTE_ORDER to
IS_LITTLE_ENDIAN via the unconditional '#elif 1' in brg_endian.h, making the
fail-fast error unreachable and risking wrong behavior on big-endian targets;
replace that unconditional branch with a proper detection or explicit error:
change the '#elif 1' branch to test real compiler macros (for example use checks
against __BYTE_ORDER__ / __ORDER_LITTLE_ENDIAN__ or vendor macros already used
earlier in the file) and only define PLATFORM_BYTE_ORDER = IS_LITTLE_ENDIAN when
those macros prove the target is little-endian; otherwise leave the '#elif 0'
branch or trigger the '#error' so PLATFORM_BYTE_ORDER is not silently
assumed—ensure references to PLATFORM_BYTE_ORDER, IS_LITTLE_ENDIAN and
IS_BIG_ENDIAN remain consistent.
In `@src/keccak_xkcp.zig`:
- Around line 58-61: The update function may overflow when computing data.len *
8; modify Hasher.update to check for multiplication overflow on data.len before
calling Keccak_HashUpdate (e.g., use std.math.mulWithOverflow or
std.mem.checkedMul to compute bit_len = data.len * 8 and return/assert on
overflow), or cast to a larger integer type (u128/u64) before multiplying and
ensure the resulting bit_len fits the parameter type expected by
Keccak_HashUpdate; then pass the safe bit_len to Keccak_HashUpdate and keep the
existing assert on the return value.
- Around line 43-44: Replace the std.debug.assert-based checks for XKCP return
values with explicit error unions: change the free function hash to return !Hash
and check KeccakWidth1600_Sponge's return value, returning a new error (e.g.
error.HashFailed) when non-zero; similarly change Hasher.init(),
Hasher.update(), and Hasher.final() to return an error union (e.g. !void or a
specific error union) and map non-zero Keccak/HashReturn results to appropriate
errors instead of asserting, ensuring each function returns success only when
the underlying C call returns the expected success code.
---
Nitpick comments:
In `@src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s`:
- Around line 454-704: Add CI jobs to exercise the AVX512/AVX2/fallback assembly
paths by adding a Linux x86_64 matrix (entries: baseline/fallback, avx2, avx512)
that builds and runs the test suite with appropriate compiler/assembler flags;
for avx2 set CFLAGS/ASFLAGS to include -mavx2 and for avx512 include
-mavx512f/-mavx512dq and use a runner or Docker image on x86_64. Ensure each job
builds the target that includes the KeccakP1600_AVX512_Permute_24rounds,
KeccakP1600_AVX512_Permute_12rounds and KeccakP1600_AVX512_Permute_Nrounds
symbols and runs the unit/integration tests that exercise those symbols (or a
small smoke test invoking the permute functions). Add conditional skipping or
feature-detection (grep /proc/cpuinfo or use gcc -march checks) so jobs fail
fast when the host lacks required ISA support, and document the matrix entries
in CI config.
In `@src/crypto/xkcp/high/KeccakHash.c`:
- Around line 17-18: Remove the unnecessary `#include` <string.h> from
KeccakHash.c (it appears unused relative to the functions implemented in that
file, e.g., those declared in KeccakHash.h); delete the include line and run
full build/tests (with link_libc = true) to confirm nothing breaks, and if any
string functions are actually needed later, re-add the include or add a brief
comment explaining why it's retained.
In `@src/keccak_xkcp.zig`:
- Around line 40-45: The hash function should explicitly handle empty input
before calling KeccakWidth1600_Sponge: detect when data.len == 0 and pass a safe
null pointer (or short-circuit to produce the known zero-length hash) instead of
blindly using data.ptr, then call KeccakWidth1600_Sponge with that safe pointer
and the same parameters (rate, capacity, delimited_suffix, &result, 32) and
assert ret == 0; update pub fn hash and reference KeccakWidth1600_Sponge,
data.ptr, data.len, delimited_suffix, and result when making this change.
In `@src/keccak.zig`:
- Around line 12-18: The Keccak256 alias points to StdlibKeccak256 so callers
that instantiate Keccak256 for incremental hashing won't get XKCP's faster
implementation; update the API by either (a) adding a concise doc comment on the
Keccak256 declaration explaining that hash() uses the XKCP backend and that
incremental usage of Keccak256 will use the slower StdlibKeccak256, or (b)
export the XKCP incremental hasher (e.g., expose xkcp.Hasher under a public
name) and add a doc comment on Keccak256 pointing users to xkcp.Hasher for
high-performance incremental hashing; reference the symbols Keccak256,
StdlibKeccak256, hash(), xkcp, and xkcp.Hasher when making the changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bench/rust-keccak/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
bench/keccak_bench_cli.zigbench/rust-keccak/.gitignorebench/rust-keccak/Cargo.tomlbench/rust-keccak/src/lib.rsbench/rust-keccak/src/main.rsbuild.zigsrc/crypto/xkcp/armv8a/KeccakP-1600-SnP.hsrc/crypto/xkcp/armv8a/KeccakP-1600-armv8a-neon.ssrc/crypto/xkcp/avx2/KeccakP-1600-AVX2.hsrc/crypto/xkcp/avx2/KeccakP-1600-AVX2.ssrc/crypto/xkcp/avx2/KeccakP-1600-SnP.hsrc/crypto/xkcp/avx512/KeccakP-1600-AVX512.hsrc/crypto/xkcp/avx512/KeccakP-1600-AVX512.ssrc/crypto/xkcp/avx512/KeccakP-1600-SnP.hsrc/crypto/xkcp/common/SnP-Relaned.hsrc/crypto/xkcp/common/SnP-common.hsrc/crypto/xkcp/common/align.hsrc/crypto/xkcp/common/brg_endian.hsrc/crypto/xkcp/common/config.hsrc/crypto/xkcp/high/KeccakHash.csrc/crypto/xkcp/high/KeccakHash.hsrc/crypto/xkcp/high/KeccakSponge.csrc/crypto/xkcp/high/KeccakSponge.hsrc/crypto/xkcp/high/KeccakSponge.incsrc/crypto/xkcp/plain64/KeccakP-1600-64.macrossrc/crypto/xkcp/plain64/KeccakP-1600-SnP.hsrc/crypto/xkcp/plain64/KeccakP-1600-opt64.csrc/crypto/xkcp/plain64/KeccakP-1600-plain64.hsrc/crypto/xkcp/plain64/KeccakP-1600-unrolling.macrossrc/keccak.zigsrc/keccak_xkcp.zig
…all inputs Vendors the XKCP (eXtended Keccak Code Package) optimized C/assembly implementations and integrates them via Zig's build system with comptime CPU feature detection. Selects AVX2/AVX-512 assembly on x86_64 and optimized C on aarch64. Benchmarks show parity with Voltaire's Rust keccak-asm FFI approach while avoiding a Rust toolchain dependency. Includes hyperfine-compatible CLI benchmarks and a Rust keccak-asm comparison harness.
koko1123
force-pushed
the
keccak-xkcp-optimization
branch
from
March 3, 2026 01:44
dc39690 to
c396285
Compare
koko1123
force-pushed
the
keccak-xkcp-optimization
branch
from
March 3, 2026 01:50
c396285 to
c93597e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/crypto/xkcp/high/KeccakSponge.h`:
- Line 23: Replace the header's libc include: in KeccakSponge.h change the
`#include` <string.h> to `#include` <stddef.h> so only size_t is exposed from the
header, and then add an explicit `#include` <string.h> to KeccakSponge.c
(immediately after the existing includes around line 17) to satisfy the
implementation's use of memset in KeccakSponge.inc; update these includes
only—no other code changes.
In `@src/crypto/xkcp/plain64/KeccakP-1600-unrolling.macros`:
- Around line 294-305: The roundsN macro calculates i = 24 - (__nrounds) with no
validation, so KeccakP1600_plain64_Permute_Nrounds (which forwards its unsigned
int nr) can cause negative/invalid indices when nr > 24; add a bounds check in
KeccakP1600_plain64_Permute_Nrounds to ensure nr is within [0,24] before calling
roundsN(nr) (e.g., clamp or return/error on out-of-range), and if rejecting
out-of-range values, document/handle the error path so callers cannot pass
invalid nrounds to roundsN; keep the macro unchanged but ensure only validated
nrounds reach roundsN.
In `@src/keccak.zig`:
- Around line 28-34: The function hashConcat currently constructs xkcp.Hasher (a
C-FFI type) unguarded and will fail if invoked at comptime; add the same
comptime protection as hash() by checking `@inComptime`() at the start of
hashConcat and emitting a compile-time error (e.g., `@compileError` with a clear
message) when true, so callers get a clear diagnostic instead of a cryptic FFI
failure; reference the hashConcat function and the use of xkcp.Hasher in your
change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
bench/keccak_bench_cli.zigbuild.zigsrc/crypto/xkcp/armv8a/KeccakP-1600-SnP.hsrc/crypto/xkcp/armv8a/KeccakP-1600-armv8a-neon.ssrc/crypto/xkcp/avx2/KeccakP-1600-AVX2.hsrc/crypto/xkcp/avx2/KeccakP-1600-AVX2.ssrc/crypto/xkcp/avx2/KeccakP-1600-SnP.hsrc/crypto/xkcp/avx512/KeccakP-1600-AVX512.hsrc/crypto/xkcp/avx512/KeccakP-1600-AVX512.ssrc/crypto/xkcp/avx512/KeccakP-1600-SnP.hsrc/crypto/xkcp/common/SnP-Relaned.hsrc/crypto/xkcp/common/SnP-common.hsrc/crypto/xkcp/common/align.hsrc/crypto/xkcp/common/brg_endian.hsrc/crypto/xkcp/common/config.hsrc/crypto/xkcp/high/KeccakHash.csrc/crypto/xkcp/high/KeccakHash.hsrc/crypto/xkcp/high/KeccakSponge.csrc/crypto/xkcp/high/KeccakSponge.hsrc/crypto/xkcp/high/KeccakSponge.incsrc/crypto/xkcp/plain64/KeccakP-1600-64.macrossrc/crypto/xkcp/plain64/KeccakP-1600-SnP.hsrc/crypto/xkcp/plain64/KeccakP-1600-opt64.csrc/crypto/xkcp/plain64/KeccakP-1600-plain64.hsrc/crypto/xkcp/plain64/KeccakP-1600-unrolling.macrossrc/keccak.zigsrc/keccak_xkcp.zig
🚧 Files skipped from review as they are similar to previous changes (9)
- src/keccak_xkcp.zig
- src/crypto/xkcp/high/KeccakSponge.inc
- src/crypto/xkcp/common/SnP-Relaned.h
- src/crypto/xkcp/common/config.h
- src/crypto/xkcp/plain64/KeccakP-1600-SnP.h
- src/crypto/xkcp/armv8a/KeccakP-1600-armv8a-neon.s
- src/crypto/xkcp/common/SnP-common.h
- src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s
- src/crypto/xkcp/avx2/KeccakP-1600-SnP.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
build.zig (1)
119-149: Add CI coverage for backend-selection branches.Given this function now routes multiple architecture/CPU paths, add matrix builds/tests for at least: baseline x86_64, x86_64+AVX2, x86_64+AVX-512F, and aarch64. This will catch selection regressions early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.zig` around lines 119 - 149, Add CI matrix builds to exercise the new backend-selection branches by adding jobs that run the build/test for arch detection and CPU feature gating used in build.zig (variables/functions to target: target.result.cpu.arch, target.result.cpu.features, features.isEnabled, module.addAssemblyFile, module.addCSourceFile). Create matrix entries for: baseline x86_64 (no extra features), x86_64 with AVX2 enabled, x86_64 with AVX-512F enabled, and aarch64; ensure runners use compilers/flags that expose those features (or set env/clang flags to simulate feature bits) and run the same build command from CI so module.addIncludePath and the plain64/avx2/avx512 branches are exercised. Ensure each job fails the workflow if its build selection path errors so selection regressions are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.zig`:
- Around line 145-149: The fallback that adds KeccakP-1600-opt64.c via
module.addCSourceFile lacks the plain64 include directory; update the same
fallback block that calls module.addCSourceFile({ .file =
b.path("src/crypto/xkcp/plain64/KeccakP-1600-opt64.c"), .flags = c_flags }) to
also add the include directory for the plain64 headers (e.g. call
module.addIncludeDir or equivalent with b.path("src/crypto/xkcp/plain64") so the
high-level XKCP C sources can resolve their headers in x86_64 non-AVX builds).
- Around line 134-143: The AVX assembly backends are being added unconditionally
when features.isEnabled(avx512f) or features.isEnabled(avx2) even for non-ELF
x86_64 targets; update the logic in the block using
features.isEnabled(avx512f)/features.isEnabled(avx2) to also check
target.result.ofmt == .elf before calling module.addAssemblyFile for
KeccakP-1600-AVX512.s or KeccakP-1600-AVX2.s, and if the object format is not
.elf fall back to using the plain64 C backend (e.g. ensure the existing plain64
include and KeccakP-1600-opt64.c are used instead of adding the .s assembly
file) so non-ELF x86_64 (macOS/Windows) build targets do not import ELF-specific
assembly.
---
Nitpick comments:
In `@build.zig`:
- Around line 119-149: Add CI matrix builds to exercise the new
backend-selection branches by adding jobs that run the build/test for arch
detection and CPU feature gating used in build.zig (variables/functions to
target: target.result.cpu.arch, target.result.cpu.features, features.isEnabled,
module.addAssemblyFile, module.addCSourceFile). Create matrix entries for:
baseline x86_64 (no extra features), x86_64 with AVX2 enabled, x86_64 with
AVX-512F enabled, and aarch64; ensure runners use compilers/flags that expose
those features (or set env/clang flags to simulate feature bits) and run the
same build command from CI so module.addIncludePath and the plain64/avx2/avx512
branches are exercised. Ensure each job fails the workflow if its build
selection path errors so selection regressions are detected.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
build.zig (1)
134-143:⚠️ Potential issue | 🟠 MajorGate AVX assembly backends to ELF targets only.
Line [134] through Line [143] still selects AVX
.sbackends only by CPU feature bits. That can break non-ELF x86_64 targets (e.g., Mach-O/COFF) when ELF/GAS-specific directives are present. Please add an object-format gate and fall back to existing plain64 C otherwise.Proposed fix
} else if (arch == .x86_64) { const features = target.result.cpu.features; const avx512f = `@intFromEnum`(std.Target.x86.Feature.avx512f); const avx2 = `@intFromEnum`(std.Target.x86.Feature.avx2); + const is_elf = target.result.ofmt == .elf; - if (features.isEnabled(avx512f)) { + if (is_elf and features.isEnabled(avx512f)) { // AVX512 SnP header must come before plain64 to shadow KeccakP-1600-SnP.h module.addIncludePath(b.path("src/crypto/xkcp/avx512")); module.addIncludePath(b.path("src/crypto/xkcp/plain64")); module.addAssemblyFile(b.path("src/crypto/xkcp/avx512/KeccakP-1600-AVX512.s")); - } else if (features.isEnabled(avx2)) { + } else if (is_elf and features.isEnabled(avx2)) { // AVX2 SnP header must come before plain64 to shadow KeccakP-1600-SnP.h module.addIncludePath(b.path("src/crypto/xkcp/avx2")); module.addIncludePath(b.path("src/crypto/xkcp/plain64")); module.addAssemblyFile(b.path("src/crypto/xkcp/avx2/KeccakP-1600-AVX2.s")); } else { module.addIncludePath(b.path("src/crypto/xkcp/plain64")); module.addCSourceFile(.{ .file = b.path("src/crypto/xkcp/plain64/KeccakP-1600-opt64.c"), .flags = c_flags, }); }#!/bin/bash set -euo pipefail echo "== x86_64 backend selection block ==" sed -n '129,151p' build.zig echo echo "== Search for object-format guard usage ==" rg -n -C2 'target\.result\.ofmt|features\.isEnabled\(avx512f\)|features\.isEnabled\(avx2\)' build.zig echo echo "== Check AVX assembly for ELF-specific directives ==" for f in $(fd 'KeccakP-1600-AVX(2|512)\.s$' src/crypto/xkcp); do echo "-- $f" rg -n '^\s*\.type\b|^\s*\.size\b|\.note\.GNU-stack|@function' "$f" || true doneExpected confirmation: no
ofmt == .elfguard inbuild.zigAVX branches, while AVX assembly files contain ELF-oriented directives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.zig` around lines 134 - 143, The AVX assembly backends are currently selected solely by CPU feature checks (features.isEnabled(avx512f) / features.isEnabled(avx2') via the block that calls module.addAssemblyFile), which can pull ELF/GAS-specific .s files into non-ELF targets; update that selection to also check the target object format (target.result.ofmt) and only addAssemblyFile for the AVX .s files when target.result.ofmt == .elf, otherwise fall back to adding only the plain64 include path (module.addIncludePath(b.path("src/crypto/xkcp/plain64"))) and do not call module.addAssemblyFile; make this change around the existing features.isEnabled(avx512f) and features.isEnabled(avx2) branches so the code paths using module.addIncludePath and module.addAssemblyFile are gated by the object-format check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@build.zig`:
- Around line 134-143: The AVX assembly backends are currently selected solely
by CPU feature checks (features.isEnabled(avx512f) / features.isEnabled(avx2')
via the block that calls module.addAssemblyFile), which can pull
ELF/GAS-specific .s files into non-ELF targets; update that selection to also
check the target object format (target.result.ofmt) and only addAssemblyFile for
the AVX .s files when target.result.ofmt == .elf, otherwise fall back to adding
only the plain64 include path
(module.addIncludePath(b.path("src/crypto/xkcp/plain64"))) and do not call
module.addAssemblyFile; make this change around the existing
features.isEnabled(avx512f) and features.isEnabled(avx2) branches so the code
paths using module.addIncludePath and module.addAssemblyFile are gated by the
object-format check.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Benchmark Results (Apple Silicon)
Hyperfine comparison vs Voltaire's keccak-asm (Rust FFI)
32 bytes (most common Ethereum hash size) × 1M iterations:
256 bytes × 1M iterations:
Achieves parity with Voltaire's Rust keccak-asm without requiring a Rust toolchain dependency.
Platform backends
Test plan
zig build testpasses (all 12 keccak tests + full suite)zig build benchshows improvementSummary by CodeRabbit
New Features
Performance
Chores