simd: complete the crate-native f64 GEMM — gemm_f64_tiled ref + FMA tiers, own engine behind backend::native::gemm_f64#237
Merged
Conversation
…pel_tap_tile (#235) Review-only session. fmt/clippy/2175 lib tests green; example runs on AMX (TDPBF16PS tier, asserts pass). Findings recorded: unguarded Gotcha-14 asserts, throughput leg measures pack/alloc overhead (PackedBf16B is the fix), false positive-operand comment, SplitMix64/gemm_f64 reuse (gemm_f64 measured bit-exact vs naive reference, 0/256 lanes differ), misc cleanups. Refuted: .gitattributes-accidental, needless_range_loop, checksum, transpose reuse.
…GEMM surface is external-backed Operator challenge answered with measurement: naive reference vs the crate's own scalar gemm_f64_tiled (native.rs:473) is bit-exact (0/256 lanes, probe operands and random f64 K=64), same as vs matrixmultiply. Structural gap surfaced: gemm_f64_tiled is dead code in private mod scalar while every public f64 GEMM path delegates to the external matrixmultiply crate.
…imd surface Graduates the tiled f64 GEMM from dead code (backend/native.rs private mod scalar, zero callers) to src/simd_ops.rs, re-exported unconditionally via ndarray::simd. Innermost j-loop vectorized on the dispatched F64x8 — one source serving AVX-512/AVX2/NEON/WASM-SIMD128/scalar. Bit-exactness contract (documented): per-element c = c + (alpha*a)*b in ascending-k order, mul and add unfused on every backend — bit-identical across backends, and at alpha=1 beta=0 bit-identical to the naive triple-loop reference. This makes it the in-crate ground-truth GEMM for probe certification, replacing reliance on the external matrixmultiply engine behind backend::native::gemm_f64. W1a: 7 parity tests (fixed-seed corpus, 13 shapes incl. multi-tile 70^3, strided leading dims + sentinel padding, alpha/beta semantics, beta=0 over NaN, k=0, m/n=0, denormals) — all to_bits equality. Zero unsafe. Bench vs naive scalar (single thread, EMR): 128^3 2.23x, 256^3 2.54x, 512^3 6.74x, bit-equal at every size. Consumer sites: direct_matmul in examples/subpel_tap_tile.rs and examples/gridlake_field_tile.rs rewired to it (bit-identical swap — identical printed output pre/post). Dead native.rs copy removed. Gates: clippy -D warnings clean (lib + examples), 2182/2182 lib tests, doctest green, no_std build green.
Three-angle verify (IEEE semantics / W1a compliance / regression trace) returned PASS on all angles; this folds in the surviving findings: - P1: correct the simd.rs re-export comment — pub mod simd/simd_ops are std-gated in lib.rs, so the kernel (though alloc-free) is reachable only in std builds; the prior comment claimed no_std availability. - Scope the cross-backend bit-identity claim to non-NaN inputs (NaN payload propagation is backend-defined; WASM may canonicalize). - Document alpha == 0.0 non-short-circuit semantics (a/b still read, 0*Inf = NaN propagates, -0.0 in c can flip — unlike BLAS quick-return). - Panics doc: state that all checks are skipped at m/n == 0 and the a/b length checks at k == 0. - Overflow-check the length-extent asserts (checked_mul) so a wrapping (rows-1)*ld cannot spuriously pass the precondition in release. - Widen the parity corpus to 70+ kernel invocations (full 13-shape sweep x 4 alpha/beta combos) to meet W1a criterion 3's 50+ letter. - Cite the free-fn GEMM-family precedent in the doc-comment for the simd-savant pre-merge audit. Gates re-run: clippy -D warnings clean, 7/7 gemm_f64_tiled tests, doctest green.
The earlier 'XTILEDATA enablement drift' guess was wrong. amx_probe shows CPUID TILE/INT8/BF16 all false and cpu_model() = OtherX86 (morning run: EMR 0xCF with AMX) — the session container was rescheduled onto non-AMX silicon. Not Gotcha 4 (has_amx()==true/available==false signature), not Gotcha 14 (corruption while available). Gotcha 9 tier-printing surfaced it. Rule recorded: re-probe before any AMX-tier claim in remote sessions; never carry amx_available() across runs.
…ne swap Completes the f64 GEMM family on the canonical surface: 1. gemm_f64_tiled_fma — fast fused tier via const-generic gemm_f64_tiled_impl<const FMA: bool> (monomorphized). Same tiling and ascending-p per-element order; each step is one fused rounding (F64x8::mul_add / f64::mul_add). Bit-identical to the reference tier on integer-valued operands (asserted across the full shape sweep); last-ulp-per-step on general floats (tolerance scaled k*eps to the summand magnitude). Cross-backend fusion caveats documented. 2. Register-resident C row-block (both tiers): the C block is loaded once into [F64x8; TILE/LANES] accumulators plus a scalar tail array, the whole kb-loop accumulates in registers, one store. f64 store/ reload never rounds, so the per-element op sequence — and every bit-exactness contract and test — is unchanged. Measured 2.2x: ref 4.6 -> 10.0 GFLOP/s, fma 4.7 -> 10.7 GFLOP/s. 3. backend::native::gemm_f64 engine swapped from matrixmultiply::dgemm to gemm_f64_tiled_fma: the f64 GEMM behind BlasFloat::backend_gemm, hpc::blas_level3::blas_gemm, and batched linalg is now entirely in-crate Rust. matrixmultiply remains for gemm_f32 and upstream Array::dot (untouched). Measured own-engine gap ~3.1x (was 6.6x before the restructure): 1024^3 fma 10.3 vs matrixmultiply 33.7 GFLOP/s, max |fma - matrixmultiply| <= 1.4e-13. Next rung recorded: B-panel register reuse + packing. Gates: clippy -D warnings clean, 2185/2185 lib tests green with the engine swap, doctests green, bit-equality suite green after the restructure.
Three-angle adversarial verify (numerics/swap-trace/docs) on the completion diff. Substantive finding acted on: the fused tier's scalar polyfill (AVX2 arm F64x8::mul_add = per-lane f64::mul_add) lowers to a libm fma() call on baseline x86-64 builds without the fma target feature — downstream consumers do not inherit this repo's .cargo pin, so backend::native::gemm_f64 now routes through the UNFUSED reference tier instead: no libm dependence on any backend, ~7% slower than fused on pinned builds (10.0 vs 10.7 GFLOP/s), and the backend engine is now bit-identical to the certification reference. gemm_f64_tiled_fma stays public for FMA-pinned consumers. Doc fixes from the same pass: removed two stale 'gemm_f64 delegates to matrixmultiply' claims that contradicted the swap (simd.rs re-export comment, gemm_f64_tiled rustdoc); added # Panics to gemm_f64 (checked preconditions vs the old wrapper's silent UB on short slices, matching CBLAS xerbla); scoped fma determinism to per-(build,runtime) — wasm relaxed-simd fusion is implementation-defined; corrected the AVX2 vfmadd naming (per-lane polyfill, fused semantics); fixed the integer-corpus bound comment (k_max=128). Gates: clippy -D warnings clean, 2185/2185 lib tests, 4 gemm doctests.
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_184a4f6d-d878-4cc1-80d9-97a6772fded8) |
Contributor
📝 WalkthroughWalkthroughIntroduces a crate-native tiled f64 GEMM implementation (reference and FMA tiers) in simd_ops.rs, publicly re-exports it via simd.rs, routes native backend's gemm_f64 to use it instead of matrixmultiply::dgemm, removes the old dead scalar tiled implementation, rewires two example programs' reference matmul, and documents the changes in blackboard.md. ChangesTiled f64 GEMM Implementation and Adoption
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Example as Example (direct_matmul)
participant NativeBackend as backend::native::gemm_f64
participant TiledGemm as simd_ops::gemm_f64_tiled
participant Impl as gemm_f64_tiled_impl
Example->>TiledGemm: call with alpha=1, beta=0
NativeBackend->>TiledGemm: delegate call
TiledGemm->>Impl: FMA=false
Impl-->>TiledGemm: computed C (unfused mul/add)
TiledGemm-->>NativeBackend: result
TiledGemm-->>Example: result
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
examples/subpel_tap_tile.rs (1)
96-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect swap to the tiled kernel. Same
direct_matmulbody is duplicated inexamples/gridlake_field_tile.rs— already tracked in.claude/blackboard.mdfinding#4as a pending follow-up (extract a sharedf64ground-truth helper across the probe 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 `@examples/subpel_tap_tile.rs` around lines 96 - 105, The direct_matmul implementation is duplicated across probe examples, so extract the shared f64 ground-truth matmul helper into a common reusable function/module and have both examples call it instead of maintaining separate copies. Keep the existing tiled kernel path in direct_matmul, but move the Vec<f64> conversion and gemm_f64_tiled invocation into the shared helper so the logic stays identical in both examples and future changes only need one update.src/backend/native.rs (1)
240-252: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift~3x throughput regression on the crate's f64 BLAS hot path — consider runtime FMA-feature dispatch instead of a static reference-tier default.
Per the PR's own measurements, the unfused reference tier is materially slower than the previous
matrixmultiply::dgemmengine (documented 256³/512³/1024³: ~10 GF vs ~33 GF). The reference tier was chosen specifically to avoid the fused tier's fallback to a slowlibm fma()call on baseline x86-64 builds without the repo's.cargotarget-cpu pin. Rather than a permanent static choice, a runtimestd::is_x86_feature_detected!("fma")dispatch (mirroring the existing LazyLock-based AVX2/AVX-512 dispatch pattern used elsewhere forsimd.rs) could safely pick the fused tier when FMA is actually available at runtime, recovering most of the lost throughput without the portability risk on non-FMA hosts.As per coding guidelines,
**/simd.rs: The simd.rs module must dispatch to simd_avx2.rs on non-AVX-512 systems using LazyLock polyfill, which establishes precedent for this kind of runtime capability-based dispatch.Also applies to: 260-264
🤖 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/backend/native.rs` around lines 240 - 252, The f64 GEMM hot path in native.rs is statically pinned to the unfused reference tier, causing a large throughput regression versus the prior matrixmultiply::dgemm path. Update the dispatch around simd_ops::gemm_f64_tiled so it chooses gemm_f64_tiled_fma at runtime when std::is_x86_feature_detected!("fma") is true, and falls back to the current reference tier otherwise. Mirror the existing LazyLock-based capability dispatch pattern used in simd.rs so the fast path is selected safely on FMA-capable hosts without changing behavior on baseline x86-64. Keep the current documentation/comments aligned with the new runtime selection.Source: Coding guidelines
src/simd_ops.rs (1)
1014-1030: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test coverage for the new panic preconditions.
The stricter
lda < k/ldb < n/ldc < n/ short-slice asserts are a new, documented behavioral contract (vs. the old silently-permissive wrapper), but the test module has no#[should_panic]cases exercising them.♻️ Suggested additions
#[test] #[should_panic(expected = "lda")] fn panics_on_lda_too_small() { let mut c = [0.0f64; 4]; gemm_f64_tiled(2, 2, 3, 1.0, &[0.0; 6], 2, &[0.0; 6], 2, 0.0, &mut c, 2); } #[test] #[should_panic(expected = "too short")] fn panics_on_short_c_slice() { let mut c = [0.0f64; 3]; // needs 4 gemm_f64_tiled(2, 2, 2, 1.0, &[0.0; 4], 2, &[0.0; 4], 2, 0.0, &mut c, 2); }Also applies to: 1108-1390
🤖 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_ops.rs` around lines 1014 - 1030, Add negative test coverage for the new panic preconditions in the SIMD GEMM path: the test module should include #[should_panic] cases that exercise the lda < k, ldb < n, ldc < n, and short-slice asserts added in the extent checks. Use the public entrypoint gemm_f64_tiled (and the analogous tiled GEMM variants covered by the same contract) so the tests lock in the documented behavior. Make the panic expectations specific enough to confirm the correct failure path is hit.
🤖 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 `@examples/subpel_tap_tile.rs`:
- Around line 96-105: The direct_matmul implementation is duplicated across
probe examples, so extract the shared f64 ground-truth matmul helper into a
common reusable function/module and have both examples call it instead of
maintaining separate copies. Keep the existing tiled kernel path in
direct_matmul, but move the Vec<f64> conversion and gemm_f64_tiled invocation
into the shared helper so the logic stays identical in both examples and future
changes only need one update.
In `@src/backend/native.rs`:
- Around line 240-252: The f64 GEMM hot path in native.rs is statically pinned
to the unfused reference tier, causing a large throughput regression versus the
prior matrixmultiply::dgemm path. Update the dispatch around
simd_ops::gemm_f64_tiled so it chooses gemm_f64_tiled_fma at runtime when
std::is_x86_feature_detected!("fma") is true, and falls back to the current
reference tier otherwise. Mirror the existing LazyLock-based capability dispatch
pattern used in simd.rs so the fast path is selected safely on FMA-capable hosts
without changing behavior on baseline x86-64. Keep the current
documentation/comments aligned with the new runtime selection.
In `@src/simd_ops.rs`:
- Around line 1014-1030: Add negative test coverage for the new panic
preconditions in the SIMD GEMM path: the test module should include
#[should_panic] cases that exercise the lda < k, ldb < n, ldc < n, and
short-slice asserts added in the extent checks. Use the public entrypoint
gemm_f64_tiled (and the analogous tiled GEMM variants covered by the same
contract) so the tests lock in the documented behavior. Make the panic
expectations specific enough to confirm the correct failure path is hit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85929b3a-3937-4dd9-b784-98285c00607e
📒 Files selected for processing (6)
.claude/blackboard.mdexamples/gridlake_field_tile.rsexamples/subpel_tap_tile.rssrc/backend/native.rssrc/simd.rssrc/simd_ops.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
Completes the f64 GEMM story: the crate-native tiled kernel graduates from dead code to the canonical
ndarray::simdsurface as a two-tier family, and becomes the engine behindbackend::native::gemm_f64— own Rust in the f64 BLAS path, replacing the externalmatrixmultiplyengine there. Plus the review/diagnostic work from the same session.ndarray::simd::gemm_f64_tiled— bit-exact reference tierF64x8— one source serving AVX-512 / AVX2 / NEON / WASM-SIMD128 / scalar.c = c + (α·A[i,p])·B[p,j]in ascending-p order, mul and add unfused on every backend (never FMA-contracted) → bit-identical across backends; at α=1 β=0 bit-identical to the naive triple-loop reference. Register-resident C row-block (load once, accumulate the k-loop in registers, store once) — f64 store/reload never rounds, so the restructure is provably sequence-preserving.ndarray::simd::gemm_f64_tiled_fma— fast fused tiergemm_f64_tiled_impl<const FMA: bool>(monomorphized, no runtime branch); one fused rounding per step.Engine swap:
backend::native::gemm_f64BlasFloat::backend_gemm→blas_gemm, batched linalg), bit-identical to the certification reference, and no libm-fma()cliff on baseline x86-64 builds that don't inherit this repo's.cargotarget-cpu pin (the reason the fused tier is not the engine — found by the adversarial verify pass).# Panicscontract documented (checked preconditions vs the old wrapper's silent UB on short slices; matches CBLASxerbla).gemm_f32and upstreamArray::dot(impl_linalg.rs) keep matrixmultiply — untouched.gemm_f64_tiledremoved frombackend/native.rsmod scalar(zero callers, verified).Measured (single thread; v3-config compile → AVX2 arm; EMR-class then non-AMX host)
Array::dot)Register residency bought 2.2× (4.6 → 10 GF). The remaining ~3.1× vs matrixmultiply is the accepted own-Rust trade on the hpc BLAS surface; the next rung (B-panel register reuse / i-tiling, then A/B packing) is recorded on the blackboard.
max |fma − matrixmultiply| ≤ 1.4e-13at 1024³.Consumers
direct_matmulf64 ground truth inexamples/subpel_tap_tile.rs+examples/gridlake_field_tile.rsrewired togemm_f64_tiled— measured bit-identical swap (identical printed output pre/post; 0/256 lanes differ vs the previous naive loops).Also on this branch
amx_available()went true→false mid-session;examples/amx_probeshows CPUID TILE/INT8/BF16 all false,cpu_model()=OtherX86(was EMR 0xCF) — the container was rescheduled onto non-AMX silicon. Rule recorded: re-probe before any AMX-tier claim in remote sessions..claude/blackboard.mdupdated per agent protocol (decisions, measurements, verify outcomes, loose ends).Verification
to_bitsequality vs two independent oracles; integer-exactness FMA suite; strided-lda/ldb/ldc with sentinel-padding asserts; zerounsafe; no new feature detection; consumer sites named above.cargo clippy -- -D warningsclean (lib + examples),cargo fmtclean, 2185/2185 lib tests, doctests green,--no-default-featuresbuild green.🤖 Generated with Claude Code
https://claude.ai/code/session_01HNLwoho6pF6TdXntWocR8n
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests