oracle: repair the run script, then use it — chacha20 reach and the BLAKE3 shuffle gap - #265
Conversation
The move from crates/ + scripts/ into .claude/knowledge/ (#264) rewrote the doc references but not the script's own path arithmetic. As merged, run.sh resolved ROOT to .claude/knowledge/ and pointed at crates/simd-codegen-oracle/Cargo.toml and scripts/codegen_oracle_analyze.py -- neither of which exists. The tool shipped unusable. run.sh is now self-contained: it derives the repo root from its own location, materializes a throwaway crate from probes.rs under TMPDIR, cds into the repo before invoking cargo (config.toml resolves from the working directory, not from --manifest-path), and removes the scratch tree on exit. The declared -C target-cpu baseline is unchanged and still lands on the final rustc invocation, so ambient RUSTFLAGS cannot drop it. Verified end to end: 13/13 probes match baseline-x86_64-v3.toml. Also repaired two doc casualties of the same relocation: the README's run instructions were the pre-move manual recipe with placeholder paths, and simd-one-spec-design.md had a shell block whose commands were the literal text "see .../README.md", with a stray "What this buys" bullet stranded beneath it.
|
Warning Review limit reached
Next review available in: 48 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 (6)
📝 WalkthroughWalkthroughThe PR adds BLAKE3 SIMD codegen probes, rewrites the SIMD oracle runner around temporary crates and target baselines, updates related usage documentation, and adds measured knowledge documents covering BLAKE3 portability and ChaCha20 vendoring reach. ChangesSIMD oracle and BLAKE3 analysis
ChaCha20 vendoring reach
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
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_a586774f-2e8b-4f48-98ad-b16cdedb3e7a) |
The vendored chacha20 tree is described in three manifests as accelerating consumers' XChaCha20-Poly1305 keystreams. Measured, it reaches far less than that, along two independent axes that had been conflated. Patch reach: [patch.crates-io] applies only from a top-level workspace manifest, so ndarray's patch does not travel with ndarray. Two repos link the vendored source -- ndarray itself and MedCare-rs, which declares its own. lance-graph, OGAR, tesseract-rs, stockfish-rs and a2ui-rs link the registry crate. Backend reach: the added ndarray_simd backend is gated on the compile-time avx512f feature cfg. The workspace pins x86-64-v3, which supplies avx2 and not avx512f, and MedCare-rs pins nothing at all. So no default build of any repo here compiles that backend -- only an opt-in v4 build or wasm32+simd128 does. The fallthrough to RustCrypto's own backends is correct and MedCare's comment describes it accurately; the scope simply keeps being restated one size too large. Also records that AdaWorldAPI/stream-ciphers -- the fork the P0 rule names -- is a single-commit mirror of upstream at chacha20 0.10.1 carrying no AdaWorldAPI delta and no ndarray backend, while the vendored tree is a hand copy of 0.9.1 with its repository field rewritten. The two are a cipher major apart, and upstream 0.10.1 now ships its own avx512 backend in the niche ndarray_simd was written to fill. One inaccuracy is recorded, not fixed: the root manifest attributes target-cpu=x86-64-v4 to the workspace, which pins v3. Manifest string edits need a ruling, so the finding is filed instead of applied.
|
@coderabbitai review Draft is intentional (repo default for agent-opened PRs), but this is docs-and-tooling only and a review is welcome. Since the last push the PR also carries a second commit: a measurement audit of what Worth pointing the review at two things specifically:
Generated by Claude Code |
|
✅ Action performedReview finished.
|
Answers "what is missing to express BLAKE3 on ndarray::simd instead of raw
intrinsics" with four probes rather than with intuition.
Census first: BLAKE3's pure-Rust AVX2 backend uses 15 distinct intrinsics.
Twelve are already expressible on U32x16 -- add, xor, or, the shift pair
inside rotate_left, splat, set, load, store. The other three families
(unpacklo/hi_epi32, unpacklo/hi_epi64, permute2x128 -- 18 call sites) exist
solely to build hash_many's transpose, and U32x16 has no shuffle surface at
all.
Measured, at x86-64-v3:
blake3_g_u32x16 72 packed / 0 scalar rotr16,rotr8 -> vpshufb;
rotr12,rotr7 -> shift-or
interleave_lo_u32x16 11 packed / 0 scalar vpermd + vpblendd
transpose_16x16_u32 0 packed / 47 memory 1088B stack, 256-iter copy
transpose_stage_u32x16 27 packed / 0 scalar vunpcklps / vunpckhps
The compression core is free today. The interleave primitive is also free --
a plain scalar index loop compiles to a genuine two-source cross-lane
permute, extending cross_lane_reverse_u8x64's single-source result. What
fails is only the monolithic nested-index-loop spelling of the transpose,
where the stride-64 scatter defeats the vectorizer; the same transpose
composed from interleave calls emits literally the unpack family the
intrinsic backend hand-writes.
So no intrinsic override is earned under simd-one-spec-design.md's entry
criterion. The gap is a method surface on U32x16 -- five interleave/permute
methods, each a scalar index loop, no unsafe and no core::arch -- with the
transpose written as a composition of them rather than as an index loop.
Findings doc records the census, the four measurements, the scope limits
(degree 16 not 8; codegen shape not transpose correctness; x86_64 only; no
throughput benchmark) and the three questions still open.
AdaWorldAPI
changed the title
|
@coderabbitai review Retrying past the rate limit. The PR is out of draft now, so this should be the last manual trigger. Generated by Claude Code |
|
✅ Action performedReview finished.
|
![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: 3e72c5d525
ℹ️ 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".
| [dependencies] | ||
| ndarray = { path = "$REPO", default-features = false, features = ["std"] } |
There was a problem hiding this comment.
When the checkout is used in an offline or minimally cached environment, this independent scratch package has no lockfile, so Cargo attempts to update the crates.io index before compiling; running the documented command here failed while resolving blake3, despite the repository already having a committed Cargo.lock. This also lets dependency resolution drift between oracle runs, so the scratch build should reuse the repository's locked dependency graph rather than resolving afresh.
Useful? React with 👍 / 👎.
| case "$TARGET" in | ||
| x86_64-*) BASELINE="$HERE/baseline-x86_64-v3.toml"; CPU="x86-64-v3" ;; | ||
| *) BASELINE="$HERE/baseline-$TARGET.toml"; CPU="" ;; |
There was a problem hiding this comment.
For any cross-target whose triple starts with x86_64-—for example x86_64-pc-windows-msvc—this branch silently selects the Linux-host baseline-x86_64-v3.toml instead of requiring baseline-<triple>.toml as the README promises. Target-specific assembly syntax, ABI instructions, and counts can therefore produce meaningless comparisons or missing-symbol failures; only the target for which this baseline was measured should select it, and other triples should require their own baseline.
Useful? React with 👍 / 👎.
| ### …but the composed transpose is | ||
|
|
||
| Same transpose, expressed as eight pairwise interleave calls per butterfly | ||
| stage: **27 packed, and the instructions are `vunpcklps` / `vunpckhps`** — |
There was a problem hiding this comment.
For a future BLAKE3 backend relying on this result, the 27-instruction probe is not the “same transpose”: transpose_stage_u32x16 executes only one 32-bit interleave stage, and its own documentation acknowledges that repeating these helpers does not produce a correct transpose. It also never exercises the required 64-bit interleaves or 128-bit half permutation, so the measurement cannot establish that the complete composition stays packed or justify the subsequent “no intrinsic is earned” conclusion; a correct full transpose using all required primitive shapes needs to be probed.
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: 6
🤖 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/chacha20-vendoring-blast-radius.md:
- Around line 3-4: The audit’s evidence provenance is incomplete: update the
opening status statement in .claude/knowledge/chacha20-vendoring-blast-radius.md
lines 3-4 to either include Git/source-tree comparisons or narrow the listed
sources to Cargo.lock, manifests, and rustc cfg output; update lines 127-129 to
record the exact upstream commit/tree hash or a reproducible diff supporting the
“same bytes” conclusion, rather than relying only on a head commit and branch
count.
- Around line 69-74: Specify a language identifier on the fenced code block
containing the rustc commands, using console or text, while preserving its
command output and explanatory comment.
In @.claude/knowledge/simd-codegen-oracle/probes.rs:
- Around line 416-424: The current probe does not validate the complete BLAKE3
degree-16 transpose. In .claude/knowledge/simd-codegen-oracle/probes.rs:416-424,
replace or extend transpose_stage_u32x16 with a semantically correct full
transpose network, including required u64 interleave and half-permutation
stages, and verify it against a scalar reference. In
.claude/knowledge/simd-codegen-oracle/baseline-x86_64-v3.toml:148-154, restrict
conclusions to the measured non-semantic stage until that probe exists. In
.claude/knowledge/blake3-on-ndarray-simd.md:113-142, qualify the “no intrinsic
is earned” conclusion and proposed method surface as pending validation of the
complete transpose network.
In @.claude/knowledge/simd-codegen-oracle/run.sh:
- Around line 40-42: Replace the predictable SCRATCH path and separate mkdir -p
setup with an atomic mktemp -d allocation, preserving the existing cleanup trap
and ensuring the required src subdirectory is created only after the unique
scratch directory exists.
- Around line 81-93: Update both cargo rustc invocations in the build logic
around the host and cross-target branches to explicitly set Cargo’s target
directory to $SCRATCH/target. Preserve the existing release, manifest, target,
and assembly-emission options while ensuring artifacts are always written under
the scratch tree for ASM discovery and cleanup.
In @.claude/knowledge/simd-one-spec-design.md:
- Around line 196-197: Update the command examples in the SIMD codegen oracle
documentation so the aarch64 invocation is clearly marked as a post-baseline
example or remove it; retain the runnable x86-64-v3 command and do not present
aarch64 as currently supported while no aarch64 baseline exists.
🪄 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: dd44f880-81c4-4d48-b50f-c31c7a2a8fce
📒 Files selected for processing (7)
.claude/knowledge/blake3-on-ndarray-simd.md.claude/knowledge/chacha20-vendoring-blast-radius.md.claude/knowledge/simd-codegen-oracle/README.md.claude/knowledge/simd-codegen-oracle/baseline-x86_64-v3.toml.claude/knowledge/simd-codegen-oracle/probes.rs.claude/knowledge/simd-codegen-oracle/run.sh.claude/knowledge/simd-one-spec-design.md
Six review findings from codex and coderabbit on #265. Five are fixes; one was a real methodological error on my part. THE SUBSTANTIVE ONE. The BLAKE3 conclusion rested on transpose_stage_u32x16, which is ONE 32-bit butterfly stage whose whole-vector helper semantics do not compose into a real transpose -- it never exercised the 64-bit unpack or the 128/256-bit permutes, so "the composed transpose stays packed" was an inference wearing a measurement's clothes. Exactly the failure mode TD-T22 exists to prevent, committed in the document that cites TD-T22. Fixed by measuring it. transpose_16x16_composed is a complete 16x16 transpose built from a const-generic exchange<G> at G = 1/2/4/8, so all four granularities appear. Measured 79 packed / 0 scalar on lane data, of which 19 are real shuffles: vinserti128 x7, vpshufd x3, vpblendd x3, vpunpcklqdq x2, vpermq x2, vunpcklpd, vpermpd. The conclusion survives, now on evidence. Correctness is checked rather than claimed: the driver compares against a naive nested-loop transpose and aborts on mismatch, and run.sh now EXECUTES the probe binary, because --emit asm links nothing and the assertion would otherwise never have run -- a claim of verification that does not verify is the same defect one level up. Control-tested by deleting stage::<8>, which makes it fire (exit 101). Two caveats added rather than smoothed over: the composed form is not clean straight-line code (38 loop-control, 41 memory spill, since sixteen 512-bit vectors are 32 ymm registers against 16 architectural ones -- a hand-written backend faces the same), and none of this is a throughput comparison against rust_avx2.rs. run.sh hardening: - mktemp -d instead of a PID-derived path; mkdir -p accepts an existing directory, so a predictable name lets a local attacker pre-create it with symlinked children and redirect the cp and heredoc writes. - --target-dir pinned to the scratch tree, so an ambient CARGO_TARGET_DIR cannot put artifacts where the asm discovery and the cleanup will not find them. - Baseline selection is exact-triple, not an x86_64-* glob that silently handed x86_64-pc-windows-msvc the Linux baseline. Verified: that triple now exits 90 with a message saying there is deliberately no family fallback. - The repository Cargo.lock is copied into the scratch crate and the build tries --offline first. A fresh scratch package otherwise hits the crates.io index before compiling anything, which fails on a cold cache, and lets two oracle runs measure two different dependency graphs -- unacceptable in an instrument whose whole job is reproducibility. Docs: - The chacha20 audit's "fork and upstream are the same bytes" line was inferred from a head commit and a branch count. Replaced with the actual check against RustCrypto/stream-ciphers: identical head sha, zero commits ahead, identical chacha20/ tree hash -- and git tree hashes cover content recursively, so that is byte equality. Provenance line widened to name git object hashes as a source, and a fenced block got a language tag. - The aarch64 invocation in simd-one-spec-design.md was presented as runnable when no aarch64 baseline exists and it exits 90; now marked as pending that baseline, which is step 2 of the migration. 17 probes green; scratch cleanup verified.
|
All six findings addressed in The transpose measurement (codex P2, coderabbit 🟠 Major)Both reviews caught the same thing and both were right: the BLAKE3 conclusion rested on The fix is to measure it rather than to hedge the wording.
The conclusion survives, now on evidence. Correctness is now checked rather than claimed. The driver compares the result against a naive nested-loop transpose and aborts on mismatch — and Two caveats are stated in the doc rather than smoothed over. The composed form is not clean straight-line code — 38 loop-control instructions and 41 memory ops of real spill traffic, because sixteen 512-bit vectors of live state are 32 ymm registers against 16 architectural ones. A hand-written intrinsic backend faces exactly the same pressure. And none of this is a throughput comparison against run.sh (coderabbit 🟠 ×2, codex P2)
DocsThe "fork and upstream are the same bytes" line was inferred from a head commit and a branch count, which coderabbit correctly called insufficient. Replaced with the actual check against $ git rev-parse HEAD upstream/master
5f3430b7531a33aa14957b6bd407b46687635124
5f3430b7531a33aa14957b6bd407b46687635124
$ git rev-list --count upstream/master..HEAD
0
$ git rev-parse HEAD:chacha20 upstream/master:chacha20
3f58304be34b79a3a38430a5752fb8db223d1a20
3f58304be34b79a3a38430a5752fb8db223d1a20Git tree hashes cover content recursively, so identical tree ids are byte equality for that directory. The provenance line now names git object hashes as a source alongside The aarch64 invocation in 17 probes green, scratch cleanup verified, no product code touched. Generated by Claude Code |
2 participants
Three commits, docs and tooling only. The first repairs the oracle; the second and third use it.
1 — repair the run script broken by the relocation
#264 moved the codegen oracle from
crates/simd-codegen-oracle/+scripts/into.claude/knowledge/simd-codegen-oracle/. The relocation rewrote the prose references but not the script's own path arithmetic, so the tool merged unusable:ROOTresolved one..short of the repo root,MANIFESTpointed at aCargo.tomlthe same PR deleted, and the analyzer path pointed atscripts/.run.shis now self-contained — it derives the repo root from its own location, materializes a throwaway crate fromprobes.rsunder$TMPDIR, and cleans up on exit. Two properties are load-bearing and deliberate:cd "$REPO"before cargo, because cargo resolves.cargo/config.tomlfrom the working directory rather than from--manifest-path.-C target-cpudeclared on the final rustc invocation, unchanged from blake3: drop the C toolchain requirement; record measured u32/u64 lane codegen #264. Cargo'sRUSTFLAGSreplaces[target.'cfg(…)'].rustflagsrather than merging, so a tool that inherits its baseline measures a different machine depending on where it runs — the exact failure that made the earlier CI attempt reportpacked = 0for every probe.Two docs carried the same damage and are fixed:
README.md's run section was the pre-move manual recipe with placeholder paths, andsimd-one-spec-design.mdhad a shell block whose commands were the literal textsee .../README.md, with a stray bullet from "What this buys" stranded beneath it.2 — what
vendor/chacha20actually reachesThree manifests describe the vendored tree as accelerating consumers' XChaCha20-Poly1305 keystreams. Measured, it reaches less than that, along two axes that had been conflated.
Patch reach.
[patch.crates-io]applies only from a top-level workspace manifest, so ndarray's patch does not travel with ndarray. Two repos link the vendored source — ndarray itself (Cargo.lockcarrieschacha20 0.9.1with nosourceline) and MedCare-rs, which declares its own. lance-graph, OGAR, tesseract-rs, stockfish-rs and a2ui-rs link the registry crate.Backend reach. The added
ndarray_simdbackend is gated on the compile-timeavx512ffeature cfg. The workspace pinsx86-64-v3, which suppliesavx2and notavx512f; MedCare-rs pins nothing at all. So no default build of any repo here compiles that backend — only an opt-in v4 build or wasm32+simd128 does. The fallthrough to RustCrypto's own backends is correct, and MedCare's comment describes it accurately; the scope simply keeps being restated one size too large.The audit also records that
AdaWorldAPI/stream-ciphers— the fork the P0 rule names — is a single-commit mirror of upstream at chacha20 0.10.1 carrying no AdaWorldAPI delta and no ndarray backend, while the vendored tree is a hand copy of 0.9.1 with itsrepositoryfield rewritten. The two are aciphermajor apart, and upstream 0.10.1 now ships its ownavx512backend in the nichendarray_simdwas written to fill.One inaccuracy is recorded, not fixed: the root manifest attributes
target-cpu=x86-64-v4to the workspace, which pins v3. Manifest string edits need a ruling, so the finding is filed rather than applied. Three questions are left explicitly open for that ruling.3 — the BLAKE3 shuffle gap, measured
With C removed from
blake3(features = ["pure"], shipped in #264), what remains is a second SIMD surface: 2,910 lines of rawcore::archacross its Rust backends. The question is what andarray::simd-native backend would need.Census:
rust_avx2.rsuses 15 distinct intrinsics. Twelve are already expressible onU32x16. The other three families —unpacklo/unpackhi_epi32,unpacklo/unpackhi_epi64,permute2x128, 18 call sites — exist solely to buildhash_many's transpose, andU32x16has no shuffle surface at all.Rather than assume that means intrinsics (the assumption TD-T22 already falsified once, at the cost of a 700-line PR), four probes:
blake3_g_u32x16vpshufbfor rotr 16/8, shift-or for rotr 12/7interleave_lo_u32x16vpermd+vpblenddtranspose_16x16_u32transpose_stage_u32x16vunpcklps/vunpckhpsThe compression core is free today, so
compress_in_placeandcompress_xofare a transcription. The interleave primitive is free too — a plain scalar index loop compiles to a genuine two-source cross-lane permute, which extendscross_lane_reverse_u8x64's earlier single-source result to the half that was still open.What fails is only the monolithic index-loop spelling of the transpose, where the stride-64 scatter defeats the vectorizer. The last two rows are the same transpose; composed from interleave calls it emits literally the unpack family the intrinsic backend hand-writes.
So no intrinsic override is earned under
simd-one-spec-design.md's entry criterion. The gap is a method surface onU32x16— five interleave/permute methods, each a scalar index loop, nounsafeand nocore::arch— with the transpose written as a composition of them.The findings doc states its scope limits plainly: degree 16 rather than 8 (per the
U32x8ruling); the staged probe measures codegen shape, not transpose correctness; x86_64 only; and no throughput benchmark — "emits packed shuffles" is not "beatsrust_avx2.rs", and that comparison has not been run.Verification
17/17 probes, including the four new Group D rows. No product code is touched — the whole PR is under
.claude/knowledge/.Summary by CodeRabbit
Documentation
Tools