The SIMD ladder: the cycle map, portable BLAKE3, and the u64 ARX rotate - #268
Conversation
The goal is that every dependency carrying its own SIMD instead consumes
ndarray::simd -- all SIMD living once, audited, in one place. This records
the rungs, what is proven at each, and the order.
The load-bearing finding is the cycle map, and it is measured rather than
inferred: whether a dependency CAN consume ndarray::simd is decided by which
PACKAGE in the workspace pulls it, not by the workspace as a whole.
chacha20 crates/encryption -> ... -> chacha20 no cycle
curve25519-dalek crates/encryption -> ed25519-dalek -> ... no cycle
blake3 ROOT ndarray (std feature, 11 hpc files) CYCLE
$ cargo tree -p ndarray -i curve25519-dalek
error: package ID specification `curve25519-dalek` did not match any packages
Only blake3 is pulled by the root package, so only blake3 closes a loop. The
intuitive rule -- "ndarray depends on X, so X cannot depend on ndarray" -- is
wrong at workspace granularity and would have parked two rungs that have no
blocker at all.
Second scoping finding: ndarray's own blake3 usage is entirely single-input
(hash / Hasher::{new,new_keyed,update,finalize,finalize_xof}). No hash_many.
So cutting the cycle needs only the PORTABLE core -- scalar [u32;16] compress
plus the chunk/tree/XOF state machine -- which uses no SIMD at all. That
splits the rung into 3a (cut the cycle, no benchmark needed to justify it)
and 3b (hash_many throughput on the merged shuffle surface, which is where a
benchmark matters and which is not on the critical path).
Rungs 1 and 2 are merged and measured: the U32x16 ARX triple at the AVX2
instruction floor, and the shuffle surface parity-checked against the real
intrinsics on all six backends. Rungs 4-6 (chacha20, dalek, the u64 ARX lane
for BLAKE2b/argon2) are independent of each other and of 3a.
Stated plainly in the doc: nothing here is claimed to be faster. Every rung
is measured at instruction class, never at time, and no benchmark of any rung
against its upstream equivalent has been run. Rung 5 is also explicitly not a
defect -- `serial` is a correct, deliberate choice and the dalek vector
backend serves no operation this crate performs.
EPIPHANIES entry records the per-package cycle rule and that its failure mode
is SILENT: cargo does not error, it declines the patch and falls back to the
registry crate with [[patch.unused]], so a port done anyway compiles clean
and does nothing -- the same shape as the chacha20 patch, now two independent
instances in one dependency graph.
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_8aa51e77-e7c4-4026-859b-4395902c5636) |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a portable in-tree BLAKE3 implementation with vector-backed tests, extends ChangesBLAKE3 and SIMD consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Hasher
participant ChunkState
participant Compression
participant TestVectors
Caller->>Hasher: update input
Hasher->>ChunkState: process chunks
ChunkState->>Compression: compress blocks
Hasher->>Compression: finalize tree
Compression-->>Caller: hash or XOF output
TestVectors->>Hasher: validate outputs
Possibly related PRs
Suggested reviewers: 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: e710766e86
ℹ️ 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".
| `vendor/chacha20` already has an `ndarray_simd` backend. Measured: no image | ||
| and no CI job compiles it (`chacha20-vendoring-blast-radius.md`). And |
There was a problem hiding this comment.
The claim that no CI job compiles this backend is false: the wasm_simd job in .github/workflows/ci.yaml explicitly builds vendor/chacha20/Cargo.toml with wasm32+simd128, and that target selects backends::ndarray_simd. Only the AVX-512 arm lacks CI coverage, so this wording incorrectly makes the broader backend appear entirely untested.
Useful? React with 👍 / 👎.
| **And the failure is silent.** Cargo does not error on the blake3 case — it | ||
| names the chain, declines to apply the patch, and falls back to the registry | ||
| crate with `[[patch.unused]]`. A port done anyway compiles clean and has zero |
There was a problem hiding this comment.
When a patched blake3 that depends on ndarray is actually selected, Cargo reports a cyclic package dependency rather than silently falling back; [[patch.unused]] means the patch was not selected, such as because of its version or the lockfile. Attributing the unused patch to the cycle teaches maintainers to misdiagnose an independently stale or incompatible patch and undermines the prescribed build-graph check.
Useful? React with 👍 / 👎.
| 3a is the only rung that unblocks anything else, and it needs no measurement | ||
| to justify — it removes a dependency edge, a C build, and a second SIMD | ||
| surface regardless of throughput. Everything after it is independent: |
There was a problem hiding this comment.
The ordering premise is incorrect for rungs 4–6: the existing ChaCha20 backend already depends on ndarray, the document itself establishes that dalek has no cycle, and the u64 lane is implemented inside ndarray; none is unblocked by replacing the root package's BLAKE3 dependency. Presenting arrows from 3a will unnecessarily serialize independent work and directly contradicts the statement here that everything after it is independent.
Useful? React with 👍 / 👎.
| 3a is the only rung that unblocks anything else, and it needs no measurement | ||
| to justify — it removes a dependency edge, a C build, and a second SIMD | ||
| surface regardless of throughput. Everything after it is independent: |
There was a problem hiding this comment.
The parent manifest already configures BLAKE3 with default-features = false, features = ["pure"], whose accompanying comment records that it removes all C/ASM compilation. Rung 3a can remove the Rust dependency and its intrinsic surface, but not a C build that is already absent, so this rationale overstates the benefit of transcribing a cryptographic implementation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.claude/knowledge/the-simd-ladder.md:
- Around line 134-139: Update the fenced ASCII diagram near the dependency
ladder to specify the text language identifier, changing the opening fence to
use ```text while preserving the diagram content and closing fence.
- Around line 33-41: Correct the dependency-cycle documentation so
missing-package errors are distinguished from valid absent edges: in
.claude/knowledge/the-simd-ladder.md lines 33-41, explicitly label the failed
curve25519-dalek graph selection as inconclusive because the package is absent
from that Cargo graph, and use a package that exists in the ndarray graph when
documenting an absent edge; apply the same clarification and package correction
to .claude/board/EPIPHANIES.md lines 21-24 and 44-46.
🪄 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: 9c792569-d82e-44c0-a51a-faf386f0a671
📒 Files selected for processing (2)
.claude/board/EPIPHANIES.md.claude/knowledge/the-simd-ladder.md
Rung 3a of the ladder, landed as the module + the measurement, WITHOUT the
call-site swap -- because the swap turns out to have a price tag.
## The module
src/hpc/blake3.rs, 771 lines, transcribed from upstream's own
reference_impl/reference_impl.rs (the spec-referenced serial implementation).
No unsafe, no core::arch, no new dependencies. Scoped by the finding that
ndarray's blake3 usage is entirely single-input -- 14 call sites across 8
files use only hash / Hasher::{new,new_keyed,update,finalize,finalize_xof} /
Hash::as_bytes, no hash_many -- so the serial core suffices.
Correctness proven against the official vectors, vendored alongside: 35/35
cases, unkeyed AND keyed, each at both 32-byte length and full extended
length via finalize_xof().fill() -- 140 assertions, input lengths 0..102400.
Plus streaming (one update vs many 37-byte updates over 102400 bytes), empty
input, and incremental fill.
Hash::eq is CONSTANT-TIME. The transcription arrived with a plain `==`, which
leaks match-prefix length through timing when a BLAKE3 output is used as a
MAC; upstream uses the constant_time_eq crate, and since this takes no new
deps the XOR-fold is written inline with a black_box on the accumulator. No
call site compares two Hash values today (seal.rs compares the truncated
MerkleRoot), so it is a guard for future consumers rather than a live fix.
## The measurement, and why the swap is not in this commit
First actual TIME number in this arc -- everything prior was
instruction-class. `.claude/knowledge/blake3-ab-bench/`, release, two runs:
16 B 137 ns vs 100 ns 1.34x
256 B 445 ns vs 350 ns 1.28x
2 KB 4.3 us vs 2.7 us 1.34-1.60x
64 KB 115 us vs 23 us 4.6-5.1x
Two gaps, different causes. The 64 KB gap is the absent hash_many (rung 3b).
The 1.3x small-input gap is NOT -- at 16 B there is one compression and no
parallelism to be had; the crate is faster because it SIMD-accelerates the
single compress itself. Closing that needs a U32x4-shaped compress, a rung
the plan did not have. Call it 3c.
So the earlier framing -- "removes things, needs no benchmark to justify" --
was true about what it removes and silent about what it costs. Dropping the
dependency is a trade, and it is the operator's call, so the call sites still
use the external crate here.
## Four corrections from codex on #268, all valid
- "No CI job compiles the chacha20 ndarray_simd backend" is FALSE.
ci.yaml:141-142 builds vendor/chacha20 for wasm32+simd128, selecting that
backend, under a comment calling it the wasm matryoshka guard. Only the
AVX-512 arm is uncovered. Fixed here AND in the already-merged
chacha20-vendoring-blast-radius.md. Fourth instance of the same
scope-quantifier error: checked the x86 path, generalized to all targets.
- 3a does NOT remove a C build. features = ["pure"] removed all C/ASM in
#264 already. 3a removes the Rust dependency and its intrinsic surface.
Overstating this matters when the counterweight is transcribing a crypto
implementation.
- The ordering arrows from 3a to rungs 4-6 contradicted the same section's
own next sentence and would serialize independent work. Only 3a -> 3b is
real; chacha20's backend already depends on ndarray, dalek has no cycle,
and the u64 lane lives inside ndarray.
- [[patch.unused]] is NOT cargo's cycle diagnostic. Cargo reports the cycle
by naming the chain (cargo update -p blake3 did); an unused patch only
means the patch was not selected, usually a version mismatch or a stale
lockfile. Conflating them teaches misdiagnosis and undermines the
cargo-tree check the entry prescribes.
4 blake3 tests pass; clippy clean; fmt clean.
CI red on seven jobs -- clippy/1.95.0, tests/1.95.0, native-backend/stable, hpc-stream-parallel/rayon, tier4-avx512-check, neon-simd/parity-qemu -- all with missing_docs errors in src/hpc/plane.rs, a file the change never touched. Cause, entirely mechanical and mine: src/hpc/mod.rs line 54 carries #[allow(missing_docs)] for `pub mod plane;`. I inserted `pub mod blake3;` directly after that attribute, which re-targeted it onto the new module and left plane unguarded. One line, seven jobs. Same class as the neon insertion earlier in this arc: adding a line before an item that had an attribute above it. Declaring blake3 separately -- it is fully documented and wants no allow -- with a comment recording the trap so the next insertion does not repeat it. Verified: RUSTFLAGS="-D warnings" cargo clippy -p ndarray --lib now reports 0 errors (was 40+); the 4 blake3 vector tests still pass. Also, two CodeRabbit findings on the same PR: - MD040 on the ordering diagram fence. - The cycle check rested on WEAK EVIDENCE. `cargo tree -p ndarray -i curve25519-dalek` failing with "package ID specification did not match any packages" cannot distinguish "no such edge" from "no such package" -- a mistyped name produces the byte-identical message, demonstrated inline. Replaced with the positive reverse tree (which terminates at `encryption`, root ndarray absent) plus a control showing the same command shape DOES hit for a real edge (`-p ndarray -i blake3`). Applied in both the ladder doc and the EPIPHANIES entry. The evidence correction is the same defect as the vacuous-assertion one, one level up: a check that cannot fail for the reason you believe it is failing.
…2 KB) Operator hypothesis, citing the blasgraph JIT-gap precedent: the 1.3x gap might be closed by proper use of the existing slice primitives rather than by new SIMD. Worth testing, because the staging path copies every byte TWICE -- input -> self.block -> block_words -- and for a full block that first copy is pure overhead. Implemented as a crate::simd_ops::array_chunks::<u8, 64> walk in ChunkState::update. array_chunks is as_chunks, a pointer cast, so a full block goes input -> words with no intermediate copy. Guarded `input.len() > BLOCK_LEN`, strictly greater. BLAKE3 must not compress a chunk's FINAL block until it knows whether more input follows, because that block carries CHUNK_END; holding back the last <= 64 bytes preserves it. Measured, three runs: 16 B 137 -> 134 ns -- (<= 1 block, never reaches the fast path) 256 B 445 -> 437 ns -- 2 KB 4322 -> 3421 ns -21% 64 KB 115 -> 109 us -5% Verdict: real, and bounded. The double copy was costing ~21% at the mid sizes -- not nothing, and free to remove. It does NOT replace the two structural gaps: inputs of one block or less never reach the fast path, and at 64 KB the copy is noise beside the absent hash_many. Ratio at 2 KB moved 1.34-1.60x to a stable 1.30x; the small-input 1.3x (the crate's SIMD single-compress) and the bulk 4.8x (hash_many) both stand. Correctness gated rather than assumed: the official vectors cover every boundary this turns on -- 63/64/65 (the > BLOCK_LEN guard itself), 127/128/129, and 1023/1024/1025 (the chunk boundary). All 35 cases still pass at both 32-byte and full XOF length. clippy clean under -D warnings; fmt clean.
The measurement doc guessed "part of the 1.3x is likely the reference-impl choice rather than the SIMD gap -- portable.rs avoids a per-block staging copy -- and that has not been separated out." It is separated now, and the guess was wrong for the case that matters. Removing the staging copy bought 21% at 2 KB and ZERO at <= 1 block, because inputs that small never reach the fast path at all. So the small-input 1.3x is not the staging copy; it is the crate's SIMD single-compress, as originally suspected. Rewrote the deviation note to say that, and narrowed the "not claimed" bullet from "1.3x is unattributed and confounded" to the accurate "the staging-copy term is separated and measured; what remains at small inputs is PRESUMED to be the SIMD single-compress and has not been isolated by building one." Also a `text` fence on the rung diagram -- same MD040 CodeRabbit raised on the sibling doc, fixed here before it is raised again.
|
@codex — all four were valid and all four are fixed. The review targeted 1. wasm ChaCha20 CI coverage — you were right, and the false claim was also in the already-merged #266 doc, so I corrected both.
The rung is now stated arm-by-arm. This was the fourth instance in this repo of the same scope-quantifier error — check the x86 path, generalize to all targets — so it's named as such in the text rather than quietly patched. 2.
Cargo did report the cycle — 3. The 3a arrows — correct, and it contradicted the sentence directly beneath it.
4. Crediting 3a with removing a C build — also correct, and the PR that removed it was mine (#264).
Worth flagging on this one: your framing — "overstates the benefit of transcribing a cryptographic implementation" — was the load-bearing part. I've since benchmarked the transcription against the crate, and the trade is sharper than the doc implied: 1.3× slower on ndarray's typical input sizes, ~4.8× at 64 KB. So the call sites are deliberately not swapped in this PR; the module lands with the numbers attached and the swap stays an operator decision. Generated by Claude Code |
|
To use Codex here, create an environment for this repo. |
…pill
Operator question: what about the synergies of making 4096 bit / 512 byte
the default lane, since that is the canonical SoA node size anyway
(key(16) | edges(16) | value(480))?
Answered by measurement rather than argument. U32x16 is 512 BIT -- one eighth
of a node -- so a node-wide unit is [U32x16; 8] = 128 u32 lanes. It cannot be
a register type (512 byte is 8 zmm / 16 ymm), so it is necessarily a TILE over
the existing lane, exactly as U32x16 is already a polyfill over 2 ymm on avx2.
The open question was whether the wider unit costs anything, and the failure
mode to watch was spill -- 128 u32 against 16 architectural registers.
Two probes, same work, asserted bit-for-bit identical in the driver:
arx_node4096 98 packed / 0 scalar / 0 memory
arx_lane512_x8 81 packed / 0 scalar / 0 memory
The ARITHMETIC IS IDENTICAL: 16 vpaddd + 16 vpxor + 16 vpshufb + 33 vmovdqa in
both. The 17-instruction delta is 16 vmovaps + 1 vxorps -- the dead zero-init
of [U32x16::splat(0); 8] in the loop form, which an implementation building
the array by value does not emit. Neither form spills.
So the node width is free, and the discriminating property is LIVENESS rather
than width:
streaming / elementwise (ARX, bitwise, add-mul) each vector independent,
never all live -> 0 spill
whole-node-live (a transpose) all live at once ->
transpose_16x16_composed
pays 41 memory ops
That is the rule to design a node-wide default unit against: viable for
streaming work; the ops that must stay lane-width are the ones needing the
whole node resident.
Also widens the blake3 A/B bench to the sizes this substrate actually hashes
-- 480 B (the node's value region) and 512 B (the node itself) sat in the gap
between the previous 256 and 2048 samples. At the canonical node size the
in-tree BLAKE3 is 1.28x (848 ns vs 664 ns), and the 4.8x bulk gap is
irrelevant there because 512 B never reaches hash_many. That reframes the
swap decision from a curve into a single number.
Rung 6 of the ladder rests on one measurement: a scalar u64 rotate loop does not vectorize. That used a single source form -- u64::rotate_right(n) -- which leaves an obvious objection open. Maybe LLVM declines the rotate IDIOM at 64-bit width and an explicit shift-or would vectorize, exactly as it does for u32's rotate_left(12)/(7). AVX2 has vpsllq/vpsrlq, so the ingredients exist. Tested. It does not. rot_u64x8 u64::rotate_right(n) runtime 0 packed shiftor_rot_u64x8 (x >> n) | (x << (64-n)) runtime 0 packed shiftor_rot_const_u64x8 explicit shift-or CONST 0 packed The third row is the sharp one. At u32 width a constant amount vectorizes two different ways depending on granularity -- byte-granular to vpshufb, bit-granular to the shift-or triple -- and both are packed. At u64 width neither happens, for either kind of constant. A fourth confirmation arrived unasked. A blake2b_g_shiftor_u64x8 probe was written as the direct counterpart to blake2b_g_u64x8; it never appeared in the emitted assembly. LLVM FOLDED the two functions -- zero mentions of the shiftor symbol, two call sites on the survivor -- proving the spellings are byte-identical rather than merely both-scalar. Removed rather than kept as a permanently-failing row: a probe the compiler cannot distinguish from its own control measures nothing. The removal and its reason are recorded in the probe file and the baseline so the next reader does not re-add it. Net: rung 6 is confirmed as the crate's one earned intrinsic override, now on three independent spellings instead of one.
Rung 6 of the SIMD ladder. `rotate_left`/`rotate_right` did not exist on `U64x8` on any backend, and the codegen oracle measured that a scalar u64 rotate loop does NOT vectorize: 0 packed, one `rorq` per lane, across three independent spellings (`u64::rotate_right`, an explicit shift-or with a runtime amount, and the same with BLAKE2b's compile-time constants 32/24/16/63). LLVM folded two of the probes into byte-identical code, which says it declines the 64-bit *operation* rather than the rotate *idiom* — and it does emit `vpsllq`/`vpsrlq` for the u32 lane on the same target. That is the entry criterion in simd-one-spec-design.md, and this is the only place in the crate that currently meets it. Contrast rung 1, where hand-written u32 intrinsics LOSE to the optimizer (.claude/knowledge/td-t22-asm-investigation.md). What each backend actually got — stated plainly, because only one of them is the override: - avx512: native `_mm512_rolv_epi64` / `_mm512_rorv_epi64` on the real `__m512i`. This is the earned intrinsic. - avx2, scalar, nightly: correct-but-unvectorized per-lane loops. These are NOT the explicit shift-or; they are the fallback, and the measured cost is known rather than overlooked. neon and wasm re-export the scalar `U64x8` and are covered by that arm. `n` is taken mod 64 and `n == 0` short-circuits, because `x >> 64` is UB on `u64` and `VPROLVQ` takes its own count mod 64 — so the two agree at every input rather than only in the interior. Test `u64x8_arx_rotate_matches_scalar` covers 0/1/7/16/24/31/32/63/64, asserts `rotr(n) == rotl(64-n)` and round-trip identity. Control-tested: with `rotate_right`'s body flipped to `rotate_left` the test fails, so it discriminates rather than passing vacuously. Nothing consumes this yet. BLAKE2b needs it and argon2 needs BLAKE2b; that is the next rung, not this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
…nsic Records what shipped rather than what was planned: the native VPROLVQ/VPRORVQ override exists on avx512 only; avx2/scalar/nightly carry per-lane loops that are measured not to vectorize, and the explicit vpsllq/vpsrlq shift-or for avx2 is NOT written. Stating that as a table avoids the reading that all six backends got the override. Also reconciles two lines the rung-6 change made stale: the 'rungs 4-6 are optional' summary, and the 'only 3a removes a C build' clause, which #264's features = ["pure"] had already made false and which rung 3a's own section already corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
AdaWorldAPI
changed the title
|
@coderabbitai review Requesting a full pass explicitly, because the automatic one didn't cover the code. The first review attempt hit the rate limit and was skipped; the walkthrough that replaced it describes only the documentation files and rates the PR "effort: 1 (Trivial), ~5 minutes". That rating is against a diff that includes:
Two things I'd specifically like challenged rather than confirmed:
Prior review on this PR caught four real defects (a retracted Generated by Claude Code |
|
I’ll specifically scrutinize the BLAKE3 state machine and chunk-final-block invariant, test-vector coverage boundaries, rotate-count semantics—including 🐇🔍 ✅ Action performedFull review finished. |
…n it `ChunkState::update` does not split across chunks — `Hasher::update` caps each call via `want = CHUNK_LEN - self.chunk_state.len()`. The `array_chunks` fast path silently depends on that cap: `full = (input.len() - 1) / BLOCK_LEN` is only bounded by the chunk if the caller already bounded the input. Given a longer slice it would compress more than 16 blocks under one chunk counter and produce a wrong-but-plausible hash. The official test vectors would NOT catch that. They only reach this method through `Hasher::update`, so they exercise the invariant rather than testing it — the failure mode is a future refactor of the caller, or any direct call, not a bad vector. So assert it at the point of reliance instead of trusting a caller 250 lines away. `debug_assert!` rather than `assert!`: this is an internal API whose callers are all in this file, the release path should keep the fast path branch-free, and the vectors already cover every boundary the fast path turns on (63/64/65, 127/128/129, 1023/1024/1025). Control-tested rather than assumed: a temporary test calling `ChunkState::update` with `CHUNK_LEN + 1` bytes fires it — "ChunkState::update overran a chunk: 0 + 1025 > 1024" — so the assert discriminates. The temporary test is not kept; it would have to be `#[should_panic]` on a debug-only assert, which is a fragile shape. Found by reading the fast path adversarially while an external review was pending, on the principle that the automated pass should land against an independent check rather than be the only one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
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_54b45226-e6b8-4f8c-a76a-30a715fa05d6) |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/hpc/blake3.rs (2)
590-594: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
.into()on an identical type may tripclippy::useless_conversion.
as_bytes()already yields&[u8; OUT_LEN], so the deref alone is the return value. As per coding guidelines, "cargo clippy -- -D warningsmust pass without any warnings".♻️ Proposed simplification
- (*hasher.finalize().as_bytes()).into() + *hasher.finalize().as_bytes()🤖 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/hpc/blake3.rs` around lines 590 - 594, Remove the redundant .into() from the return expression in derive_key, returning the dereferenced hasher.finalize().as_bytes() array directly while preserving the existing [u8; OUT_LEN] result.Source: Coding guidelines
539-569: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
fillrecompresses the root block on every call; consider caching it.Each
fillinvocation recompresses the current 64-byte output block even when the previous call left bytes unconsumed in that same block, so streaming in small increments costs one compression per call (thechunks_mut(7)test path does ~43 extra compressions for 300 bytes). Caching(block_counter, block_bytes)inOutputReaderand refilling only on block change would also letOutput::root_output_bytes(Lines 158-175) be expressed in terms of this same path instead of duplicating the word→little-endian emission.🤖 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/hpc/blake3.rs` around lines 539 - 569, Cache the compressed output block in OutputReader, keyed by block_counter, and reuse its block_bytes while fill consumes remaining bytes; recompress only when advancing to a new block. Update Output::root_output_bytes to use the shared OutputReader path instead of duplicating word-to-little-endian conversion, preserving existing output bytes and position behavior.src/hpc/mod.rs (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIncident history in a
///doc comment will render in public rustdoc.This is an outer doc comment on
pub mod blake3, so rustdoc concatenates it with the module's own//!header — themissing_docs/seven-CI-jobs anecdote becomes part of the published module description. A plain//comment keeps the rationale next to the code without publishing it.♻️ Proposed change
-/// In-tree BLAKE3 — see `.claude/knowledge/blake3-in-tree-measured.md`. -/// -/// Fully documented, so deliberately NOT under the `#[allow(missing_docs)]` -/// that guards `plane`. An earlier revision declared it on the line directly -/// after that attribute, which silently re-targeted the attribute onto this -/// module and left `plane` unguarded — seven CI jobs failed on `missing_docs` -/// errors in a file the change never touched. +// In-tree BLAKE3 — see `.claude/knowledge/blake3-in-tree-measured.md`. +// +// Fully documented, so deliberately NOT under the `#[allow(missing_docs)]` +// that guards `plane`. An earlier revision declared it on the line directly +// after that attribute, which silently re-targeted the attribute onto this +// module and left `plane` unguarded — seven CI jobs failed on `missing_docs` +// errors in a file the change never touched. pub mod blake3;🤖 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/hpc/mod.rs` around lines 57 - 64, Change the incident-history comment immediately above the public blake3 module declaration from outer documentation comments to ordinary comments, preserving the rationale text without exposing it in generated rustdoc. Keep the pub mod blake3 declaration and its module documentation behavior unchanged.src/simd.rs (1)
771-781: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a count above 64 to pin the documented mod-64 contract.
Every backend documents "
nis taken mod 64", but the largest count tested is exactly 64. Values like 65 and 127 distinguish a correctn % 64from an implementation that only special-cases the width.💚 Proposed test extension
- for n in [0u32, 1, 7, 16, 24, 31, 32, 63, 64] { + for n in [0u32, 1, 7, 16, 24, 31, 32, 63, 64, 65, 127, 128] {🤖 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 771 - 781, Extend the rotation-count cases in the SIMD rotate test around the existing loop to include at least one value greater than 64, such as 65 or 127. Keep the per-lane rotate_left/rotate_right assertions and the rotate-right/rotate-left identity checks unchanged so the test verifies the documented mod-64 behavior.src/hpc/blake3_test_vectors.json (1)
1-217: 🔒 Security & Privacy | 🔵 TrivialSecret-scanner hits here are false positives; consider an allowlist entry.
The
generic-api-keyfindings on everyhash/keyed_hash/derive_keyline are long hex strings from the upstream published BLAKE3 test-vector file — not credentials. Thekeyat Line 3 is the well-known public test key. No change needed to the data, but adding this path to the scanner's allowlist will stop the file from generating ~70 high-severity findings on every future scan.🤖 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/hpc/blake3_test_vectors.json` around lines 1 - 217, Add src/hpc/blake3_test_vectors.json to the secret scanner’s allowlist, targeting the scanner configuration or ignore-path mechanism used for false-positive suppression. Preserve the published BLAKE3 test-vector data and ensure all hash, keyed_hash, and derive_key findings in this file are excluded from future scans.Source: Linters/SAST tools
src/simd_avx2.rs (1)
1559-1590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe portable
U64x8rotate body is byte-for-byte triplicated across three backends. The samen % 64guard,to_array/loop/from_arrayshape, and (in two of the three files) the same multi-paragraph doc block are copy-pasted, so a future fix to the rotate contract has to be applied three times or the arms silently diverge. Optional, but a shared macro or a single portable helper the backends re-export would collapse it.
src/simd_avx2.rs#L1559-L1590: extract the two bodies into the shared portable definition and keep only the AVX2-specific doc note here.src/simd_scalar.rs#L543-L575: replace the duplicatedimpl U64x8block with the shared definition; this file's doc block is identical to the AVX2 one and should live in one place.src/simd_nightly/u_word_types.rs#L22-L62: same substitution, retaining this tier's distinct "codegen unmeasured on nightly" note.🤖 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 1559 - 1590, Centralize the duplicated U64x8::rotate_left and rotate_right implementations in one shared portable definition, preserving the existing n % 64 handling and lane-wise behavior. In src/simd_avx2.rs:1559-1590, src/simd_scalar.rs:543-575, and src/simd_nightly/u_word_types.rs:22-62, replace the duplicated impl blocks with the shared definition; retain only each backend’s distinct local documentation notes, including the AVX2-specific note and nightly “codegen unmeasured” note.
🤖 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 @.claude/board/EPIPHANIES.md:
- Around line 68-75: Update the guidance around the reverse cargo tree check to
require validating that <X> is a real package in the selected dependency graph
before interpreting the command result. Only a successful reverse-tree query
showing no path should unblock the work; distinguish invalid or absent
package-selection errors from evidence that no cycle exists, while retaining the
cycle-cut requirement when a dependency path is found.
In @.claude/knowledge/blake3-ab-bench/run.sh:
- Around line 13-15: Update the setup and cleanup logic around the blake3_ab
example copy to abort before overwriting an existing
$REPO/examples/blake3_ab.rs. Track whether the script created the examples
directory, and have the EXIT trap remove only that newly created directory while
preserving pre-existing directories and files.
In @.claude/knowledge/blake3-in-tree-measured.md:
- Around line 115-123: Update the “What this means for the swap” section to
remove the claim that the in-tree swap removes the C-build question, since the
blake3 pure feature resolved that independently. Retain the statements about
removing the cargo cycle and 2,910 lines of core::arch, and preserve the
trade-off framing.
In @.claude/knowledge/crypto-lane-status.md:
- Around line 109-117: Update the implementation guidance in
crypto-lane-status.md to remove the prescription for AVX2, NEON, and wasm to use
explicit u64 shift-or operations. Mark that form as future investigation or
measurement only, consistent with the evidence that LLVM folds
blake2b_g_shiftor_u64x8 with the rotate implementation and that those backends
currently use per-lane implementations.
In @.claude/knowledge/simd-codegen-oracle/probes.rs:
- Around line 522-585: Add Rustdoc examples to the public functions
shiftor_rot_u64x8, shiftor_rot_const_u64x8, arx_node4096, and arx_lane512_x8.
Each example should demonstrate constructing the required inputs, invoking the
function, and validating or otherwise showing the returned result, while
preserving the existing probe behavior and signatures.
- Around line 513-525: Update shiftor_rotr_u64x8 and its public
shiftor_rot_u64x8 path to normalize n to the u64 rotation width using n % 64,
return the original input when the normalized count is zero, and use the
normalized count for both shifts. Preserve correct behavior for counts 0, 1, 63,
64, and larger arbitrary u32 values.
In `@src/hpc/blake3.rs`:
- Around line 353-375: Update src/hpc/blake3.rs lines 353-375 in Hash::eq
documentation to describe core::hint::black_box as a best-effort optimizer hint
rather than guaranteed constant-time behavior, and remove the misleading
volatile-read inline comment. Update src/hpc/blake3.rs lines 609-615 to state
that the constant_time_eq dependency was not transcribed, while the
constant-time comparison is implemented by impl PartialEq for Hash.
- Around line 575-594: Augment the `///` documentation for `hash`, `keyed_hash`,
`derive_key`, and every public method on `Hasher` and `OutputReader` with
Rustdoc `# Examples` blocks. Each example should use the public API, demonstrate
a representative call and result, and compile as a doctest without changing the
implementations.
- Around line 629-675: Extend the vector test coverage in Case and parse_cases
to retain the JSON derive_key field as derive_key_hex, advancing the parser past
it for each case. In the existing vector test loop, use the vectors’ context
string with derive_key (or Hasher::new_derive_key) and assert the computed
output matches the decoded derive_key vector, preserving the input_len failure
context.
---
Nitpick comments:
In `@src/hpc/blake3_test_vectors.json`:
- Around line 1-217: Add src/hpc/blake3_test_vectors.json to the secret
scanner’s allowlist, targeting the scanner configuration or ignore-path
mechanism used for false-positive suppression. Preserve the published BLAKE3
test-vector data and ensure all hash, keyed_hash, and derive_key findings in
this file are excluded from future scans.
In `@src/hpc/blake3.rs`:
- Around line 590-594: Remove the redundant .into() from the return expression
in derive_key, returning the dereferenced hasher.finalize().as_bytes() array
directly while preserving the existing [u8; OUT_LEN] result.
- Around line 539-569: Cache the compressed output block in OutputReader, keyed
by block_counter, and reuse its block_bytes while fill consumes remaining bytes;
recompress only when advancing to a new block. Update Output::root_output_bytes
to use the shared OutputReader path instead of duplicating word-to-little-endian
conversion, preserving existing output bytes and position behavior.
In `@src/hpc/mod.rs`:
- Around line 57-64: Change the incident-history comment immediately above the
public blake3 module declaration from outer documentation comments to ordinary
comments, preserving the rationale text without exposing it in generated
rustdoc. Keep the pub mod blake3 declaration and its module documentation
behavior unchanged.
In `@src/simd_avx2.rs`:
- Around line 1559-1590: Centralize the duplicated U64x8::rotate_left and
rotate_right implementations in one shared portable definition, preserving the
existing n % 64 handling and lane-wise behavior. In src/simd_avx2.rs:1559-1590,
src/simd_scalar.rs:543-575, and src/simd_nightly/u_word_types.rs:22-62, replace
the duplicated impl blocks with the shared definition; retain only each
backend’s distinct local documentation notes, including the AVX2-specific note
and nightly “codegen unmeasured” note.
In `@src/simd.rs`:
- Around line 771-781: Extend the rotation-count cases in the SIMD rotate test
around the existing loop to include at least one value greater than 64, such as
65 or 127. Keep the per-lane rotate_left/rotate_right assertions and the
rotate-right/rotate-left identity checks unchanged so the test verifies the
documented mod-64 behavior.
🪄 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: 8846d7dd-883e-410c-aeb5-351a4252eb15
📒 Files selected for processing (17)
.claude/board/EPIPHANIES.md.claude/knowledge/blake3-ab-bench/blake3_ab.rs.claude/knowledge/blake3-ab-bench/run.sh.claude/knowledge/blake3-in-tree-measured.md.claude/knowledge/chacha20-vendoring-blast-radius.md.claude/knowledge/crypto-lane-status.md.claude/knowledge/simd-codegen-oracle/baseline-x86_64-v3.toml.claude/knowledge/simd-codegen-oracle/probes.rs.claude/knowledge/the-simd-ladder.mdsrc/hpc/blake3.rssrc/hpc/blake3_test_vectors.jsonsrc/hpc/mod.rssrc/simd.rssrc/simd_avx2.rssrc/simd_avx512.rssrc/simd_nightly/u_word_types.rssrc/simd_scalar.rs
| /// Explicit shift-or u64 rotate, runtime-variable amount, 8 lanes. | ||
| #[inline(never)] | ||
| pub fn shiftor_rot_u64x8(v: U64x8, n: u32) -> U64x8 { | ||
| shiftor_rotr_u64x8(v, n) | ||
| } | ||
|
|
||
| /// Explicit shift-or u64 rotate with COMPILE-TIME-CONSTANT amounts — BLAKE2b's | ||
| /// 32/24/16/63. Separated from the runtime-variable probe above because the | ||
| /// u32 lane vectorizes constants and variables differently (`vpshufb` for | ||
| /// byte-granular constants, shift-or otherwise), so the two cases must be | ||
| /// distinguished rather than averaged. | ||
| #[inline(never)] | ||
| pub fn shiftor_rot_const_u64x8(v: U64x8) -> U64x8 { | ||
| let a = shiftor_rotr_u64x8(v, 32); | ||
| let b = shiftor_rotr_u64x8(a, 24); | ||
| let c = shiftor_rotr_u64x8(b, 16); | ||
| shiftor_rotr_u64x8(c, 63) | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Group E — UNKNOWN. Is 4096 bit / 512 byte a viable DEFAULT lane? | ||
| // ============================================================================ | ||
| // The substrate's canonical node is 4096 bit = 512 byte | ||
| // (`key(16) | edges(16) | value(480)`). `U32x16` is 512 BIT — one eighth of | ||
| // it. So the question is whether a node-wide unit, `[U32x16; 8]` = 128 u32 | ||
| // lanes, stays fully packed and amortizes better than looping eight times | ||
| // over `U32x16`. | ||
| // | ||
| // It cannot be a register type: 512 byte is 8 zmm / 16 ymm, so a node-wide | ||
| // lane is necessarily a TILE over the existing lane, exactly as `U32x16` is | ||
| // already a polyfill over 2 ymm on avx2. The question is therefore not "does | ||
| // it fit" but "does the wider unit cost anything" — if the 8x form emits the | ||
| // same instruction count as 8 separate ops with no extra spill, the node | ||
| // width is free and can be the default unit; if it spills, it cannot. | ||
| // | ||
| // 128 u32 of live state is 8 zmm or 16 ymm against 16 architectural | ||
| // registers, so spill is the thing to watch — the same pressure that showed | ||
| // up as 41 memory ops in `transpose_16x16_composed`. | ||
|
|
||
| /// One ARX triple over a NODE-WIDE unit: `[U32x16; 8]` = 4096 bit = 512 byte. | ||
| #[inline(never)] | ||
| pub fn arx_node4096(a: [U32x16; 8], b: [U32x16; 8]) -> [U32x16; 8] { | ||
| let mut out = [U32x16::splat(0); 8]; | ||
| for i in 0..8 { | ||
| out[i] = ((a[i] + b[i]) ^ b[i]).rotate_left(16); | ||
| } | ||
| out | ||
| } | ||
|
|
||
| /// The SAME work as `arx_node4096`, expressed as the existing 512-bit lane | ||
| /// applied eight times by the caller — the baseline it must not lose to. | ||
| #[inline(never)] | ||
| pub fn arx_lane512_x8(a: [U32x16; 8], b: [U32x16; 8]) -> [U32x16; 8] { | ||
| [ | ||
| ((a[0] + b[0]) ^ b[0]).rotate_left(16), | ||
| ((a[1] + b[1]) ^ b[1]).rotate_left(16), | ||
| ((a[2] + b[2]) ^ b[2]).rotate_left(16), | ||
| ((a[3] + b[3]) ^ b[3]).rotate_left(16), | ||
| ((a[4] + b[4]) ^ b[4]).rotate_left(16), | ||
| ((a[5] + b[5]) ^ b[5]).rotate_left(16), | ||
| ((a[6] + b[6]) ^ b[6]).rotate_left(16), | ||
| ((a[7] + b[7]) ^ b[7]).rotate_left(16), | ||
| ] | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add examples to the new public probe APIs.
shiftor_rot_u64x8, shiftor_rot_const_u64x8, arx_node4096, and arx_lane512_x8 have doc comments but no examples. 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 @.claude/knowledge/simd-codegen-oracle/probes.rs around lines 522 - 585, Add
Rustdoc examples to the public functions shiftor_rot_u64x8,
shiftor_rot_const_u64x8, arx_node4096, and arx_lane512_x8. Each example should
demonstrate constructing the required inputs, invoking the function, and
validating or otherwise showing the returned result, while preserving the
existing probe behavior and signatures.
Source: Coding guidelines
| /// The default hash function. | ||
| pub fn hash(input: &[u8]) -> Hash { | ||
| let mut hasher = Hasher::new(); | ||
| hasher.update(input); | ||
| hasher.finalize() | ||
| } | ||
|
|
||
| /// The keyed hash function. | ||
| pub fn keyed_hash(key: &[u8; KEY_LEN], input: &[u8]) -> Hash { | ||
| let mut hasher = Hasher::new_keyed(key); | ||
| hasher.update(input); | ||
| hasher.finalize() | ||
| } | ||
|
|
||
| /// The key-derivation function. | ||
| pub fn derive_key(context: &str, key_material: &[u8]) -> [u8; OUT_LEN] { | ||
| let mut hasher = Hasher::new_derive_key(context); | ||
| hasher.update(key_material); | ||
| (*hasher.finalize().as_bytes()).into() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Public API doc comments lack examples.
hash, keyed_hash, derive_key, and the public Hasher / OutputReader methods (Lines 327, 430-517, 542) all have /// prose but no example blocks. As per coding guidelines, "All public APIs (public functions and methods) must have /// doc comments with examples". Doctests here would also double as always-run smoke tests for the public surface.
📝 Example for `hash`
/// The default hash function.
+///
+/// ```
+/// use ndarray::hpc::blake3::hash;
+///
+/// let h = hash(b"abc");
+/// assert_eq!(h.as_bytes().len(), 32);
+/// assert_eq!(h, hash(b"abc"));
+/// ```
pub fn hash(input: &[u8]) -> Hash {🤖 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/hpc/blake3.rs` around lines 575 - 594, Augment the `///` documentation
for `hash`, `keyed_hash`, `derive_key`, and every public method on `Hasher` and
`OutputReader` with Rustdoc `# Examples` blocks. Each example should use the
public API, demonstrate a representative call and result, and compile as a
doctest without changing the implementations.
Source: Coding guidelines
…iming doc Addresses the CodeRabbit pass on #268. Taking the findings that hold and saying which do not, rather than applying all nine. FIXED — correctness: - **`derive_key` had zero vector coverage.** The parser read `input_len`, `hash`, and `keyed_hash` and skipped past the `derive_key` field already present in the file. It is the one public mode built as TWO passes (DERIVE_KEY_CONTEXT over the context string, whose output keys a DERIVE_KEY_MATERIAL pass over the material), so a swapped or collapsed transcription would have been invisible while hash/keyed_hash stayed green. Now asserted for all 35 cases at both the 32-byte and extended XOF lengths. It passed on the first run — the transcription was right; it just wasn't proven. Control-tested: swapping the two flags fails ONLY the new assertion. - **`Hash::eq`'s documentation described the opposite of the code.** The doc-comment said `read_volatile` (the code uses `core::hint::black_box`) and the not-transcribed list flatly asserted "`PartialEq` here is a normal short-circuiting byte-array compare" — stale since the constant-time fold landed. On a MAC-comparison path a backwards timing claim is worse than no claim: an auditor would have believed this file leaks match-prefix length when it does not. Also now states that `black_box` is a best-effort barrier, not a guarantee; the data-independent loop is the actual property. FIXED — hazards: - `run.sh` copied the bench to a fixed `examples/blake3_ab.rs` and had the EXIT trap `rm` it unconditionally: a pre-existing file there would be clobbered and then deleted. Now refuses to overwrite, and only removes the directory it created. - `shiftor_rot_u64x8` is `pub` and took arbitrary `u32` into `a >> n` — a debug panic and a release wrapping-shift for `n >= 64`. Normalized at the PUBLIC wrapper and deliberately NOT in the measured inner body, which would have changed the codegen the oracle exists to observe. - Rotate test extended past 64 (65/127/128/191) against std's `u64::rotate_*`, pinning the mod-64 contract rather than only the width case. - Test-local context const renamed `VECTORS_CONTEXT`; as `DERIVE_KEY_CONTEXT` it shadowed the module-scope u32 flag of that name. - Redundant `.into()` in `derive_key` (`clippy::useless_conversion`). - The `pub mod blake3` incident history was an OUTER doc comment, so rustdoc would publish it as module documentation. Now a plain `//` comment. Three more stale doc claims, all the same class already corrected elsewhere in this PR — an assertion outliving the change that falsified it: - EPIPHANIES said "an error there means no cycle and the work is unblocked" one paragraph after explaining the error is ambiguous. Read literally it marks blocked work unblocked on the strength of a typo. Now a four-step procedure with the error explicitly inconclusive. - crypto-lane-status prescribed an explicit shift-or for AVX2/NEON/wasm, contradicting the measurement directly above it (LLVM folds the two forms byte-identically) and misdescribing what shipped. - blake3-in-tree-measured still credited the swap with removing the C build, already removed by `features = ["pure"]` in #264. NOT taken, with reasons: - Doctest examples on every public API. Worth doing, but it is a separate change across a surface this PR is not otherwise touching, and the vectors already smoke-test the public path harder than an example would. - Caching the compressed block in `OutputReader`. A real inefficiency for small repeated `fill` calls, but unmeasured, and the module is explicitly not on a hot path pending the swap ruling. - Deduplicating the rotate body across backends. Every other lane type in this crate is duplicated per backend by design; collapsing one method would make it the exception, not the rule. - Secret-scanner allowlist for the vectors file. Correct that the hex is published test data, not credentials, but no scanner config lives in this repo to add it to. Full lib suite green: 2200 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
2 participants
The plan for making "all SIMD lives once, audited, inside
ndarray::simd" true rather than aspirational — plus the two rungs that landed while the plan was under review.What ships
the-simd-ladder.md,blake3-in-tree-measured.md,crypto-lane-status.md, the cycle-map epiphanysrc/hpc/blake3.rs— portable BLAKE3, 771 lines, nounsafe, nocore::arch, no new depsU64x8::rotate_left/rotate_right— the one earned intrinsic overrideThe finding that orders everything
Whether a dependency can consume
ndarray::simdis decided by which package in the workspace pulls it, not by the workspace as a whole:chacha20crates/encryption→ … →chacha20curve25519-dalekcrates/encryption→ed25519-dalek→ …blake3ndarray(stdfeature, 11 files insrc/hpc/)Only blake3 is pulled by the root package, so only blake3 closes a loop. The intuitive rule — "ndarray depends on X, so X can't depend on ndarray" — is wrong at workspace granularity, and reasoning that way would have parked two rungs with no blocker at all.
The evidence is the positive reverse tree with a known-hit control beside it:
The original body proved this with
cargo tree -p ndarray -i curve25519-dalekreturning "did not match any packages". That was weak evidence and is retracted — a mistyped crate name produces the byte-identical message, so the error cannot distinguish "no such edge" from "no such package" (caught by CodeRabbit on this PR). Likewise the original body's[[patch.unused]]framing conflated "patch not selected" with "cycle", which teaches the next reader to misdiagnose a stale patch (caught by codex).Rung 3a — portable BLAKE3, correct, and what it costs
ndarray's blake3 usage is entirely single-input — no
hash_many— so cutting the cycle needs only the portable core, which uses no SIMD at all.Correct: 35/35 official test vectors, unkeyed and keyed, each at both 32-byte and extended XOF length (140 assertions), inputs 0…102,400. Plus streaming, empty input, and incremental
fill.Hash::eqis constant-time (XOR-fold +black_box), matching upstream'sconstant_time_eqwithout adding the dep.And it is not free — measured, not assumed:
blake3crateVSA_BYTES)ndarray's call sites all sit in the 1.3× band, not the 5× one. An
array_chunksfast path (the operator's lead, citing the blasgraph JIT-gap precedent) removed a double copy worth −21 % at 2 KB — real, and bounded: it buys nothing at ≤ 1 block, which falsified my own guess that the staging copy explained the small-input gap.The call sites are NOT swapped in this PR. The module lands and is tested; flipping the 11 call sites is a decision with a measured price tag and it is the operator's.
The original body claimed 3a "removes the C build question". That is false —
features = ["pure"]already removed all C/ASM compilation in #264. Overstating the benefit matters here specifically, because what it is being weighed against is transcribing a cryptographic implementation.Rung 6 — the u64 ARX rotate
The oracle measured that a scalar u64 rotate loop does not vectorize, across three spellings (
u64::rotate_right, runtime shift-or, and shift-or with BLAKE2b's constants 32/24/16/63): 0 packed, onerorqper lane, on a target that emitsvpsllq/vpsrlqfor the u32 lane. LLVM folded two byte-identical probes together — it declines the 64-bit operation, not the rotate idiom.That is the entry criterion in
simd-one-spec-design.md, and this is the only place in the crate that meets it. What each backend got is not uniform:_mm512_rolv_epi64/_mm512_rorv_epi64— the overrideU64x8The explicit
vpsllq/vpsrlqshift-or for avx2 is not written.nis taken mod 64 with ann == 0short-circuit, so the portable and native arms agree at every input rather than only in the interior.Test covers 0/1/7/16/24/31/32/63/64 and is control-tested — flipping
rotate_right's body torotate_leftmakes it fail, so it discriminates rather than passing vacuously.The rungs
U32x16ARXU32x16shuffle surfacehash_manythroughput_mm*under 52unsafe, cfg'd out; no cycle, gated on appetiteWhat is NOT claimed
serialis correct and deliberate — X25519's Montgomery ladder never touches the vector backend.Five items needing a ruling, not work
Fork-vs-vendored for
vendor/chacha20; whether the Docker images should buildencryptionat all; a CI job for the chacha20 v4 arm;Cargo.toml:481's comment; and theblake3+curve25519-dalekcrates.io-vs-fork P0 violations (3a dissolves the blake3 half by removing the dependency).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation