simd: ChaCha20 hardware-accel + U32x16 ARX lane + wasm parity CI (crypto-SIMD arc)#240
Merged
AdaWorldAPI merged 11 commits intoJul 12, 2026
Merged
Conversation
…el primitive The load-bearing piece of the ogar-crypto-SIMD unification: the ChaCha20 block function as an ndarray::simd primitive, so the Ada `encryption` crate (XChaCha20-Poly1305) can draw its hot-path keystream from the ONE SIMD polyfill (server AVX-512 / browser wasm128) per the workspace "all SIMD from ndarray::simd" invariant, instead of a per-consumer copy. Why safe to hand-vectorize (unlike AES): ChaCha20 is ARX — every op is a 32-bit wrapping_add / xor / fixed rotate_left, with NO secret-dependent branch or memory index, so the block function is constant-time by construction and a straight-line SIMD form preserves that (the reason RFC 8439 chose it for constant-time software). This rev lands the SCALAR reference + the correctness anchor: - `chacha20_block(&[u32;16]) -> [u8;64]` (RFC 8439 §2.3.1, 20-round double-round). - `chacha20_state(key, counter, nonce)` state assembler. - RFC 8439 §2.3.2 block KAT + §2.1.1 quarter-round KAT — both green. The AVX-512 (4-way) / wasm128 / NEON backends are the next increment behind the SAME signature, each a drop-in that MUST reproduce this KAT (parity gate) — "reference + KAT, then vectorize with parity", the W1a discipline. clippy clean.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_651a9580-daea-4465-984b-af13e6d206ec) |
Contributor
📝 WalkthroughWalkthroughAdds a public ChangesSIMD crypto keystream
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant chacha20_keystream
participant AVX512Backend
participant WasmSIMDBackend
participant ScalarReference
participant RustCryptoParityTest
Caller->>chacha20_keystream: provide key, counter, nonce, and output
chacha20_keystream->>AVX512Backend: process AVX-512 batches when enabled
chacha20_keystream->>WasmSIMDBackend: process wasm SIMD batches when enabled
chacha20_keystream->>ScalarReference: process fallback blocks and tails
AVX512Backend-->>Caller: serialized keystream blocks
WasmSIMDBackend-->>Caller: serialized keystream blocks
ScalarReference-->>Caller: serialized keystream blocks
RustCryptoParityTest->>chacha20_keystream: generate deterministic keystream
RustCryptoParityTest->>RustCryptoParityTest: compare with RustCrypto oracle
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7923c177cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // keystream from. Scalar reference + RFC 8439 KAT land first (arch-agnostic, | ||
| // no cfg gate); the AVX-512 / wasm128 / NEON vectorized backends slot in behind | ||
| // the same signature, parity-checked against the KAT (W1a contract). | ||
| pub mod simd_crypto; |
There was a problem hiding this comment.
Registering this only as ndarray::simd_crypto leaves the new primitive outside the canonical ndarray::simd::* import path; I checked src/simd.rs, which re-exports other substrate primitives such as simd_ops, but it never re-exports chacha20_block or chacha20_state. Consumers following the documented W1a invariant in this change (use ndarray::simd::chacha20_block) will fail to compile until these functions are surfaced through the simd facade.
Useful? React with 👍 / 👎.
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/simd_crypto.rs`:
- Around line 76-97: Add rustdoc # Examples sections to both public functions:
chacha20_block at src/simd_crypto.rs lines 76-97 should demonstrate assembling a
state with chacha20_state and calling chacha20_block, while chacha20_state at
lines 104-115 should demonstrate construction from a key, counter, and nonce.
Ensure both examples are valid compiled doctests and preserve the existing
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64ffc166-5882-44fb-8e61-f50352732d38
📒 Files selected for processing (2)
src/lib.rssrc/simd_crypto.rs
Comment on lines
+76
to
+97
| pub fn chacha20_block(state: &[u32; 16]) -> [u8; 64] { | ||
| let mut w = *state; | ||
| // 10 double-rounds = 20 rounds. | ||
| for _ in 0..10 { | ||
| // Column round. | ||
| quarter_round(&mut w, 0, 4, 8, 12); | ||
| quarter_round(&mut w, 1, 5, 9, 13); | ||
| quarter_round(&mut w, 2, 6, 10, 14); | ||
| quarter_round(&mut w, 3, 7, 11, 15); | ||
| // Diagonal round. | ||
| quarter_round(&mut w, 0, 5, 10, 15); | ||
| quarter_round(&mut w, 1, 6, 11, 12); | ||
| quarter_round(&mut w, 2, 7, 8, 13); | ||
| quarter_round(&mut w, 3, 4, 9, 14); | ||
| } | ||
| let mut out = [0u8; 64]; | ||
| for (i, word) in w.iter().enumerate() { | ||
| let sum = word.wrapping_add(state[i]); | ||
| out[i * 4..i * 4 + 4].copy_from_slice(&sum.to_le_bytes()); | ||
| } | ||
| out | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Public ChaCha20 primitives are missing rustdoc examples. Both public functions have descriptive /// docs but no example code block; the guideline requires examples on all public APIs (they also double as compiled doctests).
src/simd_crypto.rs#L76-L97: add a# Examplessection tochacha20_block(e.g. assemble a state viachacha20_stateand showlet ks = chacha20_block(&state);).src/simd_crypto.rs#L104-L115: add a# Examplessection tochacha20_stateshowing construction from a key/counter/nonce.
As per coding guidelines: "All public APIs (public functions and methods) must have /// doc comments with examples".
📍 Affects 1 file
src/simd_crypto.rs#L76-L97(this comment)src/simd_crypto.rs#L104-L115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_crypto.rs` around lines 76 - 97, Add rustdoc # Examples sections to
both public functions: chacha20_block at src/simd_crypto.rs lines 76-97 should
demonstrate assembling a state with chacha20_state and calling chacha20_block,
while chacha20_state at lines 104-115 should demonstrate construction from a
key, counter, and nonce. Ensure both examples are valid compiled doctests and
preserve the existing documentation.
Source: Coding guidelines
Add the AVX-512 backend of the ChaCha20 ARX keystream and the runtime
dispatcher the `encryption` AEAD hot path will draw from:
- `chacha20_keystream(key, counter, nonce, &mut [[u8;64]])` — fills N
keystream blocks, dispatching the AVX-512 backend in 16-block strides
when `avx512f` is detected at runtime, scalar reference for the ragged
tail and every non-AVX-512 target.
- `chacha20_block16_avx512` — 16 consecutive blocks in parallel, word-
sliced ("vertical") layout so every round is pure SIMD add/xor/rotate
(`_mm512_add_epi32` / `_mm512_xor_si512` / `_mm512_rol_epi32`), no
cross-lane shuffles; only the final LE serialization transposes.
- Re-export `chacha20_{block,keystream,state}` through `ndarray::simd::*`
so consumers pull from the polyfill surface per the W1a contract.
Correctness gate (mandatory for SIMD crypto): two new tests pin the
dispatcher byte-for-byte to the scalar RFC 8439 reference —
`chacha20_keystream_dispatch_parity_scalar` (n=40 spans two AVX-512
strides + an 8-block scalar tail) and `chacha20_keystream_dispatch_
matches_rfc8439_kat` (drives the RFC §2.3.2 vector THROUGH the AVX-512
path when the host CPU has it). ChaCha20 is ARX (no secret-dependent
branch/index) so the vectorization is constant-time by construction.
4 tests green, clippy -D warnings clean, fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
The mandatory second oracle before any consumer draws keystream from the accelerated primitive: prove `ndarray::simd::chacha20_keystream` is byte-for-byte identical to RustCrypto's vetted `chacha20::ChaCha20` (the IETF RFC 8439 variant), not just to our own scalar reference. - tests/chacha20_rustcrypto_parity.rs — deterministic SplitMix64 vectors across sub-stride / exact-16-stride / multi-stride / every ragged tail, 8 random (key,nonce,counter) draws each; plus a single-block check and an oracle-sanity KAT (confirms the RustCrypto side is the 96-bit-nonce IETF variant, so parity means something). `chacha20 = "0.9"` added as a dev-dependency ONLY (matches the version chacha20poly1305 0.10 already pulls; no change to the consumer dependency graph). - This is the "reference + KAT, then vectorize with parity" discipline extended to an independent implementation. The `encryption` AEAD stays on RustCrypto's authenticated XChaCha20-Poly1305 construction — the keystream primitive is now trusted, but the Poly1305 + HChaCha20 composition is NOT reimplemented (never roll your own AEAD). Audit follow-ups (sentinel-qa PASS, two P2 doc nits): - Backend-ladder table: AVX-512 marked DONE (was "(next)"). - chacha20_keystream doc: cite the real parity test name. lib simd_crypto 4/4 + integration parity 3/3 green, clippy -D clean, fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Add the WebAssembly SIMD backend of the ChaCha20 keystream — the browser
half of the accelerated crypto spine (server = AVX-512, browser = wasm128):
- `chacha20_block4_wasm` — 4 consecutive blocks in parallel using the wasm
`v128` register as four `u32x4` lanes, same word-sliced ("vertical")
layout as the AVX-512 backend so every round is a pure `u32x4`
add/xor/rotate. Uses NO `unsafe`: the wasm arithmetic + `u32x4_extract_lane`
intrinsics are safe under the `simd128` target feature (only raw
v128_load/store would need unsafe, and output is read via extract_lane).
- Dispatcher `chacha20_keystream` gains a
`cfg(all(target_arch="wasm32", target_feature="simd128"))` arm: 4-block
strides + scalar tail. `simd128` is a compile-time wasm feature (not
runtime-detected), so the arm is present iff built for it.
Verification (crypto is security-sensitive — verified before commit):
- Compiles clean for `wasm32-unknown-unknown` with `-C target-feature=+simd128`
(isolated compile-check; the intrinsic API is correct).
- `chacha20_wasm_logic_matches_scalar` — a native scalar mirror emulating
each `v128` as `[u32;4]` runs the IDENTICAL op sequence and is asserted
byte-equal to the scalar reference for the 4 covered counters, validating
the wasm arm's LOGIC (round schedule / counter lanes / transpose) on a
target that can't run wasm. The same word-sliced algorithm is byte-parity-
proven 16-wide by the AVX-512 tests. Runtime wasm parity (wasm-bindgen-test
in node) is the final gate, deferred pending a wasm test harness — the
full-crate wasm build is currently blocked by an unrelated getrandom
wasm_js config gap, not by this code.
lib simd_crypto 5/5 green, clippy -D clean, fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Record the security decision so no future session naively "wires the AEAD hot path through ndarray::simd::chacha20_keystream" and ends up hand- composing HChaCha20 + Poly1305 + the AEAD framing (the roll-your-own-AEAD footgun). The accelerated keystream primitive is trusted and available for raw-stream use sites that already own a vetted MAC; this authenticated AEAD is not one of them and keeps the vetted RustCrypto XChaCha20Poly1305. Doc-only; encryption crate 23/23 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
U32x16 already carried Add + BitXor across all tiers; rotate_left (the third ChaCha20/BLAKE ARX primitive) was the one missing op — the only `rol` in the tree was a raw intrinsic inside simd_crypto.rs. Add it to U32x16 as a first- class method on every tier, so `ndarray::simd::U32x16` exposes the full quarter-round vocabulary (the matryoshka chacha20 backend rides this lane; consumers pull it from `ndarray::simd::*`, never hpc): - avx512 : native `_mm512_rolv_epi32` (VPROLVD, single instruction) — executed + parity-green here. - avx2 : per-lane `u32::rotate_left` loop (this tier's U32x16 is the [u32;16] polyfill; native 2×__m256i is the deferred TD-SIMD-3 lowering). - scalar : per-lane `u32::rotate_left` — the completeness tier + bit-exact reference; also the type neon/wasm re-export, so both inherit rotate_left today. - nightly : core::simd shift-or with the n%32 guard (a >>32 on a u32 lane is UB), matching u32::rotate_left wrap-by-32 semantics. Parity test `u32x16_arx_ops_match_scalar` locks add/xor/rotate_left bit- identical to per-lane u32 (rotate consts 16/12/8/7 + edges 0/1/24/31); it uses `super::*` so it gates whichever tier the build compiled — avx512 here. Verification: avx512 tier executed (parity green), clippy -D clean, fmt clean. The scalar/avx2 rotate is `u32::rotate_left` (== the test reference, trivially correct); nightly is inspection-only (workspace is Rust-1.95 stable, no nightly toolchain). Native wasm/neon U32x16 = [U32x4;4] (real browser/ARM SIMD, vs today's scalar re-export) is the next increment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_80b32a08-3b46-4a21-ab41-c3a24800453c) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/simd_avx2.rs`:
- Around line 1545-1561: Add a `# Examples` section to the rustdoc for the
public `U32x16::rotate_left` method, demonstrating its lane-wise rotation
behavior. Keep the existing documentation and implementation unchanged.
In `@src/simd_avx512.rs`:
- Around line 1435-1447: Add a `# Examples` section to the `U32x16::rotate_left`
documentation, showing representative usage and the expected lane-wise rotation
result. Keep the existing safety comment and implementation unchanged.
In `@src/simd_crypto.rs`:
- Around line 117-131: Add a Rustdoc # Examples section to the public
chacha20_keystream function documentation, showing initialization of a key,
nonce, and output blocks, invocation of the function, and a basic assertion that
generated keystream data is nonzero. Keep the existing API behavior and
documentation unchanged.
In `@src/simd_nightly/u_word_types.rs`:
- Around line 367-380: Add a Rustdoc # Examples section to the public
U32x16::rotate_left method, demonstrating lane-wise rotation and matching
u32::rotate_left semantics. Keep the existing implementation and prose
documentation unchanged.
In `@src/simd_scalar.rs`:
- Around line 1031-1045: Add a `# Examples` documentation section to each public
`rotate_left` implementation on `U32x16` across the scalar, AVX2, AVX512, and
nightly word-type modules. Use a consistent doctest demonstrating construction,
rotation, conversion with `to_array`, and an assertion of the expected lane
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0e10a8f-ac03-4e24-9ca3-8700c3fc3eba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/encryption/src/aead.rssrc/simd.rssrc/simd_avx2.rssrc/simd_avx512.rssrc/simd_crypto.rssrc/simd_nightly/u_word_types.rssrc/simd_scalar.rstests/chacha20_rustcrypto_parity.rs
✅ Files skipped from review due to trivial changes (1)
- crates/encryption/src/aead.rs
Comment on lines
+1545
to
+1561
| impl U32x16 { | ||
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | ||
| /// `u32::rotate_left`), completing `Add` + `BitXor` for ChaCha20/BLAKE. | ||
| /// This arm's `U32x16` is the `[u32; 16]` polyfill (native `2× __m256i` is | ||
| /// the deferred TD-SIMD-3 lowering), so the rotate is a per-lane | ||
| /// `u32::rotate_left` loop — bit-identical to the scalar tier and to the | ||
| /// native `VPROLVD` path, the shared parity reference. | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| let mut out = [0u32; 16]; | ||
| for i in 0..16 { | ||
| out[i] = self.0[i].rotate_left(n); | ||
| } | ||
| Self(out) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
U32x16::rotate_left is missing a # Examples doc section.
Same issue as src/simd_scalar.rs — this new public method has /// docs but no # Examples section. As per coding guidelines: "All public APIs (public functions and methods) must have /// doc comments with examples".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_avx2.rs` around lines 1545 - 1561, Add a `# Examples` section to the
rustdoc for the public `U32x16::rotate_left` method, demonstrating its lane-wise
rotation behavior. Keep the existing documentation and implementation unchanged.
Source: Coding guidelines
Comment on lines
+1435
to
+1447
|
|
||
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | ||
| /// `u32::rotate_left`), the third ChaCha20/BLAKE-family primitive alongside | ||
| /// `Add` + `BitXor`. Single `VPROLVD` (AVX-512F variable rotate). The rotate | ||
| /// amount in ARX ciphers is a public constant, never secret, so a runtime | ||
| /// `n` carries no timing concern; LLVM folds a constant `n` to the immediate | ||
| /// `VPROLD` form. | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| // SAFETY: `_mm512_rolv_epi32` is AVX-512F; this whole module is gated to | ||
| // an AVX-512F build. Reads only `self`, returns an owned register. | ||
| Self(unsafe { _mm512_rolv_epi32(self.0, _mm512_set1_epi32(n as i32)) }) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
U32x16::rotate_left is missing a # Examples doc section.
Same issue as src/simd_scalar.rs — this new public method has /// docs but no # Examples section. The // SAFETY: comment on the unsafe block is correctly present.
As per coding guidelines: "All public APIs (public functions and methods) must have /// doc comments with examples".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_avx512.rs` around lines 1435 - 1447, Add a `# Examples` section to
the `U32x16::rotate_left` documentation, showing representative usage and the
expected lane-wise rotation result. Keep the existing safety comment and
implementation unchanged.
Source: Coding guidelines
Comment on lines
+117
to
+131
| /// Fill `out` with successive ChaCha20 keystream blocks: block `b` uses block | ||
| /// counter `counter.wrapping_add(b)`, key and nonce fixed (RFC 8439 CTR mode). | ||
| /// | ||
| /// This is the accelerated entry point the `encryption` AEAD hot path draws its | ||
| /// keystream from. It runs the **AVX-512 backend in 16-block strides** when | ||
| /// `avx512f` is detected at runtime, and the scalar [`chacha20_block`] reference | ||
| /// for the ragged tail and on every non-x86 / non-AVX-512 target. Every backend | ||
| /// is a drop-in for the scalar reference — see the module KAT and the | ||
| /// `chacha20_keystream_avx512_parity` test that pins byte-for-byte equality. | ||
| /// | ||
| /// **Constant-time:** all backends are straight-line ARX (add / xor / rotate), | ||
| /// no secret-dependent control flow or memory indexing. Backend equivalence is | ||
| /// pinned by `chacha20_keystream_dispatch_parity_scalar` (byte-for-byte vs the | ||
| /// scalar reference across two AVX-512 strides + a scalar tail). | ||
| pub fn chacha20_keystream(key: &[u8; 32], counter: u32, nonce: &[u8; 12], out: &mut [[u8; 64]]) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
chacha20_keystream is missing a # Examples doc section.
This public function has /// documentation but no # Examples section, which the coding guidelines require for all public APIs. The prior review flagged chacha20_block and chacha20_state for the same omission; this new public function was not covered.
As per coding guidelines: "All public APIs (public functions and methods) must have /// doc comments with examples".
📝 Add a `# Examples` section
/// `chacha20_keystream_dispatch_parity_scalar` (byte-for-byte vs the
/// scalar reference across two AVX-512 strides + a scalar tail).
+///
+/// # Examples
+///
+/// ```
+/// use ndarray::simd::{chacha20_keystream, chacha20_state};
+///
+/// let key = [0u8; 32];
+/// let nonce = [0u8; 12];
+/// let mut blocks = vec![[0u8; 64]; 2];
+/// chacha20_keystream(&key, 0, &nonce, &mut blocks);
+/// assert!(blocks.iter().all(|b| b.iter().any(|&byte| byte != 0)));
+/// ```
pub fn chacha20_keystream(key: &[u8; 32], counter: u32, nonce: &[u8; 12], out: &mut [[u8; 64]]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Fill `out` with successive ChaCha20 keystream blocks: block `b` uses block | |
| /// counter `counter.wrapping_add(b)`, key and nonce fixed (RFC 8439 CTR mode). | |
| /// | |
| /// This is the accelerated entry point the `encryption` AEAD hot path draws its | |
| /// keystream from. It runs the **AVX-512 backend in 16-block strides** when | |
| /// `avx512f` is detected at runtime, and the scalar [`chacha20_block`] reference | |
| /// for the ragged tail and on every non-x86 / non-AVX-512 target. Every backend | |
| /// is a drop-in for the scalar reference — see the module KAT and the | |
| /// `chacha20_keystream_avx512_parity` test that pins byte-for-byte equality. | |
| /// | |
| /// **Constant-time:** all backends are straight-line ARX (add / xor / rotate), | |
| /// no secret-dependent control flow or memory indexing. Backend equivalence is | |
| /// pinned by `chacha20_keystream_dispatch_parity_scalar` (byte-for-byte vs the | |
| /// scalar reference across two AVX-512 strides + a scalar tail). | |
| pub fn chacha20_keystream(key: &[u8; 32], counter: u32, nonce: &[u8; 12], out: &mut [[u8; 64]]) { | |
| /// Fill `out` with successive ChaCha20 keystream blocks: block `b` uses block | |
| /// counter `counter.wrapping_add(b)`, key and nonce fixed (RFC 8439 CTR mode). | |
| /// | |
| /// This is the accelerated entry point the `encryption` AEAD hot path draws its | |
| /// keystream from. It runs the **AVX-512 backend in 16-block strides** when | |
| /// `avx512f` is detected at runtime, and the scalar [`chacha20_block`] reference | |
| /// for the ragged tail and on every non-x86 / non-AVX-512 target. Every backend | |
| /// is a drop-in for the scalar reference — see the module KAT and the | |
| /// `chacha20_keystream_avx512_parity` test that pins byte-for-byte equality. | |
| /// | |
| /// **Constant-time:** all backends are straight-line ARX (add / xor / rotate), | |
| /// no secret-dependent control flow or memory indexing. Backend equivalence is | |
| /// pinned by `chacha20_keystream_dispatch_parity_scalar` (byte-for-byte vs the | |
| /// scalar reference across two AVX-512 strides + a scalar tail). | |
| /// | |
| /// # Examples | |
| /// | |
| /// |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_crypto.rs` around lines 117 - 131, Add a Rustdoc # Examples section
to the public chacha20_keystream function documentation, showing initialization
of a key, nonce, and output blocks, invocation of the function, and a basic
assertion that generated keystream data is nonzero. Keep the existing API
behavior and documentation unchanged.
Source: Coding guidelines
Comment on lines
+367
to
+380
|
|
||
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | ||
| /// `u32::rotate_left`), completing `Add` + `BitXor` for ChaCha20/BLAKE. | ||
| /// `core::simd` has no bit-rotate, so this is the shift-or composition with | ||
| /// the `n % 32 == 0` guard (a `>> 32` on a `u32` lane is UB), matching | ||
| /// `u32::rotate_left`'s wrap-by-32 semantics. | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| let n = n % 32; | ||
| if n == 0 { | ||
| return self; | ||
| } | ||
| Self((self.0 << u32x16::splat(n)) | (self.0 >> u32x16::splat(32 - n))) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check if simd_nightly module is feature-gated
rg -n 'mod simd_nightly' src/ --type rust -C2
# 2. Check for NEON rotate_left implementation
rg -n 'rotate_left' src/simd_neon.rs 2>/dev/null || echo "No simd_neon.rs found"
fd -t f 'simd_neon' src/ 2>/dev/null || echo "No NEON SIMD file found"
# 3. Check if chacha20_block and chacha20_state now have # Examples (past review compliance)
rg -n '# Examples' src/simd_crypto.rs || echo "No # Examples sections found in simd_crypto.rs"
# 4. Check feature gates around simd_nightly
rg -n 'cfg.*nightly\|feature.*nightly' src/lib.rs src/simd.rs --type rust -C1Repository: AdaWorldAPI/ndarray
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the method and surrounding docs in the reported file
sed -n '340,390p' src/simd_nightly/u_word_types.rs | cat -n
# Check whether this file already contains any Examples sections
rg -n '# Examples' src/simd_nightly/u_word_types.rs || true
# Inspect the module declaration for nightly gating context
sed -n '248,268p' src/lib.rs | cat -nRepository: AdaWorldAPI/ndarray
Length of output: 3322
Add a # Examples section to U32x16::rotate_left. This public method only has prose docs; the API guideline requires examples on public functions and methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_nightly/u_word_types.rs` around lines 367 - 380, Add a Rustdoc #
Examples section to the public U32x16::rotate_left method, demonstrating
lane-wise rotation and matching u32::rotate_left semantics. Keep the existing
implementation and prose documentation unchanged.
Source: Coding guidelines
Comment on lines
+1031
to
+1045
| impl U32x16 { | ||
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | ||
| /// `u32::rotate_left`). The completeness tier: a full, correct | ||
| /// implementation held to the same bar as the SIMD tiers, and the bit-exact | ||
| /// reference they are parity-checked against. Delegates to `u32::rotate_left` | ||
| /// per lane (`rotate_left(0)` and `rotate_left(32)` both no-op, matching std). | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| let mut out = [0u32; 16]; | ||
| for i in 0..16 { | ||
| out[i] = self.0[i].rotate_left(n); | ||
| } | ||
| Self(out) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
U32x16::rotate_left is missing a # Examples doc section.
This new public method has /// documentation but no # Examples section. The same omission applies to the rotate_left implementations in src/simd_avx2.rs, src/simd_avx512.rs, and src/simd_nightly/u_word_types.rs.
As per coding guidelines: "All public APIs (public functions and methods) must have /// doc comments with examples".
📝 Add a `# Examples` section
/// per lane (`rotate_left(0)` and `rotate_left(32)` both no-op, matching std).
+///
+/// # Examples
+///
+/// ```
+/// use ndarray::simd::U32x16;
+///
+/// let v = U32x16::from_array([0x1234_5678; 16]);
+/// let r = v.rotate_left(16);
+/// let arr = r.to_array();
+/// assert_eq!(arr[0], 0x5678_1234);
+/// ```
#[inline(always)]
pub fn rotate_left(self, n: u32) -> Self {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl U32x16 { | |
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | |
| /// `u32::rotate_left`). The completeness tier: a full, correct | |
| /// implementation held to the same bar as the SIMD tiers, and the bit-exact | |
| /// reference they are parity-checked against. Delegates to `u32::rotate_left` | |
| /// per lane (`rotate_left(0)` and `rotate_left(32)` both no-op, matching std). | |
| #[inline(always)] | |
| pub fn rotate_left(self, n: u32) -> Self { | |
| let mut out = [0u32; 16]; | |
| for i in 0..16 { | |
| out[i] = self.0[i].rotate_left(n); | |
| } | |
| Self(out) | |
| } | |
| } | |
| impl U32x16 { | |
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | |
| /// `u32::rotate_left`). The completeness tier: a full, correct | |
| /// implementation held to the same bar as the SIMD tiers, and the bit-exact | |
| /// reference they are parity-checked against. Delegates to `u32::rotate_left` | |
| /// per lane (`rotate_left(0)` and `rotate_left(32)` both no-op, matching std). | |
| /// | |
| /// # Examples | |
| /// | |
| /// |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_scalar.rs` around lines 1031 - 1045, Add a `# Examples`
documentation section to each public `rotate_left` implementation on `U32x16`
across the scalar, AVX2, AVX512, and nightly word-type modules. Use a consistent
doctest demonstrating construction, rotation, conversion with `to_array`, and an
assertion of the expected lane value.
Source: Coding guidelines
wasm previously re-exported the scalar U32x16, so a ChaCha20 lane ran soft in the browser. Give wasm a native u32 ARX lane, NEON-style: - U32x4(v128) — the dispatched native unit: splat/from_array/to_array + add (u32x4_add) / bitxor (v128_xor) / rotate_left (v128_or of u32x4_shl|u32x4_shr with the n%32 guard). All safe wasm intrinsics; only v128 load/store are unsafe. - U32x16([U32x4; 4]) — the 16-wide polyfill fanning each op over 4 sub-lanes; its consumer API (Add / BitXor / rotate_left / splat / from_array / to_array + Debug/PartialEq) mirrors simd_avx512::U32x16 exactly, so the one ChaCha20 source compiles unchanged on every tier. Same composition simd_neon uses. - F32x16::to_bits/from_bits (U32x16 is F32x16's bit-repr) routed through from_array/to_array so they work against the native [U32x4;4] shape. - simd.rs wasm arm: U32x16/u32x16 now come from the native wasm32_simd module, dropped from the scalar re-export. BitXor added to the module's ops import. Verification (held to the wasm bar, per the 500d57e node pattern): - Full lib builds for wasm32+simd128 (`cargo --config .cargo/config-wasm.toml build -p ndarray --lib --target wasm32-unknown-unknown`) — exercises the F32x16 interplay + the reductions.rs U32x16 consumer together. - Standalone selfcheck built to wasm + RUN UNDER NODE: add/xor/rotate_left (consts 0/1/7/8/12/16/24/31) + from_array/to_array roundtrip bit-identical to wasm-scalar u32 — rc=0 green. - x86 untouched (edits are wasm32-gated): U32x16 ARX parity test green, clippy -D + fmt clean. Next: native neon U32x16 (extend U32x4 with xor/rotl, compose [U32x4;4]); a wasm+node parity CI job to make the manual node run a standing gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
The x86 `cargo test` suite never touches `simd_wasm.rs` (cfg'd to wasm32), so wasm SIMD correctness was guarded only by a one-off node run recorded in the blackboard (commit 500d57e, "51 numeric checks") — no regression gate. A silent wasm rotate/xor bug would corrupt browser ChaCha with zero CI signal. Make that standing: - crates/wasm-simd-parity/ — a cdylib that depends on the REAL ndarray (no copy → no drift) and calls `ndarray::simd::{U32x16, F32x16, I8x16}`, self-checking each lane's arithmetic against the scalar reference in-module: U32x16 ARX triple (add/xor/rotate_left, consts 0/1/7/8/12/16/24/31 + roundtrip), F32x16 (splat/roundtrip/add/reduce_sum), I8x16 (roundtrip/add). `selfcheck()` returns a distinct code per failing lane+op. WORKSPACE-EXCLUDED so it has zero effect on the x86 jobs; built only by the gate. - scripts/wasm-parity.sh — builds it for wasm32-unknown-unknown +simd128 (the correctness/integration half: building the harness builds ndarray for wasm), then runs the selfcheck under node asserting rc==0 (the parity half). - ci.yaml `wasm_simd` job (in the `conclusion` needs) — installs the wasm target + node 22 and runs the script. The parallel of the existing `nostd` target-guard job, one tier over. Extend the per-lane blocks in lib.rs whenever a new ndarray::simd lane lands. Verified locally: `./scripts/wasm-parity.sh` → "wasm SIMD parity: OK (rc=0)"; x86 workspace lib build clean (exclude respected); ci.yaml valid YAML. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_96ced4af-ff92-438b-89fa-b68f71dfbf10) |
…to-do Durable handoff for the SIMD-crypto arc: the matryoshka plan for USING the U32x16 ARX lane (fork chacha20, clone one backend over ndarray::simd::U32x16, keep RustCrypto's algorithm/tests, [patch] into the encryption stack for transitive acceleration, retire the interim simd_crypto.rs), plus the U32x16 tier-status table and the deferred to-do (native neon U32x16 + aarch64 cross parity CI + matryoshka execution). Blackboard entry cross-refs the plan doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/simd.rs (1)
716-742: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the full rotate-count boundary in parity tests.
The implementation has an explicit
n % 32path, but these tests never exercise32,33, oru32::MAX. Add those counts here and incrates/wasm-simd-parity/src/lib.rsso scalar, AVX, and wasm semantics remain pinned at the boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/simd.rs` around lines 716 - 742, Extend the rotate-count list in u32x16_arx_ops_match_scalar to include 32, 33, and u32::MAX, while retaining the existing counts and scalar rotate_left comparisons. Add the same boundary counts to the corresponding rotate parity test in the wasm-simd-parity library so scalar, AVX, and wasm behavior is covered consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 115-136: Harden the wasm_simd job by setting job-level permissions
to contents: read and disabling credential persistence on its actions/checkout
step with persist-credentials: false. Keep the existing checkout, toolchain,
Node setup, and parity script steps unchanged.
In `@Cargo.toml`:
- Line 410: Update the workspace exclusion change so crates/wasm-simd-parity
remains covered by cargo fmt and cargo clippy -- -D warnings: either remove it
from the exclude list or add equivalent standalone format and Clippy checks to
the wasm_simd job. Preserve the existing wasm_simd build behavior.
In `@crates/wasm-simd-parity/src/lib.rs`:
- Around line 17-19: Add a Rust doc example to the public selfcheck() function
documentation showing that a successful call returns 0, while preserving the
existing description and function behavior.
In `@src/simd_wasm.rs`:
- Around line 872-967: Add rustdoc comments with examples for the public U32x4
and U32x16 methods splat, from_array, to_array, add, and bitxor, matching the
existing rotate_left documentation style. Explicitly document that add performs
lane-wise wrapping addition modulo 2^32, and ensure the examples cover each
method’s API behavior.
---
Nitpick comments:
In `@src/simd.rs`:
- Around line 716-742: Extend the rotate-count list in
u32x16_arx_ops_match_scalar to include 32, 33, and u32::MAX, while retaining the
existing counts and scalar rotate_left comparisons. Add the same boundary counts
to the corresponding rotate parity test in the wasm-simd-parity library so
scalar, AVX, and wasm behavior is covered consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06790015-df40-49f6-a187-3b8c718501a0
⛔ Files ignored due to path filters (1)
crates/wasm-simd-parity/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/ci.yamlCargo.tomlcrates/wasm-simd-parity/Cargo.tomlcrates/wasm-simd-parity/run.mjscrates/wasm-simd-parity/src/lib.rsscripts/wasm-parity.shsrc/simd.rssrc/simd_wasm.rs
✅ Files skipped from review due to trivial changes (2)
- crates/wasm-simd-parity/run.mjs
- crates/wasm-simd-parity/Cargo.toml
Comment on lines
+115
to
+136
| wasm_simd: | ||
| # The x86 `cargo test` suite never touches `simd_wasm.rs` (it is cfg'd to | ||
| # wasm32), so wasm SIMD correctness is otherwise unverified. Build the real | ||
| # `ndarray::simd` wasm types (+simd128) via the excluded `wasm-simd-parity` | ||
| # cdylib and run its numeric selfcheck under node — both integration (the | ||
| # lib compiles for wasm32+simd128) and parity (each lane == scalar). The | ||
| # standing version of the one-off node check from commit 500d57e6. | ||
| runs-on: ubuntu-latest | ||
| name: wasm-simd/parity-node | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| targets: wasm32-unknown-unknown | ||
| # rust-toolchain.toml pins 1.95.0 — install the wasm target for the pinned | ||
| # toolchain too (dtolnay installs for `stable`, which may differ). | ||
| - run: rustup target add wasm32-unknown-unknown | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "22" | ||
| - name: wasm SIMD parity (build + node run) | ||
| run: ./scripts/wasm-parity.sh |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden the new job’s GitHub token permissions.
Line 125 enables checkout credential persistence, and the job inherits broad default permissions. A compromised build script could read the persisted token; this job only needs read access to repository contents. Set persist-credentials: false and grant only contents: read.
Proposed CI hardening
wasm_simd:
+ permissions:
+ contents: read
steps:
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wasm_simd: | |
| # The x86 `cargo test` suite never touches `simd_wasm.rs` (it is cfg'd to | |
| # wasm32), so wasm SIMD correctness is otherwise unverified. Build the real | |
| # `ndarray::simd` wasm types (+simd128) via the excluded `wasm-simd-parity` | |
| # cdylib and run its numeric selfcheck under node — both integration (the | |
| # lib compiles for wasm32+simd128) and parity (each lane == scalar). The | |
| # standing version of the one-off node check from commit 500d57e6. | |
| runs-on: ubuntu-latest | |
| name: wasm-simd/parity-node | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: dtolnay/rust-toolchain@stable | |
| with: | |
| targets: wasm32-unknown-unknown | |
| # rust-toolchain.toml pins 1.95.0 — install the wasm target for the pinned | |
| # toolchain too (dtolnay installs for `stable`, which may differ). | |
| - run: rustup target add wasm32-unknown-unknown | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: "22" | |
| - name: wasm SIMD parity (build + node run) | |
| run: ./scripts/wasm-parity.sh | |
| wasm_simd: | |
| permissions: | |
| contents: read | |
| # The x86 `cargo test` suite never touches `simd_wasm.rs` (it is cfg'd to | |
| # wasm32), so wasm SIMD correctness is otherwise unverified. Build the real | |
| # `ndarray::simd` wasm types (+simd128) via the excluded `wasm-simd-parity` | |
| # cdylib and run its numeric selfcheck under node — both integration (the | |
| # lib compiles for wasm32+simd128) and parity (each lane == scalar). The | |
| # standing version of the one-off node check from commit 500d57e6. | |
| runs-on: ubuntu-latest | |
| name: wasm-simd/parity-node | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - uses: dtolnay/rust-toolchain@stable | |
| with: | |
| targets: wasm32-unknown-unknown | |
| # rust-toolchain.toml pins 1.95.0 — install the wasm target for the pinned | |
| # toolchain too (dtolnay installs for `stable`, which may differ). | |
| - run: rustup target add wasm32-unknown-unknown | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: "22" | |
| - name: wasm SIMD parity (build + node run) | |
| run: ./scripts/wasm-parity.sh |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 125-125: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 115-136: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml around lines 115 - 136, Harden the wasm_simd job
by setting job-level permissions to contents: read and disabling credential
persistence on its actions/checkout step with persist-credentials: false. Keep
the existing checkout, toolchain, Node setup, and parity script steps unchanged.
Source: Linters/SAST tools
| "crates/*", | ||
| ] | ||
| exclude = ["crates/burn"] | ||
| exclude = ["crates/burn", "crates/wasm-simd-parity"] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the excluded parity crate under format and Clippy coverage.
Excluding crates/wasm-simd-parity removes its Rust source from workspace-wide checks, while the new script only runs cargo build. Therefore, the required cargo fmt and cargo clippy -- -D warnings checks do not cover this crate.
Either keep it in the workspace or add standalone format and Clippy steps to the wasm_simd job.
As per coding guidelines, Rust code must be formatted with cargo fmt and pass cargo clippy -- -D warnings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 410, Update the workspace exclusion change so
crates/wasm-simd-parity remains covered by cargo fmt and cargo clippy -- -D
warnings: either remove it from the exclude list or add equivalent standalone
format and Clippy checks to the wasm_simd job. Preserve the existing wasm_simd
build behavior.
Source: Coding guidelines
Comment on lines
+17
to
+19
| /// Distinct nonzero return codes make a CI failure point at the exact lane+op. | ||
| #[no_mangle] | ||
| pub extern "C" fn selfcheck() -> u32 { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file listing ==\n'
git ls-files crates/wasm-simd-parity/src/lib.rs
printf '\n== line-count ==\n'
wc -l crates/wasm-simd-parity/src/lib.rs
printf '\n== relevant slice ==\n'
cat -n crates/wasm-simd-parity/src/lib.rs | sed -n '1,120p'Repository: AdaWorldAPI/ndarray
Length of output: 4912
Add a doc example for selfcheck() — it’s a public API, and the docs should show the expected 0 success return.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/wasm-simd-parity/src/lib.rs` around lines 17 - 19, Add a Rust doc
example to the public selfcheck() function documentation showing that a
successful call returns 0, while preserving the existing description and
function behavior.
Source: Coding guidelines
Comment on lines
+872
to
+967
| /// 4×u32 in one WASM `v128` (u32x4 interpretation) — the native ARX unit. | ||
| #[derive(Copy, Clone)] | ||
| #[repr(transparent)] | ||
| pub struct U32x4(pub v128); | ||
|
|
||
| impl U32x4 { | ||
| pub const LANES: usize = 4; | ||
|
|
||
| #[inline(always)] | ||
| pub fn splat(v: u32) -> Self { | ||
| Self(u32x4_splat(v)) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn from_array(a: [u32; 4]) -> Self { | ||
| // SAFETY: a `[u32; 4]` is exactly 16 bytes; one unaligned v128 load. | ||
| Self(unsafe { v128_load(a.as_ptr() as *const v128) }) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn to_array(self) -> [u32; 4] { | ||
| let mut a = [0u32; 4]; | ||
| // SAFETY: store writes exactly 16 bytes into the 16-byte array. | ||
| unsafe { v128_store(a.as_mut_ptr() as *mut v128, self.0) }; | ||
| a | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn add(self, o: Self) -> Self { | ||
| Self(u32x4_add(self.0, o.0)) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn bitxor(self, o: Self) -> Self { | ||
| Self(v128_xor(self.0, o.0)) | ||
| } | ||
|
|
||
| /// Lane-wise left-rotate by `n` bits — the ARX rotate (matches | ||
| /// `u32::rotate_left`). wasm has no rotate op, so this is the shift-or | ||
| /// with the `n % 32 == 0` guard (`u32x4_shr` by 32 would over-shift); | ||
| /// `u32x4_shl`/`u32x4_shr` already mask the count to `& 31`, so | ||
| /// non-zero `n` is exact. Rotate amount is a public ARX constant. | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| let n = n % 32; | ||
| if n == 0 { | ||
| return self; | ||
| } | ||
| Self(v128_or(u32x4_shl(self.0, n), u32x4_shr(self.0, 32 - n))) | ||
| } | ||
| } | ||
|
|
||
| /// 16×u32 as `[U32x4; 4]` — the NEON-style 16-wide polyfill. Consumer API | ||
| /// (`Add` / `BitXor` / `rotate_left`) matches `simd_avx512::U32x16`. | ||
| #[derive(Copy, Clone)] | ||
| #[repr(align(64))] | ||
| pub struct U32x16(pub [U32x4; 4]); | ||
|
|
||
| impl U32x16 { | ||
| pub const LANES: usize = 16; | ||
|
|
||
| #[inline(always)] | ||
| pub fn splat(v: u32) -> Self { | ||
| Self([U32x4::splat(v); 4]) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn from_array(a: [u32; 16]) -> Self { | ||
| Self([ | ||
| U32x4::from_array([a[0], a[1], a[2], a[3]]), | ||
| U32x4::from_array([a[4], a[5], a[6], a[7]]), | ||
| U32x4::from_array([a[8], a[9], a[10], a[11]]), | ||
| U32x4::from_array([a[12], a[13], a[14], a[15]]), | ||
| ]) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn to_array(self) -> [u32; 16] { | ||
| let mut o = [0u32; 16]; | ||
| for i in 0..4 { | ||
| o[i * 4..i * 4 + 4].copy_from_slice(&self.0[i].to_array()); | ||
| } | ||
| o | ||
| } | ||
|
|
||
| /// Lane-wise left-rotate by `n` bits (ARX rotate), fanned over 4 lanes. | ||
| #[inline(always)] | ||
| pub fn rotate_left(self, n: u32) -> Self { | ||
| Self([ | ||
| self.0[0].rotate_left(n), | ||
| self.0[1].rotate_left(n), | ||
| self.0[2].rotate_left(n), | ||
| self.0[3].rotate_left(n), | ||
| ]) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding definitions.
wc -l src/simd_wasm.rs
ast-grep outline src/simd_wasm.rs --view expanded | sed -n '1,220p'
echo "---- snippet around lines 840-980 ----"
sed -n '840,980p' src/simd_wasm.rs
echo "---- search for nearby public doc patterns in SIMD wrappers ----"
rg -n "///|pub fn (splat|from_array|to_array|add|bitxor|rotate_left)" src/simd_wasm.rs src/simd_*.rsRepository: AdaWorldAPI/ndarray
Length of output: 50378
Document the new U32x4/U32x16 API and wrapping semantics. splat, from_array, to_array, add, and bitxor still need /// docs with examples here; rotate_left already has one. Also call out that add is lane-wise wrapping modulo 2^32.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/simd_wasm.rs` around lines 872 - 967, Add rustdoc comments with examples
for the public U32x4 and U32x16 methods splat, from_array, to_array, add, and
bitxor, matching the existing rotate_left documentation style. Explicitly
document that add performs lane-wise wrapping addition modulo 2^32, and ensure
the examples cover each method’s API behavior.
Source: Coding guidelines
AdaWorldAPI
changed the title
`chacha20_keystream` used `std::is_x86_feature_detected!("avx512f")` — a
std-only macro, so the no-default-features build (tests/1.95.0, blas-msrv)
failed with E0433. Switch to the workspace's compile-time model:
`#[cfg(all(target_arch="x86_64", target_feature="avx512f"))]`, matching the
wasm128 arm and the `ndarray::simd` lane dispatch (no runtime detect, no
`#[target_feature]`). `chacha20_block16_avx512` is now cfg-gated + a safe fn
(intrinsics available statically; body in one unsafe block).
no-default-features build + clippy green; default avx512 build simd_crypto
5/5 + u32x16_arx pass; fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
…n test, tri.rs type The is_x86_feature_detected fix let the lib compile, which unblocked the --no-default-features test build and surfaced three compile errors: 1. simd_crypto tests used `vec![[0u8;64]; n]` — no `vec!` under no_std. Use fixed-size arrays ([[0u8;64];40] / [[0u8;64];16]); block counts are const. 2. tests/chacha20_rustcrypto_parity.rs imports `ndarray::simd` (cfg std) + the `chacha20` dev-dep. Gate the whole file `#![cfg(feature = "std")]` — a no-op in no_std builds. 3. src/tri.rs test_odd_k: `serde_json` (dev-dep via criterion) now ships `impl PartialEq<Value> for i32`, making bare `Array2::zeros` ambiguous in the `assert_eq!`. Pin `Array2::<i32>::zeros` — dep-drift, not from this PR, but #240 can't be green without it. no_std test compile Finished clean; default std simd_crypto 5/5 + rustcrypto parity 3/3; fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6pT32kk6pnuAAqR3JiYqu
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_24612cb8-0a26-4337-9ad4-3f594bfd2eb8) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/CHACHA20_MATRYOSHKA_PLAN.md (1)
84-88: 🩺 Stability & Availability | 🔵 TrivialMake the compile-time dispatch deployment contract explicit.
This is safe only when every produced binary is guaranteed to run on hardware with the enabled features. Otherwise, forcing AVX-512 can terminate the process with
SIGILL; upstreamchacha20currently defaults to runtime feature detection and requires explicit configuration to force AVX backends. (docs.rs)Please add an enforced build/deployment guard—or retain a runtime fallback—for artifacts that may run on heterogeneous x86 hosts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/CHACHA20_MATRYOSHKA_PLAN.md around lines 84 - 88, Update the “Dispatch” section of the CHACHA20_MATRYOSHKA_PLAN to require an enforced build/deployment guard proving that compile-time enabled target features are available on every deployment host, or specify runtime feature detection with a portable fallback for heterogeneous x86 artifacts. Explicitly cover the SIGILL risk and ensure the chosen approach prevents unsupported binaries from being produced or deployed.src/simd_crypto.rs (1)
210-228: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffDe-vectorization uses scalar store+extract loops instead of a SIMD transpose.
Two nested 16×16 scalar copy loops (store-then-index-copy, then per-byte serialize) run per 16-block stride. Functionally correct, but leaves throughput on the table versus a shuffle/permute-based transpose. Given the AEAD isn't wired to this path yet, this is safe to defer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/simd_crypto.rs` around lines 210 - 228, Replace the scalar de-vectorization and serialization loops in the SIMD block-processing path with a SIMD shuffle/permute-based transpose that converts the lane-oriented vectors into per-block words and emits the 16 serialized blocks efficiently. Preserve the existing RFC 8439 feed-forward addition and little-endian output layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.claude/CHACHA20_MATRYOSHKA_PLAN.md:
- Around line 84-88: Update the “Dispatch” section of the
CHACHA20_MATRYOSHKA_PLAN to require an enforced build/deployment guard proving
that compile-time enabled target features are available on every deployment
host, or specify runtime feature detection with a portable fallback for
heterogeneous x86 artifacts. Explicitly cover the SIGILL risk and ensure the
chosen approach prevents unsupported binaries from being produced or deployed.
In `@src/simd_crypto.rs`:
- Around line 210-228: Replace the scalar de-vectorization and serialization
loops in the SIMD block-processing path with a SIMD shuffle/permute-based
transpose that converts the lane-oriented vectors into per-block words and emits
the 16 serialized blocks efficiently. Preserve the existing RFC 8439
feed-forward addition and little-endian output layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06509d44-2bf2-45c0-a2a2-4c827e5c499f
📒 Files selected for processing (5)
.claude/CHACHA20_MATRYOSHKA_PLAN.md.claude/blackboard.mdsrc/simd_crypto.rssrc/tri.rstests/chacha20_rustcrypto_parity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/chacha20_rustcrypto_parity.rs
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
2 participants
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
The ogar-crypto-SIMD arc: give
ndarray::simdthe hardware-accelerated ChaCha20 ARX vocabulary (server AVX-512 / browser wasm128) so the Adaencryptionstack can be accelerated without anyone hand-rolling a cipher — RustCrypto owns the algorithm, ndarray owns the SIMD.Why hand-vectorizing ChaCha20 is safe (unlike AES): ChaCha20 is ARX — every op is a 32-bit
wrapping_add/xor/ fixedrotate_left, no secret-dependent branch or memory index → constant-time by construction, preserved by a straight-line SIMD form (RFC 8439's reason for choosing it). AES-in-software is out of scope (secret-indexed tables; browsers expose hardware AES via WebCrypto).What landed
Interim ChaCha20 primitive (
src/simd_crypto.rs) — scalar reference + AVX-512 (16-blockVPROLVD/_mm512_*) + wasm128 (v128u32x4) backends ofchacha20_block/chacha20_keystream, re-exported throughndarray::simd::*. Trust-gated three ways: RFC 8439 §2.3.2 KAT, dispatcher-vs-scalar parity (spanning strides + tail), and byte-parity vs the vetted RustCryptochacha20across the parameter space. sentinel-qaunsafeaudit: PASS (constant-time confirmed).The
U32x16ARX lane — the reusable substrate the matryoshka rides.U32x16already hadAdd+BitXor; this adds the missingrotate_lefton every tier and makes it native where it was scalar:rotate_left__m512i_mm512_rolv_epi32[U32x4; 4](was scalar re-export)v128_or(u32x4_shl, u32x4_shr)[u32;16]/core::simdwasm SIMD parity CI gate —
crates/wasm-simd-parity/(workspace-excluded cdylib on the realndarray::simdtypes, no drift) +scripts/wasm-parity.sh+ awasm_simdCI job: builds forwasm32+simd128(correctness/integration) and runs a lane-by-lane selfcheck under node (parity —U32x16ARX +F32x16+I8x16). The x86cargo testnever touchessimd_wasm.rs, so this is its only standing guard — the permanent version of the one-off node run from commit500d57e6.Security ruling (documented, not code): the
encryptionXChaCha20-Poly1305 AEAD is deliberately NOT re-wired to the primitive — that would mean hand-composing HChaCha20 + Poly1305 (the roll-your-own-AEAD footgun). Seecrates/encryption/src/aead.rsmodule doc. The accelerated lane is for the matryoshka backend, which keeps RustCrypto's algorithm intact.The matryoshka (how the lane gets used — full plan in
.claude/CHACHA20_MATRYOSHKA_PLAN.md)Fork
chacha20, clone one backend (avx2.rs) and rewire its round body overndarray::simd::U32x16(nounsafe, no intrinsics in the carried file — all in ndarray),[patch]the fork intochacha20poly1305 → encryption → consumersfor transitive acceleration with zero AEAD/consumer changes, gate bit-exact vs RustCrypto'ssoft, then retiresimd_crypto.rs. Keeps RustCrypto's rounds/constants/tests verbatim; a security re-sync is a ~zero-conflict one-file re-apply; upstreamable (→ fork evaporates).Deferred (token limit — tracked in the plan doc + blackboard)
U32x16 = [U32x4; 4](extendU32x4(uint32x4_t)with xor/rotl).wasm-simd-parity; closes NEON's identical x86-suite blind spot.chacha20+ patch-in).Test Plan
cargo test --lib simd_crypto+--test chacha20_rustcrypto_parity— RFC 8439 KAT, dispatcher parity, RustCrypto cross-parity (all green).cargo test --lib u32x16_arx_ops_match_scalar—U32x16add/xor/rotate_left vs per-laneu32(green on the compiled tier)../scripts/wasm-parity.sh— full lib buildswasm32+simd128; node selfcheckrc=0.clippy -D warnings/fmtclean; ci.yaml valid.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit