Repository navigation
perf(cpu): remove per-block bookkeeping from the int4 acc4 decode kernel (1.69x t=1, 1.38x block 64) - #1628
Merged
Conversation
`nibble_outputs_avx2` built two bounds-checked slices for every block and then
had `nibble_block_acc_avx2` recover the group count from `packed.len()` and
re-branch on `group` -- all once per block. At `block_size = 32` a block is a
single 16-byte group, roughly ten uops, so that bookkeeping is amortized over
nothing.
Hoisting it into a raw-pointer wide-path accumulator is 1.61x-1.63x on the
decode loop at 1-8 threads, with bit-identical output:
threads block sessions baseline patched speedup
1 32 1 22.963 14.126 1.626x
4 32 1 11.545 7.071 1.633x
8 32 1 5.880 3.655 1.609x
16 32 1 4.622 4.339 1.065x
8 64 1 3.715 3.064 1.212x
8 32 2 5.904 3.945 1.497x
Every row passed an A/A null control (baseline entered twice, worst 1.53%
apart); three further runs failed theirs and were discarded.
The t=16 row is the boundary rather than a shortfall: once the kernel overhead
is gone, t=8 is already within 1.09x of t=16, so the binding constraint has
moved off the kernel and onto the memory system and the parallel runtime.
This came out of establishing the execution regime for the accuracy-4 kernel,
which also bounded the two levers that were under consideration and found both
not worth building -- an N-column tile sharing activation work measures 0.94x
DRAM-resident and 0.76x at full width (its positive number is an L3-residency
artifact of small benchmark shapes), and bf16 scales measure 0.96x/1.10x
because the kernel uses only 18% of the bandwidth available to it. Evidence,
method and the two implementation dead ends are in the accompanying doc.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1628 +/- ##
==========================================
+ Coverage 81.57% 81.62% +0.04%
==========================================
Files 383 384 +1
Lines 175927 180593 +4666
Branches 175927 180593 +4666
==========================================
+ Hits 143511 147407 +3896
- Misses 27512 28241 +729
- Partials 4904 4945 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… its measured regime Takes over Sebastian's draft, audits the unsafe it introduces, and re-measures every published cell on latest main. The mechanism stands; four of its nine numbers did not. Safety. `nibble_block_acc_avx2_wide_raw` derives raw pointers into the weight and activation buffers, and the caller derives one per block. Three safe formulations were built and measured first, because unsafe is only defensible if safe is slower: bounds-checked index sub-slicing 1.340x, nested `chunks_exact` 1.542x, and the same with a hoisted group count 1.542x. The latter two being identical to three decimals localises the entire cost to caller-side slice construction rather than the inner loop, and safe leaves 5-17% of the win on the table, so codegen is not equivalent. The unsafe is therefore kept but confined to one `#[inline]` function with its contract stated as a loop invariant, and a new `validate_nibble_outputs` pass on the safe entry point checks every length and every dimensional product (`checked_mul` throughout) before any pointer is formed. The activation-length invariant it enforces was previously established only by the caller and checked nowhere. No integer-pointer round trips. Tests. The tiled AVX2 path had never been executed by a test in its own module: `the_kernel_tracks_the_float64_contract` drives `k_blocks <= 3` against `BLOCK_TILE = 4`, so the tile loop's trip count was always zero. Adds an exhaustive wide-accumulator differential against the scalar reference, an f64-oracle test over `k_blocks` 4/5/7/8/9 x ragged tails x all block sizes x zero-point present/absent x hostile scales, and a fail-before-unsafe test that asserts the *panic message* -- asserting `is_err()` alone let the mutant survive, because malformed inputs also trip an incidental bounds check after the pointers are formed. Six mutations, all now caught. Measurement. Re-run interleaved against latest main with per-row A/A controls: 1.678x at t=1 and 1.667x at t=4, but 1.141x at t=8 and 1.245x at t=16, and block 64/128 are 1.005x/1.003x -- nothing. The draft's flat "1.61x-1.63x at 1-8 threads" came from a bimodal baseline whose slow mode it sampled; its t=16 row was pessimistic. Multi-session rows were re-measured on aggregate throughput because the harness's pooled median latency mixes contended and uncontended tokens as sessions desynchronise (one baseline read 9.348 against a 15.9 population, A/A 52.7%); they land at ~1.42x, not ~1.59x. `accuracy_level = 0` is a 1.000x null control at t=1/4/8. Also retires the N-tile and bf16-scale directions in the docs as measured negatives, corrects this repo's host model (L3 is 32 MiB per CCX, not 64 MiB shared; 75.8 GB/s is not achievable), and adds an executing aarch64 gate to roy_validate.sh -- the matrix previously only compiled for aarch64. That gate needs QEMU_LD_PREFIX rather than `qemu -L`, because tests that re-exec `current_exe()` are launched through binfmt and do not inherit a `-L` sysroot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…review Opus adversarial review found two ratios that do not recompute, and a test comment contradicted by this branch's own documentation. Recomputing the safe ladder from the raw ms values surfaced a third, which the review had accepted. - Variant B is 23.525/14.527 = 1.619x, not 1.620x. - F over B is 14.527/14.016 = 1.036x, not 1.038x. - "Safe leaves 5-17% of the win on the table" was a deficit measured against B rather than against the shipped form, and matched no consistent definition. Replaced with two defined quantities: the best safe form runs 8.8% slower than shipped and gives up 13.0% of the win (F saves 9.509 ms/token over main, C/D saves 8.271); the worst runs 25.3% slower and gives up 37.2%. - The headline said "1.68x at 1-4 threads", which rounds the t=4 cell's 1.667x up. It now names both cells, per this document's own instruction to quote the cell rather than a multiplier. - `the_tiled_path_tracks_the_float64_contract` claimed `tiles - 1` fails it. That mutant is *equivalent* -- it only migrates blocks into the scalar tail, which computes the same dot product -- and this branch's benchmark doc says so. The comment now names the effective mutant (`tiled_blocks + 1`) and records why the other one is not. - Removed a superseded duplicate of the `wide`/`wide_groups` comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
changed the title
justinchuby
marked this pull request as ready for review
August 21, 2026 08:31
…leaves
The speedup this PR measures is against our own previous self, which is not the
number the assignment asks for. There was no way to answer the real question
from the tree: `benches/ort_baseline.py` covers f32 ops only, so no int4
`MatMulNBits` comparison existed.
`benches/ort_matmulnbits_baseline.py` is the ORT counterpart of
`int4_decode_loop_ab` -- the same five llama3-8B projections in one graph, same
K/N, same block size, same accuracy_level, m=1, matched `intra_op_num_threads`,
and one `Run` per decode token so the unit matches. ORT's `accuracy_level` is
verified honoured rather than assumed: 30.632 ms at acc0 against 7.822 ms at
acc4, 3.9x apart.
Gap against ORT at block 32, acc4:
t=1 3.01x -> 1.79x
t=4 3.70x -> 2.22x
t=8 2.84x -> 2.49x
t=16 1.47x -> 1.18x
Reported with the three qualifications that keep it honest: ORT saturates by
t=8 (1.249 -> 1.227), so the t=16 row flatters us and carries the most framing
uncertainty; t=8 at 2.49x is the worst remaining row and is the same anomaly
that reads 1.141x in the speedup table; and every row is measured without
zero-points, which is ORT's fastest configuration and so the harder comparison
(zero-points cost ORT 29%).
Also recorded: the *default* path is untouched. This kernel is gated to
accuracy_level=4, and at accuracy_level=0 native is 56.307 ms/token against
ORT's 30.632 -- 1.84x, exactly where it was. Nothing here is progress on acc0.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
`cargo fmt --all -- --check` fails on an **unmodified `origin/main`** (`843b0bf7d`): ``` crates/onnx-genai-engine/src/native_decode/mod.rs:1115 crates/onnx-genai-engine/src/native_decode/tests.rs:1412 ``` Formatting is a required check, so this blocks *every* open PR regardless of its own contents. It surfaced on an unrelated CPU-kernel PR (#1628) whose own tree is clean. Pure `cargo fmt --all` output — one function signature that now fits on one line, one `.expect()` chain that no longer does. No hand edits. ### Scope, and why this does not overlap #1640 This PR originally carried a larger repair for the fmt + clippy debt from #1637/#1641. While I was validating it, **#1640 landed and fixed exactly that set**, so I reset this branch onto current main and reduced it to only what is still red. I verified the rest of the matrix is genuinely green on `843b0bf7d` rather than assuming #1640 covered it: | gate on current main | result | |---|---| | `cargo fmt --all -- --check` | **RED** — this PR | | `clippy -p onnx-genai-engine --features native-backend --all-targets -D warnings` | clean | | `clippy -p onnx-genai-engine --features native-cuda --all-targets -D warnings` | clean | | `clippy -p onnx-genai-cli --all-targets -D warnings` | clean | So formatting is the only outstanding gate, and this change is scoped to it. > Process note: this is the **second** fmt repair against main today. #1640 cleaned up #1637/#1641, and #1644 merged a few hours later and reintroduced violations in new files. Four required gates were red on main simultaneously this morning, and the clippy ones are only visible after formatting is fixed — so they surface one round-trip at a time on whichever unrelated PR happens to be open. A merge queue, or running the quality job on `main` post-merge, would catch this at the source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
#1649) `origin/main` (`73e6fe15a`) fails a required check on an unmodified checkout: ``` error: methods `set_retain_decode_graph_across_spec` and `retain_decode_graph_across_spec` are never used --> crates/onnx-genai-engine/src/native_decode/cuda.rs:5516 ``` That is the **Check the native backend compiles** step of `Rust quality`, so it blocks every open PR regardless of contents. It surfaced on an unrelated CPU-kernel PR (#1628). Both methods are `#[cfg(test)]` accessors for the option-c graph-retention seam that #1648 landed as an *enabling primitive* — deliberately ahead of the WP4 tests that will drive them. Their `#[cfg(test)]` siblings either side (`set_retain_graph_on_rewind`, `padded_query_capacity`) are already called, which is why only these two trip. Fix is `#[allow(dead_code)]` on the pair with the reason recorded at the site — rather than deleting a seam that is about to be used, or widening the allow to the whole `impl` block. ### Local verification | gate | result | |---|---| | `cargo fmt --all -- --check` | clean | | `clippy -p onnx-genai-engine --features native-backend --all-targets -D warnings` | clean | | `clippy -p onnx-genai-engine --features native-cuda --all-targets -D warnings` | clean | | `clippy -p onnx-genai-cli --all-targets -D warnings` | clean | | `cargo test -p onnx-genai-engine --features native-backend` | 0 failed | > **Process note — this is the third main-is-red repair today**, and I am only finding them because they land on an unrelated PR: > - this morning: fmt + clippy debt from #1637/#1641 (four required gates red at once) → fixed by #1640 > - midday: #1644 reintroduced fmt violations → fixed by #1642 > - now: #1648 introduces this dead-code lint > > Each costs a full CI round-trip to discover, because the gates are sequential — the clippy steps only run once formatting passes. Running the `Rust quality` job on `main` post-merge, or a merge queue, would catch these at the source instead of on whoever's PR is open next. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er-claimed
Main moved eleven commits (the native-MTP series) while this was in review, and
`strict_required_status_checks_policy=false` means a stale green proves nothing.
Re-measured end to end against `2f94cba4d` with a freshly built baseline arm.
Two rows changed, and both corrections are against my own previous numbers
rather than the originating draft's.
**t=8 reversed to a wash.** 1.141x against `f8eb8a3e2` becomes 1.016x against
current main -- two independent windows (240 and 400 tokens) at A/A 0.52% and
0.03%. The cause is visible in the arms: the baseline got 7.8% faster
(3.542 -> 3.264 ms/tok) while the patched arm barely moved, so the native-MTP
series closed that cell for both sides. Reported as a wash.
**"Specific to block_size 32" was wrong.** Block 64 and 128 had only ever been
measured at t=8 -- precisely the thread count where this change buys nothing --
confounding two variables in one cell. Measured at t=1 where the mechanism is
visible, block 64 is **1.380x** and block 128 is 1.091x. The win covers the two
most common configurations, not one.
The corrected ladder is what the mechanism predicts rather than a fitted curve:
`wide` requires `group >= WIDE_GROUP` (32) and `wide_groups = blob / 16`, so the
removed fixed per-block cost is amortized over 1, 2 and 4 groups at block
32/64/128, and block 16 never enters the wide path at all. Fitting the fixed
cost from the block-32 row predicts block 64 within 3% and block 128 within 7%,
and block 16 measures 1.028x -- an internal control.
Standing matrix on `2f94cba4d`, min-of-6+ interleaved, per-row A/A:
t=1 block 32 1.686x (A/A 0.00%) t=1 block 16 1.028x
t=4 block 32 1.640x (A/A 0.16%) t=1 block 64 1.380x
t=8 block 32 1.016x (A/A 0.03%) t=1 block 128 1.091x
t=16 block 32 1.242x (A/A 0.33%)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
`cargo fmt --all -- --check` fails on an **unmodified `origin/main`** (`90ddd284e`): ``` crates/onnx-runtime-session/src/lib.rs:38 ``` `pub use onnx_runtime_ep_api::DeviceGraphSlot;` was added above the existing `WorkspaceRequirement` re-export rather than in sorted order. One-line swap, pure `cargo fmt --all` output. Formatting is a required check, so this blocks every open PR regardless of contents. It surfaced on an unrelated CPU-kernel PR (#1628). ### This is the fourth main-is-red repair today | # | PR | what was red on main | source | |---|---|---|---| | 1 | #1640 (not mine) | fmt + 3 clippy lints, four required gates at once | #1637 / #1641 | | 2 | #1642 | fmt, two sites | #1644 | | 3 | #1649 | clippy `dead_code`, `native_decode/cuda.rs` | #1648 | | 4 | **this** | fmt, one re-export | #1647 / #1648 | The pattern is consistent and worth fixing at the source: quality gates run on PR branches *before* merge but not on the merge result, so any merge can land violations that then fail whoever opens the next PR. Because the gates are sequential — clippy steps only run once formatting passes — each breakage costs a full CI round-trip to even *discover*, and they arrive one at a time. Two concrete options: enable a merge queue (gates run on the merge result), or run the `Rust quality` job on `main` post-merge so the break is attributed to the PR that caused it instead of the next unrelated one. Also still red on main and **not** fixed here, because I cannot reproduce it locally and it is not mine: `Rust (Windows ARM64)` → *Test cross-platform offline crates* has been failing on main since at least `73e6fe15a` (it is non-required, so it does not block merges). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
changed the title
…d claims Opus review of the re-measurement found a blocking defect and three minor ones, all in the evidence rather than the kernel. BLOCKING: the ORT gap table in the shared ledger quoted old-base (f8eb8a3) native arms next to the new-base A/B table without saying so. At t=8 that manufactured a 2.84x -> 2.49x "win" out of the one cell the same document retracts as a wash. Both ledger and evidence-doc tables are now on current-main arms, and every retained historical table carries an explicit base label. Also corrected: - Re-measured the ORT zero-point cost with the full rep count: 9.885 vs 7.816 ms, ~26%, not the 28% taken from a single window. That arm is noisier than the plain one (a short window read 12.2 ms), so it is quoted as approximate. - The conclusions section still asserted "block 64 and 128 are 1.00x", the claim this document elsewhere overturns; block 64 is 1.380x at t=1. - "exactly what the mechanism predicts" -> "consistent with": it is a one-parameter fit checked at two points, and block 128 lands 8% out, not 7%. The block-16 null is the stronger evidence and is now labelled as such. - The t=8 reversal was attributed to the MTP series; those are CUDA-graph and speculative-decode commits with no stated path into a CPU int4 kernel, so it is recorded as correlated with the rebase window, not explained. benches/ort_matmulnbits_baseline.py: zero points are packed per column, so an odd block count pads each column -- N*ceil(blocks/2), not ceil(N*blocks/2). Those agree only for even `blocks`, which every shape here happens to have, so this was latent rather than wrong. The 117 MB model artifact now gets a pid-unique name and is removed in a `finally`, so concurrent invocations cannot race on it and it stops being left in the source tree. Verified: baseline reproduces at 7.816 ms vs 7.822 published (0.08%); zp path builds and runs; fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
H check-win-arm64 had been failing for every PR in this series, and I had been recording it as "needs a Windows SDK, unavailable on this host". That was wrong, and it is exactly the failure mode the directive warns about: a gate that always fails is indistinguishable from a gate that is not testing anything. Plain `cargo check --target aarch64-pc-windows-msvc` cannot work on Linux because ort-sys runs bindgen over the ORT headers, which #include <stdlib.h>, so it needs the MSVC CRT/SDK and dies with "'stdlib.h' file not found". cargo-xwin supplies exactly that and was already installed on this host; the target std was not. With `rustup target add aarch64-pc-windows-msvc` and `cargo xwin check`, the gate passes on this branch. Now routed through step_if, so an absent cargo-xwin is a loud SKIP with the install hint rather than a permanent red that everyone learns to ignore. Local matrix on this head is now PASS=21 FAIL=0 SKIP=0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 21, 2026 12:07
This was referenced Aug 21, 2026
Open
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…t=1/4/8 (#1852) ## The 1.84x acc0 gap is stale. Re-measured, it is ~1.12x. The published acc0 (`accuracy_level = 0`, the production default) int4 decode gap against ORT — **1.84x at t=1** — is what made acc0 the top remaining CPU MatMulNBits target in the ledger. It dates from `e9754e7ef` (#1628) and **eight merges have landed since**, three of them direct acc0 kernel work. On current main the gap measures **~1.12x**. | width | native tok/s | ORT tok/s | **gap** | gap range | cells (trusted/taken) | A/A range | |---:|---:|---:|---:|---:|---:|---:| | 1 | 27.9 | 31.2 | **1.120x** | 1.112–1.128 | 3/3, **2 retained** | 1.025–1.036 | | 4 | 107.2 | 122.3 | ~1.15x | 1.087–1.284 | 4/6 | **0.868–1.150** | | 8 | 211.0 | 238.0 | **1.120x** | 1.089–1.145 | 3/3 | 0.997–1.028 | | 16 | — | — | ~1.64x, **does not resolve** | 1.456–1.831 | 2/3 | **0.969–1.295** | Both arms are `tokens_s_total`, paired within each launch and then medianed. *Trusted/taken* is the harness's verdict; *retained* is editorial — all three `t=1` cells passed the guard and one was dropped afterwards by me, disclosed below. Only `t=1` and `t=8` resolve. `t=4` sits inside its own A/A null (0.868–1.150). **`t=16` reads ~1.64x and is the open row** — see below; a second revision of this PR corrects an earlier claim that it was wholly contaminated. **acc0 is no longer the top CPU target — conditional on `t=16`.** At the two widths that resolve, the remaining ~12% is a kernel efficiency difference that sits below several other open items. `t=16` is the width closest to an unconfined production process, and a confirmed 1.64x there would reverse that. ## Why the movement is kernel — measured, not inferred The first revision of this PR argued from a control: ORT re-measures at 31.99 ms, within 4.4% of its published 30.632, therefore the harness is comparable and *"the movement is entirely on our side."* **Review objected that this does not follow, and review was right.** The ORT arm reproducing shows the *ORT* ruler did not move. It says nothing about the native ruler, which sits in a different binary and changed repeatedly over the same window — `81e611c03` (#1722) is literally titled *"make the acc0 native and ORT arms measure one quantity"*. So the inference was replaced with a measurement. `e9754e7ef`'s tree is checked out in a second worktree, its `int4_decode_loop_ab` rebuilt, and run **beside** current main's on the same host, same environment, `PROBE_REPS=1` on both so neither gets a rep loop the other lacks, arms interleaved and the launch order alternated: | width | kernel-only, measured | published pair implies | verdict | |---:|---:|---:|---| | 1 | **1.64x** [1.61–1.88], 12 paired cells | 1.59x | apparent movement **is** kernel | | 8 | **1.82x** [1.78–1.89], 6 paired cells | 3.08x | **3.08x retracted** | ### Retracting the t=8 3.08x Both old figures reproduce today to within 0.4% — but only **unpinned**: | published | rebuilt `e9754e7ef` today, unpinned | delta | |---|---:|---:| | `56.307 ms` (t=1) | 56.519 (56.402 / 56.519 / 56.878) | +0.4% | | `14.091 ms` (t=8) | 14.115 (14.105 / 14.115 / 14.196) | +0.2% | The old bench never called `EpFactory::initialize`, so it never ran `bound_process_to_decode_budget()` and its process was never confined. That function — physical-core `select_budget_cpus` included — **already existed at `e9754e7ef`**, and production always called it; only the bench was missing the call, which #1766 `11cb8e5f3` added. The old `t=8` row therefore measured eight decode workers scattered over 32 logical CPUs onto SMT siblings: **a topology no served session ever ran in.** | binary at t=8 | 8 physical cores | 4 cores + SMT siblings | unpinned | |---|---:|---:|---:| | `e9754e7ef` | 8.430 ms | 16.121 ms | **14.115 ms** | | current main | 4.664 ms | 7.988 ms | **4.619 ms** | **1.67x of the claimed 3.08x was placement, not kernel work.** Today's binary is pin-insensitive (0.99x) because it confines itself. This is exactly the effect `docs/benchmarks/2026-08-21-decode-worker-cpu-placement.md` (#1680, ledger §24) already recorded — landing on a number I quoted two days later. ### Two corrections that look right and are not Recorded so they are not re-applied at this site: - **The ~11% warmup/spawn handicap of §27 is in `tokens_s_total`.** Both published figures are **`ms_token`** — the old `ort_matmulnbits_baseline.py` docstring names *"the native harness's `steady` column-2 median"* as its comparand, and the reproductions above land on it to 0.4%. Deducting 11% from `56.307` yields a number no run of either tree produces. - **The statistic asymmetry that *is* real points the other way.** Old ORT took `min` over reps of a per-`Run` median; old native was single-shot with no rep loop. Best-of-N against single-shot flatters ORT, so it made the old gap look *worse*. Calling the two arms "the same statistic", as the first revision did, was wrong. ## Headline-table defects fixed in this revision - **Mixed statistics.** The first table printed native *median latency* beside ORT *throughput-equivalent* and called the ratio a gap, so its columns did not yield its own gap figure. Both sides are now `tokens_s_total`; the mixed variant is shown, labelled, and noted to decline (1.113 → 1.098 → 1.085) rather than be flat. - **False precision at t=4.** `1.148x` quoted against an A/A null of 0.868–1.150. Now "~1.15x, does not resolve". - **Undisclosed post-hoc discard.** "three independent launches per width" was false (3 / 6 / 3 / 3 cells across two invocations), and the `t=1` 1.4% spread depended on discarding a cell after seeing it. Retained-cell figures are now published beside the headline (**1.112x [0.927–1.128], 18.1%, n=3** — the median barely moves, the *precision* does not survive), and the discard rule is stated prospectively for next time. - **Reproducibility.** `acc0_gap_matrix.py` gains `--launches`, a per-width `--tokens 1:64,4:192,8:384` map, and a `gap` column in ORT÷native orientation beside `ratio`, so the Reproduce block names a command that produces the published table. ## Method preconditions added 1. **The two arms were not getting the same machine.** `ONNX_GENAI_CPU_DECODE_THREADS=w` confines the *whole native process* to `w` CPUs; the script pinned ORT to all 16 even CPUs at every width. Measured effect on a quiet host: **1–2%** — real, small, and now data rather than argument. 2. **The realized width is read back and checked** (`decode_width requested=4 realized=4 as_requested`). Timings cannot detect a vacuous sweep. 3. **`LoadWatch` samples the runnable count *during* every arm**, refusing above `width + slack`. A pre-check cannot see a competitor that arrives mid-cell — one did, and four cells were discarded because of it. 4. **The wide-pin arm turned out to be a contention detector.** One `t=1` cell passed every host-level guard while CPU 0 alone was busy: both matched-pin arms ~2x slow, the roaming arm normal. A single-CPU pin is the most fragile cell in any width sweep, and it is what every speedup is quoted against. ## Still unresolved — and a correction to the first revision **`t=16`, and it is the row that matters.** The first revision of this PR wrote the width off as "every cell contaminated". **That was wrong, and wrong in the direction that flattered the conclusion.** Two of the three `t=16` cells passed the load guard cleanly (runnable 6, no competitor recorded): | `t=16` | gap | A/A | native spread | ORT spread | |---|---:|---:|---:|---:| | launch 1 | 1.831 | 1.295 | 17.8% | **55.4%** | | launch 2 | 1.456 | 0.969 | 27.7% | **19.6%** | | **median** | **1.643** | — | — | — | So it is **~1.64x from two accepted cells**, not "no data". It still does not resolve, but on the correct ground: the A/A null spans 0.969–1.295 (±30%, against 3.6% at `t=1` and 2.8% at `t=8`) and both arms are unstable at this width. The cell the guard *did* refuse reads 1.585 — between the two retained — so the discard is not load-bearing either way. This is the one open cell that could reverse the re-ranking, and it needs a dedicated quiet-host study with launch distributions and a pre-registered A/A acceptance threshold. ## Second-review fixes (this revision) An adversarial review returned MERGE AFTER FIXES with all six core claims surviving falsification and seven defects. All are fixed: 1. **`t=16` mischaracterised** — the headline fix above, propagated to all four sites that carried the re-ranking claim. 2. **Cell counts** — `t=4` is 4 trusted of **6** taken; the published table came from **two** script invocations, not three. 3. **Column semantics** — harness-trust and editorial retention were conflated in one column; now separated, which makes the `t=1` discard visible in the table rather than only in prose. 4. **`t=1` placement probe samples published**, including a `114.94 ms` outlier on one pinned rep of three (the old binary's bimodality — it is why the `t=1` A/B range reaches 1.88x). Placement is worth 1.9% at this width, against 1.67x at `t=8`, as expected for a single-threaded process with no SMT sibling to hit. 5. **ORT-side spread disclosed at `t=16`** (55.4%) — the denominator is no better behaved than the numerator there. 6. **Stale checksum constants** in `int4_decode_loop_ab`'s module doc corrected, with a note that they drift under reduction reassociation (#1667, #1783) and that the *pattern* — block 16 moves under `ONNX_GENAI_CPU_MM_INT4_GEBP=0`, block 32 does not — is the route evidence, not the digits. 7. **`--tokens` map footgun** — a map missing a `--threads` width died with a bare `KeyError` after the first cell had already waited out the load guard. It now refuses at parse time, naming the missing widths. ## Validation Docs plus one benchmark harness; no library code. The harness was stub-validated end to end (launch loop, per-width token map, `gap` column, paired per-width summary) and both binaries were run for the numbers above. Required CI (`Fast (Linux x86_64)`, `Rust quality`) must be green before merge — no admin bypass. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Roy <roy@squad.local>
This was referenced Aug 24, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
…tion Independent review returned REQUEST CHANGES on five findings. All five were reproduced before being fixed; two of them are defects in the section's own reasoning rather than typos. Citations (both MAJOR). The hostmon guard was cited as #2026 and the benchmark harness-bug record as #1628. Both wrong, and wrong by the same mechanism: I used `git log -1 -- <file>`, which resolves to the file's most recent commit, not to the commit that introduced the line. `git log -S` on the exact text gives #1950 for the guard (a7003c9) and #1619 for the record (fb38341); #2026 does not touch the guard at all (0 hunks). For a section whose entire value is that the prior records were unfindable, wrong pointers are the defect itself, so this is now a **Check:** in the text. The `1 passed` claim was backwards (MINOR). I wrote that the substring check false-alarms when a filter selects more than one test -- the safe direction. Measured, it fails in the dangerous one: 11 passing tests -> "11 passed; 0 failed" contains "1 passed" -> accepted 2 selected, 1 failing -> "1 passed; 1 failed" contains "1 passed" -> accepted Both are false greens; the second accepts a run holding a real failure. The section now states this, explains why the check is nonetheless sound in agrees_with_hostlock_sh.rs (window_probe_child is a crate-root `#[test]`, so exactly one test is selected by construction), and draws the conclusion the measurements actually support: the two checks compose. The listing pins the selection to one, which is what makes reading the run output sound afterwards. Neither half is sufficient alone. `--list` counts `#[ignore]`d tests (MINOR). `tests::an_ignored_test` lists with the same `: test` suffix and resolves to n=1 while the run executes nothing (`0 passed; 0 failed; 1 ignored`). The listing proves the name resolves, never that the arm ran -- which is why the run-output half is not optional. Noted in the text. My first probe of this claim returned n=0 and appeared to refute it; the probe had used a bare name and selected nothing, i.e. it fell into the trap being documented, so the refutation was the apparatus failing rather than the claim. Design doc (NIT): "a bare name matches nothing" is false for a crate-root test, the asymmetry §9 is careful about. Qualifier restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
## What `cargo test --lib <name> -- --exact` with a **bare** test name matches nothing when the test lives in a module. It prints `running 0 tests`, `test result: ok`, and **exits 0**. To a script reading the exit code that is indistinguishable from the test passing. Inside a mutation battery it is indistinguishable from *the mutant surviving* — and that is the dangerous direction, because every arm then reports "your test does not cover this". You go write coverage you already have, or conclude a guard is unreachable and delete it. ## Measured, not argued ``` cargo test -q --lib a_continuing_turn_is_admitted -- --exact running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 2 filtered out exit 0 cargo test -q --lib tests::a_continuing_turn_is_admitted -- --exact running 1 test test result: ok. 1 passed; 0 failed; 0 ignored; 1 filtered out exit 0 ``` Two properties make it silent rather than noisy, both measured: 1. **A renamed test and a test that never existed produce byte-identical output.** So a battery repeats the same verdict forever after a rename — there is no moment at which it announces that it stopped measuring. 2. **Whether a bare name matches depends only on module nesting.** A `#[test]` at crate root *is* its own full path and matches; the same name inside `mod tests` does not. One battery can therefore hold working arms and vacuous arms simultaneously, which reads as *partial coverage* rather than as a broken instrument. ## This is the third independent discovery in this repo | where | form | |---|---| | `crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs` (#2026) | guarded in code, with a comment naming the exact failure mode | | `docs/benchmarks/2026-08-21-int4-packed-nibble-avx2.md:313` (#1628) | recorded after the battery reported **seven of seven** mutations undetected | | #1982 | paid for again, in full | The knowledge existed both earlier times. It was in a benchmark report and a test comment — places nobody writing a battery is going to look. That is the actual defect this PR repairs: not the trap, which is well understood, but that finding it a fourth time was the expected outcome. ## The check this recommends is a count, not a string ```sh n=$(cargo test -q --lib "$FILTER" -- --exact --list 2>/dev/null | grep -c ': test$') [ "$n" -eq 1 ] || { echo "FILTER-DRIFT: '$FILTER' selected $n, expected 1"; exit 2; } ``` `--list` enumerates matches **without running them**, so "did the filter resolve" is separable from "what did it find". Asserting `1 passed` in the run output is the same idea and is what `agrees_with_hostlock_sh.rs` does. I measured its limits rather than assuming them: it false-alarms the moment a filter selects more than one test — a module filter over two tests prints `2 failed`, which contains neither `1 passed` nor `1 failed` — and it cannot separate a broken filter from a real failure. Fine where exactly one test is selected by construction, as in that file; not a general rule. ## Changes - **`.github/skills/measurement-discipline/SKILL.md`** — new failure mode **§9**, in the file's existing house style (incident, evidence, `**Check:**`). Also adds the vacuity-arm rule: the only arm whose expected result you know independently of the code under test, and therefore the only one that can report that the apparatus is lying. Frontmatter `source:` updated. - **`RULES.md` §8** — one bullet, cross-referencing §9. - **`docs/research/testing/00-integration-stress-design.md:150`** — this doc gave `cargo test ... -- --exact <scenario>` as *the* reproduction recipe, i.e. the fragile form, in a tracked document. Corrected to the full module path. This is `measurement-discipline`'s own "correct the record where the claim lives" applied to itself. ## Validation - Frontmatter parses (`yaml.safe_load`); failure-mode numbering verified contiguous 1..9. - Both new relative links resolved to existing files on disk. - Docs-only: verified against `ci.yml`'s own `is_docs_path()` classifier — all three paths classify docs, `docs_only=true`. - Ran the `root-files` guard from #2052 locally: 36 entries, no strays. - No code, no test, no workflow changes. ## Credit The trap in its current form is Gaff's write-up; the earlier two records are #1628's and #2026's. My contribution is the measurement of *why* it is silent (byte-identical outputs; module-nesting dependence), the falsification of the `1 passed` form's generality, and putting it where the next person will actually hit it. --------- 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.
Removes the per-block bookkeeping in the int4
accuracy_level=4packed-nibble decode kernel:nibble_outputs_avx2built two bounds-checked slices per block and made the callee recover its group count frompacked.len()on every call. Raw pointers are hoisted above thewidebranch and the group count out of the block loop. Output is bit-identical.Originally Sebastian's draft. I took it over to harden and re-measure it; his commit is preserved in the history.
Measured on current main (
c729613a3)Min-of-6+ interleaved repetitions, per-row A/A control,
tasksetto physical cores.accuracy_level=0null control: 1.000x / 0.998x / 1.000x at t=1/4/8 — the same apparatus that reports 1.686x at acc4 reports nothing at acc0.The ladder is the mechanism, not a curve fit
widerequiresgroup >= WIDE_GROUP(32) andwide_groups = blob / 16, so the removed fixed per-block cost is amortized over 1, 2 and 4 groups at block 32 / 64 / 128, and block 16 never enters the wide path. Fitting the fixed cost from the block-32 row predicts block 64 within 3% and block 128 within 7%; block 16 measures 1.028x and is the internal control.Two things I got wrong and corrected
t=8 reversed. It measured 1.141x against
f8eb8a3e2; re-measured against current main it is 1.016x — a wash (two windows, A/A 0.52% and 0.03%). The baseline got 7.8% faster across that rebase window while the patched arm did not move, so the cell closed on its own. Why is not established — the window contains the native-MTP series, but those are CUDA-graph/speculative-decode commits with no stated path into a CPU int4 kernel, so it is recorded as correlated with the window, not explained. Reported as a wash rather than dropped."Specific to block_size 32" was wrong. Block 64/128 had only ever been measured at t=8 — the one thread count where this change buys nothing — confounding two variables in one cell. At t=1, block 64 is 1.380x. The win covers the two most common configurations.
Four of the originating draft's nine rows also did not reproduce (three optimistic; t=16 was pessimistic at 1.065x vs a true ~1.24x). Details, including the inflated t=8 baseline and an invalid multi-session statistic, are in the evidence doc.
Gap against ORT
There was no int4 ORT baseline in the tree (
ort_baseline.pyis f32-only), so this addsbenches/ort_matmulnbits_baseline.py— same five projections, matched block size / accuracy_level / thread count, oneRunper token. ORT'saccuracy_levelis verified honoured (30.632 ms at acc0 vs 7.822 at acc4).Both native columns are the current-main arms. An earlier revision of this
table quoted the old-base (
f8eb8a3e2) arms next to the new-base A/B tablewithout saying so, which at t=8 manufactured a 2.84x -> 2.49x "win" out of the
one cell this PR retracts as a wash. Found in review; base labels now travel
with the numbers.
Honest qualifications: ORT saturates by t=8 (1.249 → 1.227), so the t=16 row flatters us and carries the most per-
Runframing uncertainty; t=8 at 2.57x is now the worst row, and it barely moved (2.61x → 2.57x) because the change is a wash there; and every row is measured without zero-points on both sides, ORT's fastest configuration and therefore the harder comparison (they cost ORT ~26%: 9.885 vs 7.816 ms, min over three windows each). The default path did not move — ataccuracy_level=0native is 56.307 ms/tok against ORT's 30.632, i.e. 1.84x, unchanged. Nothing here is progress on acc0.Safety: why the unsafe stays
Three safe formulations were built and measured at t=1: index sub-slicing 1.340x, nested
chunks_exact1.542x, and the same with a hoisted group count 1.542x — identical to three decimals, which rules out the inner-loop shape and localises the whole cost to caller-side slice construction. The best safe form runs 8.8% slower and gives up 13% of the win, so codegen is not equivalent.The shipped form therefore keeps unsafe, but confines it: one
#[inline]function with a stated safety contract and loop invariant, and a new safevalidate_nibble_outputsprevalidation pass on the entry point (allchecked_mul) coveringvalues,result,block_sums,scales,packedandzero_points, so malformed lengths fail before any unsafe runs. No integer-pointer round trips. The validator is call-once and outside the hot path — the rebuilt binary was byte-identical, so 1.686x already includes it.Test coverage was vacuous, and is now proved otherwise
The tiled path had never been executed by a test in its own module:
the_kernel_tracks_the_float64_contractdrivesk_blocks <= 3againstBLOCK_TILE = 4, so the tile loop's trip count was always zero. "1607 tests pass" was true and meaningless. Three new tests, and six mutations all die. Two only died after the tests were fixed:tiles - 1is an equivalent mutant (work migrates to the tail loop) and was replaced withtiled_blocks + 1; and the fail-before-unsafe test had to assert the panic message, because malformed inputs also trip an incidental bounds check after the pointers form.Miri covers this only if you make it.
is_x86_feature_detected!("avx2")is false under Miri, so a default run takes the generic path and the vector tests early-return — vacuous. UnderRUSTFLAGS=-C target-feature=+avx2with-Zmiri-strict-provenanceMiri interprets the intrinsics; that it genuinely covers both raw-pointer derivations was proved by injecting an off-by-one group read and a+8element offset and confirming Miri reports UB for each. Clean as shipped.Validation on this head
cargo fmt --all -- --checkclippy --all-targets -p onnx-runtime-ep-cpu -D warningsclippy --all-targets -p onnx-genai-engine --features native-backend -D warningscargo test -p onnx-runtime-ep-cpu--lib)+avx2, strict provenanceaccuracy_level=0null controlGating is unchanged by construction —
matmul_nbits.rsis untouched, so theaccuracy_level == 4,m <= 64, VNNI-guard and block-size predicates are exactly as before.Retired as negative directions
The int4 acc4 N-tile (0.94x DRAM-resident; its positive number was an L3-residency artifact of undersized shapes) and bf16 scales / block-major prepack (0.96x) are marked RETIRED in the docs. Both remain defensible for footprint; neither may be sold as speed.
Closes #1619.
Review round 2 (Opus, on the re-measured evidence)
One blocking defect and three minor, all in the evidence rather than the kernel — the code is unchanged since the last round.
Two latent issues in the new Python baseline, both fixed: the zero-point size formula
(N*blocks+1)//2is not the general contract (zero points pack per column, so it isN*ceil(blocks/2); the two agree only for evenblocks, which every tested shape has), and the fixed.onnxfilename could collide across concurrent runs and left a 117 MB artifact behind — now pid-unique and removed in afinally.Re-verified after the change: ORT baseline reproduces at 7.816 ms vs 7.822 published (0.08%), zp path builds and runs,
cargo fmt --all -- --checkclean.