Repository navigation
fix(cpu-ep): close the SDPA/MHA coverage hole behind #1685, and the concat it hid - #1714
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1714 +/- ##
========================================
Coverage 79.71% 79.72%
========================================
Files 408 408
Lines 192432 192545 +113
Branches 192432 192545 +113
========================================
+ Hits 153406 153499 +93
- Misses 33684 33704 +20
Partials 5342 5342
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
force-pushed
the
squad/leon-1685-sdpa-mlas-row-hole
branch
from
August 22, 2026 03:33
1bb11ff to
bb724e0
Compare
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
) ## What `cargo fmt --all` output. Three files, 4 insertions, 6 deletions, all whitespace and line wrapping. No semantic change. ## Why #1715 (`d3688e7e0`) landed with these three files unformatted. That turns two **required** checks red on `main` itself: - `Rust quality` — fails on `cargo fmt --all --check` - `Fast (Linux x86_64)` — same check, same three diffs Because they fail on `main`, they also fail on every PR branched from it, so no PR can currently show a green required set. Confirmed by running the check against pristine `origin/main` locally and by reading both job logs, which cite exactly: ``` crates/onnx-genai-engine/src/engine/load.rs:1172 crates/onnx-genai-metadata/src/lib.rs:54 crates/onnx-genai-metadata/tests/metadata_fixtures.rs:1106 ``` ## Scope Split out of #1714 deliberately. That PR is CPU-EP attention work and owns none of these files; unblocking `main` should not have to wait on an unrelated review, and an unrelated review should not have to carry someone else's formatting fix. `cargo check -p onnx-genai-metadata -p onnx-genai-engine` passes. Note: `Mobius metadata packages` is **also** red on `main`, for an unrelated reason (`validation/generated/diffusion` and friends fail pipeline-spec validation: `workflow image output 'image' must declare value_range`, and an `unknown field 'access'`). That one is a real content/schema mismatch rather than formatting, it is outside my lane, and it is **not** addressed here — it needs whoever owns the metadata schema.
…oncat it hid #1685 reported an intermittent 8-row hole of exact `0.0` in SDPA output under `--features mlas`, plus a SIGSEGV and `identity specialization diverged`. The defect is already fixed on main — incidentally, by PR #828's unrelated IR redesign, which added `wait_for_workers` to the MLAS work-stealing pool — and nothing guarded it. So this closes the coverage hole rather than the kernel. Root cause, now documented on `wait_for_workers`: `wait_for_completion` returns when the last *block* has run, but the worker that ran it is still inside `run_job` holding a by-value copy of the old `Job`. Pre-#828 the dispatcher released `dispatch_lock` there, so the next dispatch republished the loop bounds while that straggler was live; it then decremented the new job's `remaining` without running the new closure (a partition never executes, and a `beta = 0` SGEMM leaves rows of `C` unwritten) and invoked the *old* closure with an index from the *new* range (the SIGSEGV). The race needs two dispatches to overlap, so it is ~3% per process, not per call: 6000 `sdpa_f32` invocations and 3000 nested rayon x sgemm iterations reproduced nothing. `crates/mlas-sys/tests/concurrent_dispatch.rs` instead falsifies the fix — 4 dispatchers on an 8-thread pool. It passes in 0.10s on main and, with `wait_for_workers` removed, one test SIGSEGVs and the other reports the resulting deadlock through a watchdog. Coverage closed: - `AB_COVERED` gains `AttentionTranspose`, whose `PLAN` entry already claimed `Graduation::Partial` while the graduation rule reads `AB_COVERED`. - `sdpa_f32_native` / `sdpa_f32_mlas` name the two routes, since `sdpa_f32` short-circuits to MLAS and cannot be the native half of an A/B. - `native_vs_mlas_differential` gains 8 SDPA shapes x causal, a NaN-prefill fail-loud check that decodes a hole to (tile, row, column), a convex- combination oracle that still holds in a default MLAS-free build, and a concurrent-sessions test. - `identity_hook_specialization_matches_the_general_epilogue` prefills NaN. Falsified: dropping 8 rows/tile (6144 of 98304 elements, #1685's exact signature) leaves the old zero-prefill version *passing*. Two real defects the new coverage found: 1. `concat_cache` looped head-dim outside sequence, striding 512B per store through a contiguous `[b][h][s][d]` buffer and traversing it `dim` times — ~20ms of a ~28ms decode node. It is now two `copy_from_slice` calls per plane with the same fan-out threshold the sibling transforms use. `llama_decode_past1023` 27.2x -> 13.8x ORT, `chunk8` 24.8x -> 13.1x, `chunk32` 27.7x -> 18.5x, parity preserved. 2. No golden exercised the causal offset: every self-attention case has `past == 0` and the one past-KV case has `q_seq == 1`, so `causal = unidirectional && q_seq > 1` is false. Hard-coding `past_seq = 0` leaves all 12 pre-existing goldens green; the new `past_kv_chunked_prefill_causal` is the only one that fails (2.22 abs). `gen_mha.py` gains causal, decode and past-KV rows, and now *rejects* `unidirectional` with `q_seq != kv_seq` and no past-KV: a NumPy oracle swept over every offset in `0..=kv_seq` matches ORT at none of them, so that cell has no defined answer and its 24/384 parity failures were not a kernel bug. Ledger section 45 records the audit, the falsifier method, the 18-cell x 4-thread production-default matrix (432/432 parity PASS, A/A null control per cell), and the finding it exposed: `native_p50` is flat across 1..16 threads on every cell because `sdpa_f32_simd` — the route a default build actually takes — has no rayon fan-out at all, while the MLAS research route does. That is filed separately rather than folded in here. Gates: fmt; clippy default + `--features mlas` + aarch64 `-D warnings`; `check_cross_compile.sh`; full ep-cpu suite both feature configs; mlas-sys; Miri task_runtime/strided/provider/dtype; default cdylib 0 MLAS symbols by `nm`/`nm -D`/`strings`/`ldd` with an 842-symbol positive control. Closes #1685 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Opus review raised both of these as non-blocking robustness notes. The `peak > 1` overlap guard asserted that two workers were inside the closure at once. That is the property the test exists to exercise, but on a single-vCPU runner the workers can be serialised end to end and the absence of overlap is not a defect. Gate the assertion on `available_parallelism() > 1` so it still fails loudly where overlap is possible and does not misfire where it is not. `concurrent_sdpa_sessions_lose_no_work` scanned for lost work but had no watchdog, so the other failure mode -- a pool that stops making progress entirely -- would have hung the suite until CI's own timeout killed it with no diagnosis. Run the sessions on a watchdog thread with the same 120s bound and message used in `concurrent_dispatch`. Neither change touches production code.
justinchuby
force-pushed
the
squad/leon-1685-sdpa-mlas-row-hole
branch
from
August 22, 2026 04:13
bb724e0 to
903ea5e
Compare
justinchuby
pushed a commit
that referenced
this pull request
Aug 22, 2026
Incorporate #1714 so the PR is validated with current CPU SDPA/MHA and concat coverage. Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
…#1737) ## What One docs change: a measurement-conditions block in §45.7 of the CPU-EP benchmark ledger, plus a rule. No code. ## Why §45 landed with an 18-cell × 4-thread grid of **absolute** milliseconds measured on the shared 16p/32l host, and I did not record host occupancy for that window. Roy has since demonstrated that contention on this box moved the *same* benchmark cell between **197.2 and 22.8 tok/s** — an 8.6× swing — while intra-run spreads stayed as tight as **6%** in the corrupted samples. That is the part worth writing down: a tight spread only says contention was *steady* during the run, not that the host was quiet. An A/A null control detects drift, not a steady offset. So the ledger's §45 numbers needed splitting into what that threatens and what it does not. **Survives:** - **Ratios** — arms are interleaved, so a steady occupancy tax inflates both and cancels. Covers the parity grid and the `concat_cache` figures in §45.6. - **The scaling shape in §45.8** — ORT improved 3.05 → 1.06 ms across `t = 1 → 16` *in the same interleaved window* in which the native arm stayed flat. Contention cannot starve one interleaved arm while the other scales 2.9×, so ORT acts as a positive control for core availability — structurally the same move as the 842-symbol MLAS build being the positive control for the symbol probe. The finding also has a timing-free leg: `sdpa_f32_simd` contains no fan-out at all. **Does not survive:** - The **absolute millisecond columns**. A steady tax is invisible in the spread *and* in the ratio. Now labelled shape rather than throughput, with the mild `t = 16` regression (9.2 → 14.4 ms) explicitly called out as a possible artifact that nothing in §45.8 depends on. ## The rule added Record host occupancy alongside the numbers, and gate on the **instantaneous runnable count** (`cut -d' ' -f4 /proc/loadavg | cut -d/ -f1`) rather than the 1-minute load average. The EMA misleads in both directions — it stays high for a minute after a heavy run ends and reads low while a burst is still in flight. (Credit to Sebastian for that one; I had read `loadavg 2.48` as calm while the runnable count was spiking.) An unrecorded window cannot be defended after the fact. It can only be re-run. ## Scope Docs only — no kernel, no test, no behaviour. The conclusions of #1714 and #1718 are unchanged; this constrains how §45's numbers may be quoted. Cross-referenced from a comment on #1718 so anyone picking that up sees the same constraint.
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…he tax (#1793) ## What Retracts the ratio defence I added to §45.7 yesterday (#1737) and adds §45.9 explaining why it was wrong. Docs only. ## The retraction §45.7 argued that the native-vs-ORT ratios survived host contention because (a) the arms are interleaved, so a steady tax cancels, and (b) ORT scaling 2.9× in the same window proved cores were available to both arms. (b) is wrong, and (a) does not apply. **The ORT arm is not an independent control — it is the source of the tax.** In a paired run `bench_generic` builds `ort_session` once and holds it open for the entire loop, alternating `measure_native()` and `measure_ort()` inside it. ORT's intra-op pool **spin-waits between its own runs**, so it is burning cores *during* the native timing. This was already documented — in my own harness. `ab.py`'s docstring: > ORT's intra-op pool spin-waits, so a paired run steals cores from the native arm -- measured at up to 6x depression on long cells which is precisely why `--native-only` exists. I used paired mode correctly (the comparison *is* native-vs-ORT), then reasoned about the output as though the paired-mode tax were not there. ## Why this is structural, not just noise `ab.py` passes the same width to both arms — `--native-threads T` **and** `--ort-intra-threads T`. So at `T = 16` there are ~16 ORT threads spinning during each native sample; at `T = 1`, ~1. **The tax is one-directional and grows with `T`** — which is the exact shape §45.8 reported as a native scaling failure. Interleaving cancels drift over time. It does nothing about a tax one arm imposes on the other. ## Second asymmetry (flagged, not yet verified) Sebastian's #1729 shows the EP's default 16-wide pool pins to cpus 0–15 — on this host 8 physical cores, two workers per core, one L3 domain — and that cpu0 carries a permanent external competitor (0.503 relative throughput). ORT's pool is unpinned across all 32 logical CPUs. Whether that reaches the single-node MHA graphs measured in §45 is **not yet verified**; it needs a `/proc` read against a live bench process, which is queued behind the current host owner. It is marked as unverified in the text. It cannot be assumed away, because `ONNX_GENAI_CPU_DECODE_THREADS` is both the knob `bench_generic` sets for the native arm and the knob governing the pool `decode_affinity` places. ## What survives, and what does not **Survives** — the #1718 finding, because it never rested on a timing: `sdpa_f32_simd` is a plain `for b { for n { … } }` with no fan-out while `sdpa_f32_fast` beside it uses `par_chunks_mut`. The native arm cannot scale with `T` whatever the host does. **Does not survive** — the size of the gap at high `T`, the 13.6× at `t = 16`, and the 9.2 → 14.4 ms regression between `t = 8` and `t = 16`. That regression now has a mechanism pointing at the **instrument**: `t = 16` is where the spinning pool is widest. I had already flagged it as a suspected artifact; it now has a cause. Suggested re-measurement: `--native-only` arms against a fixed reference, or hold `--ort-intra-threads` at 1 while `--native-threads` sweeps. ## Rules added 1. An A/B is a controlled experiment only if the **harness** treats the arms symmetrically. A spin-waiting pool in the comparison arm is a tax, not a control. 2. "The control scaled, so cores were available" is valid only when the control is independent of the treatment. Here the control *was* the competitor. A positive control must be something that cannot be causing the effect it is being used to rule out. ## Scope Docs only. No kernel, no test, no behaviour change. #1714's correctness results (432/432 parity, zero-MLAS artifact, numerics) are untouched — those are not timing measurements. Cross-posted to #1718.
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…d a knob that no longer exists (#1822) Follow-up to #1173, correcting two defects I shipped in it and repairing the rule they undermined. Docs, one ledger string, one new test, one new script. No production kernel or routing change. ## 1. The ledger named a route gate that had already been deleted `PLAN[MatMulF32].shape_gate` said the native `SimdX86` route "gates M=1 on `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` (default off, #1116)". #1183 shipped that GEMV on by default and removed the probe. `git merge-base --is-ancestor 5417d04 bdb4599` confirms it landed **before** #1173 merged — so the ledger was wrong the day it landed. Today `sgemm_simd` calls `sgemm_simd_variant(a, b, c, m, k, n, true)` unconditionally and `use_m1_gemv` is a plain parameter that only the in-process A/B harness passes as `false`. No environment variable reaches that route. `docs/performance/CPU_MATMUL_ASSIGNMENT.md:559` already recorded the correct fact ("It is measured now, and the route is the default. There is no env probe on the dispatch any more"). Two files in the same directory disagreed and nothing compared them. **Now guarded.** `ledger_prose_only_names_environment_variables_that_still_exist` requires every `NXRT_*` / `ONNX_GENAI_*` token in the ledger's prose to still exist as a string literal in the crate's sources. It cannot check that the description is *right*, only that the knob is *real* — which is the half that goes stale silently. Mutation-verified, not just observed green: ``` matmul_f32: ledger prose names environment variable `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV`, but no source file in this crate contains the literal "ONNX_GENAI_CPU_MM_SIMD_M1_GEMV". ``` ## 2. The doc published a toggle A/B that could not have been run #1173 carried a table captioned **"same binary, same session, toggle the only difference"**, reporting `decode 1×2048×2048` at 0.146 with `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` off against 0.337 with it on, and called turning it on "the obvious next slice". Nothing reads that variable. Setting it measures the same route twice; it cannot produce two different columns. The table is withdrawn and the retraction kept in the text rather than quietly deleted. This is the failure mode the document's own graduation rule warns about — **an arm that was not on the route it was labelled with** — committed by the document that wrote the rule. It survived review because a plausible number in a well-formed table is not self-evidently unmeasured. Readers are pointed at `bench_f32_gemm_ab`, which holds the route as a function parameter and carries the M≥2 rows as a built-in control. ## 3. The gap table is re-measured and the ≥5% rule is repaired The old table was one unguarded invocation per row at an unstated width, taken before the decode-placement corrections (#1729, #1794, #1811) — i.e. when the decode pool put 16 workers on 8 physical cores. New harness: `scripts/bench_native_vs_mlas_width.py`. Arms interleaved rep by rep so host drift lands on both equally; per-rep `os.wait4` CPU-efficiency guard adapted from #1809; six reps per arm; two widths. Raw verdicts, spreads and discards are all reported rather than summarised away. **Three findings, all about method rather than kernels.** | | narrow (6 cores, 1 L3) | wide (32 logical CPUs) | |---|---|---| | `matmul_f32 16×512×512` | 1.581, spread 41% | 0.866, spread 134% | | `matmul_f32 decode 1×2048×2048` | 1.117, spread 21% | 0.934, spread 13% | - **Two cases change verdict on width alone.** Same binary, same half-hour, only the CPU mask differs. `x86_sgemm` parallelises over column strips and MLAS declines to parallelise some shapes, so interleaving the two *routes* inside one process does not protect the ratio — it changes both at once. - **`16×512×512` disagrees with itself on both arms**, alternating `keep-mlas` / `native-graduates` from a byte-identical binary. **One more run of the old table could have graduated a route on this row.** - **The narrow arm is more trustworthy despite having fewer cores** — spreads 4–42% against 5–134%, and it lost no reps to the guard. Isolation beat parallelism. **Softmax now decomposes cleanly**, because no vendored MLAS kernel has changed since #1173 (the only `mlas-sys` edits are the additive straggler handshake in `work_stealing_pool.rs`, #828/#1714, which adds waiting). At matched width the MLAS control arm is stationary to within 4% while native improved **1.24–1.27×** — matching #1416's claim for the row kernel. The f32 GEMM rows get no such attribution and now say so explicitly: their control moved **2.0× the wrong way**, so only the current ratio at a stated width is defensible. **The rule gains what it lacked**: spread must be smaller than the claimed win; reps that did not get the CPU are discarded rather than averaged; a verdict is valid only at a stated width. Under it, `decode 1×2048×2048` — the first f32 GEMM case to show a real native win — **still does not graduate**: it costs more CPU (cpu_ratio 0.875), does not hold at 32 threads, and its 21% spread exceeds its 12% win. ## The width claim is verified, not asserted #1815 landed while this was in progress and observed the neighbouring `bench_generic` harness spawning its ORT arm *outside* the affinity confinement it applied to the native arm. That hazard applies to any `taskset` claim, including mine, so I checked it instead of trusting it — sampling `Cpus_allowed_list` from `/proc/<pid>/task/*/status` 40× across a live narrow-arm run: ``` '16,20,22,26,28,30': 478 observations native_vs_mlas- 273, mlas-sys-ws-0..4 39 each, nxrt-task-0..4 2 each '0-31': 1 (the taskset process itself, before exec) ``` Both routes confined identically; no thread escaped. The rule now requires this check. ## Validation - `dispatch_ledger` **17/17**, including the new falsifier, after merging latest `main`. - `default_artifacts_are_mlas_free` **9/9** — the no-MLAS-in-defaults invariant is untouched. - `cargo clippy -p onnx-runtime-ep-cpu --lib --all-targets` clean; `cargo fmt --check` clean. - Normal merge of `origin/main` (`aee2b9d11`), no rebase, no conflicts. ## Limitations - The narrow arm is six cores on one L3 of one x86-64 host. Nothing here transfers to aarch64 or to a two-socket box. - The `activations erf 1 Mi` row shows native 13.5% slower at matched width. The nearest scatter figure is the wide arm's 8% spread, but that is a spread of *ratios* against a move in a *native time*, so the two are not strictly commensurable. Its MLAS control also moved 11%. **Flagged for pinned re-measurement, not reported as a regression.** - The wide arm was taken with ~4–5 cores of unrelated load present. That is stated in the doc rather than hidden, and it is why its spreads are wider; the guard reports which reps were discarded instead of pretending the host was quiet. - No production behaviour changes here, so there is no performance claim to make about the shipped artifact. Refs #1173, #1183, #1809, #1815, #1416. ## Independent review, and what it changed An independent adversarial review of the full diff returned **no blockers** — it confirmed the ancestry argument behind the retraction, the stationary-control premise for the softmax attribution, and that the headline case is correctly *refused* by the rule (21% spread against a 12% win). It also found seven real defects, all now fixed in `f0323f9ed`. The one that mattered most was in the new test. It only proved the variable name appeared *somewhere* in the crate, so a variable whose read site had been deleted but whose name survived in an `EnvVarGuard::set(...)` line would still have passed — which is the precise shape of the defect this PR exists to correct. The test now requires the matching line to be an `env::var(` / `env::var_os(` read or an `_ENV: &str =` binding. Verified by mutation in **both** directions: | mutation | before | after | |---|---|---| | reinsert retired `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` into ledger prose | fails ✅ | fails ✅ | | retire the two real `NXRT_CPU_GEMM_BACKEND` reads, leaving the literal only in test guards | **passes ❌** | fails ✅ | The remaining six were prose defects in the doc: a stated spread range that contradicted its own table's 82% row, "within 4%" against a table reading −4.2%, a narrow-arm ratio fused with a wide-arm attribution, a spread quoted as 7.5% that was 8% *and* compared against an incommensurable quantity, the CPU-efficiency guard oversold as "what makes this table measurable at all" (in-process interleaving is what protects the ratio; the guard catches only *differential* descheduling), and a one-directional provenance argument standing in for the direct control measurement that actually carries the softmax attribution. **Two further defects I found myself while checking the tables against each other**, neither raised by the review: - The `ratio` column is a median of per-rep ratios while the `ns/unit` columns are medians of times. Medians do not distribute over division, so every row looked internally inconsistent to anyone who tried to divide it out (`0.0684 / 0.0617 = 1.109` against a stated `1.117`). Now documented, along with why the per-rep form is the correct one to quote: it pairs each MLAS invocation with the native invocation it was interleaved against, which is the entire point of interleaving. The then→now figures are relabelled as quotients of medians. - "wider than nine of the twelve wide-arm rows" was eleven of twelve. ## Adopting #1814 `aee2b9d11` (#1814) landed on `main` while this was in review, and it closes the exact hole the review found in the guard this document recommends. A differential CPU-efficiency check cannot see contention that lands evenly on both arms; #1814's confined-set meter reads busy jiffies on the process's own `Cpus_allowed_list` and subtracts the process's own CPU, so foreign load shows up directly. The rule now points at it, and the tables here are explicitly marked as predating it and guarded by the weaker method. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1685.
The defect is already fixed on
main— and nothing was guarding it#1685 reports an intermittent SDPA failure under
--features mlas: ~3% of runs leave an 8-row hole of exact0.0in the output, or die with a SIGSEGV, trippingidentity specialization diverged.The fix landed incidentally in
0f40538b2(PR #828, "Redesign inference metadata as a generic control-flow IR"), which added 12 lines towork_stealing_pool.rsas a side effect. The reported-bad SHA8aed77a17has 0 occurrences ofwait_for_workers/observed; currentmainhas 7.Mechanism.
wait_for_completionreturns whenremaininghits zero — when the last block has run. The worker that ran it is still insiderun_job, holding a by-value copy of the oldJoband looping inclaim_iterationsagainst the shared counters. Pre-fix the dispatcher releaseddispatch_lockat that moment; MLAS fans out under rayon insdpa_f32_fast, so the next dispatch republished the loop bounds and bumped the epoch while the straggler was live. It then did two things at once:remainingwithout running the new closure →wait_for_completionreturned early → a partition never executed → abeta = 0SGEMM left rows ofCunwritten (the 8-row hole:8 · dvcontiguous zeros at a tile boundary — MLAS had split the 128-rowprobs·VGEMM 16 ways);C→ the SIGSEGV.Reproduction failed; falsification worked
sdpafilter,--test-threads 8sgemm, NaN-prefilledCsdpa_f32invocations at the issue's shapeThe race needs two dispatches to overlap, and
dispatch_lockserialises them — so the window is only the gap between the last block finishing and the straggler leavingrun_job. That is ~3% per process, not per invocation.So I stopped trying to trigger it and tried to break the fix instead.
crates/mlas-sys/tests/concurrent_dispatch.rsruns 4 dispatcher threads against one 8-thread pool. Onmainit passes in 0.10 s. With onlywait_for_workersremoved:overlapping_dispatches_never_return_with_work_unexecuted→ SIGSEGVparallel_for_returns_only_after_every_worker_has_left_the_closure→ the straggler drivesremainingbelow zero,usizewraps, and the pool deadlocks; a watchdog reports that instead of hanging CI(A third test passed under the falsifier and was deleted rather than shipped as false assurance.)
The audit — the actual deliverable
backend_ab.rsAB_COVEREDAttentionTransposeabsent, while itsPLANentry claimsGraduation::Partial— and the graduation rule readsAB_COVEREDtests/native_vs_mlas_differential.rsbenches/native_vs_mlas.rsidentity_hook_specialization_matches_the_general_epiloguescripts/ort_ab/gen_mha.pyunidirectionalsupported but never set; noq_seq = 1; no past-KVmha_parity/cases.rsq_seq = 1The family with a known reference-route defect was the family with no same-binary A/B.
Production-route coverage was already sound —
mha_ort_parity.rs,msft_attention_ort_parity.rsandqwen35_ort_parity.rsall run ORT goldens in the default MLAS-free build. The hole was entirely in the research/reference route and in the shape grid.Zero-prefill cannot see a dropped write
Falsified by skipping 8 rows of the
probs·VGEMM — 6144 of 98304 elements, #1685's exact signature:0.0, and a hole landing identically in both compared runs cancels out)left 6144 of 98304 output elements unwritten; first at index 7680 = (tile 0, row 120, column 0)Two real defects the new coverage found
1.
concat_cachewalked a row-major buffer column-majorBnshis contiguous[b][h][s][d], so consecutive stores weredimfloats — 512 B at Llama's head size — apart: every store touched a fresh cache line and the tensor was traverseddimtimes. ~4.2 M near-certain misses per tensor, ~20 ms of a ~28 ms decode node. The operator spent most of a decode step copying its own cache. The two sibling transforms document this exact rule in their doc comments; this was the one place that broke it, and no benchmark row supplied a past-KV cache, so nothing measured it.Now two
copy_from_slicecalls per(b, h)plane, fanned out on the sameMIN_PARALLEL_TRANSPOSE_ELEMENTSthreshold:llama_decode_past1023llama_chunk8_past1016llama_chunk32_past992Parity preserved on all three.
2. Nothing checked the causal offset
past_seqis load-bearing only whenq_seq > 1and the cache is non-empty. Every self-attention golden haspast == 0; the one past-KV golden hasq_seq == 1, socausal = unidirectional && q_seq > 1is false.Hard-coding
past_seq = 0leaves all 12 pre-existing goldens passing. The newpast_kv_chunked_prefill_causalis the only case that fails (max abs diff 2.22). Our convention was already correct; it is now pinned. Regenerating with ORT 1.26.0 was byte-stable for the existing 12 cases (+19 lines only).An undefined benchmark cell is not a slow one
Adding causal/decode rows produced 24/384 parity failures on
q_seq=8, kv_seq=1024, unidirectional=1, no past-KV. That looked like a kernel bug and was not one:unidirectional = 0, no mask0..=kv_seqpast_seq, with a real past-KV cacheORT's
unidirectionalis simply undefined whenq_seq != kv_seqwith no past input — neither runtime computes a defined answer, so no timing from that cell is meaningful. It is replaced by past-KV cells (llama_decode_past1023,llama_chunk8_past1016,llama_chunk32_past992), which is how the runtime actually emits chunked prefill, andbuild_mhanow raises on the invalid combination.Complete results — production default build, 18 cells × 4 thread counts
Default MLAS-free
bench_generic:nm,nm -D,strings,ldd→ 0 MLAS symbols, nolibstdc++. Positive control: the same probes on a--features mlasbuild report 842 symbols / 105 strings /libstdc++linked, so the probe demonstrably sees MLAS when present.MultiHeadAttentionexecutes natively at 99.97% of node time, 1 call, no ORT fallback. 432/432 trials parity PASS.native/ortp50, lower is better;(…)= same-invocation A/A null control:bert_base_b8_s128bert_base_decode_kv1024bert_base_s128bert_base_s384bert_large_s128clip_l14_s257llama_chunk32_past992llama_chunk8_past1016llama_decode_b8_kv1024llama_decode_kv1024llama_decode_kv128llama_decode_kv4096llama_decode_past1023llama_prefill_s128_causalllama_prefill_s512_causalphi35_prefill_s256_causalvit_b16_s197whisper_cross_s1500Regressions are included, not filtered. The null arm tracks the native arm within a few percent on every cell, so these are the measurement and not the instrument.
What the table actually says
Read the last column.
native msis flat from 1 → 16 threads on every cell. The ratio degrades with thread count purely because ORT scales and we do not (llama_decode_past1023: ORT 3.05 ms → 1.06 ms across the same sweep).The code agrees:
sdpa_f32_simd— the route a default build takes on x86 and aarch64 — is a plainfor b { for n { … } }with no rayon fan-out at all.sdpa_f32_fast, the MLAS research route, does usepar_chunks_mut. The shipped route is the serial one.At
t = 1the grid is 1.5–7.0×, which is a per-core efficiency gap. Everything above that is unclaimed parallelism worth roughly the thread count. The cells that already win (bert_base_decode_kv10240.55×,llama_decode_kv1280.63×) are the ones small enough that one core suffices.This is filed separately rather than folded into a coverage PR — it is a large change and it touches pool ownership, which is Sebastian's lane. Now tracked as #1718.
Gates
cargo fmt --all --checkonnx-genai-engine/onnx-genai-metadata, untouched here)clippy --locked -p onnx-runtime-ep-cpu -p mlas-sys --all-targetsclippy --no-default-features --features mlas --all-targetsclippy --target aarch64-unknown-linux-gnu --all-targets -- -D warningsneedless_returnin my newsdpa_f32_nativecfg ladder)scripts/check_cross_compile.shcargo test -p onnx-runtime-ep-cpu(default)cargo test -p onnx-runtime-ep-cpu --features mlascargo test -p mlas-systask_runtime/strided/provider/dtypedefault_artifacts_are_mlas_freenm0,nm -D0,strings0,lddnolibstdc++— positive control: the same probes on a--features mlasbuild see 842 symbols, so the probe demonstrably detects MLAS when presentNote:
onnx-runtime-ep-cudahas a pre-existingapproximate value of PIclippy error (a CUDA C++ kernel string inkernels/window.rs). This diff touches 0 files in that crate.Review
Reviewed by Opus (
claude-opus-4.8): no blocking issues, with hand-verified index algebra for theconcat_cacherewrite and confirmation that the new tests are non-vacuous.Two non-blocking robustness notes were raised and both are now addressed in
9ae8f7b26:peak > 1overlap guard could misfire on a single-vCPU runner where workers may serialise; it is now gated onavailable_parallelism() > 1, so it still fails loudly wherever overlap is possible;concurrent_sdpa_sessions_lose_no_workhad no watchdog, so a reintroduced pool deadlock would have hung the suite rather than failing it. It now runs on a 120 s watchdog thread with the same diagnosis message asconcurrent_dispatch.Neither touches production code. All gates re-run green after both changes and after the rebase onto
d3688e7e0.