Repository navigation
perf(cpu): finish the int4 pack modulo matrix — the withheld rows were the answer - #2102
Merged
Merged
Conversation
…re the answer #1809 removed the per-group integer division from `dequant_panel_avx2` and reported it as 1.015x on block-16 decode and "a null on prefill". The prefill half was taken at m = 64/256/512, having withheld m = 1 and m = 8 for failing their A/A null at 5.31% and 4.62%. The pack is amortized over m rows, so the mechanism puts the effect at *small* m: the sweep had a hole exactly where the answer was. m = 8 is 1.0071, 95% CI [1.0050, 1.0092]. m = 16 is 1.0064. The gain decays monotonically to a bounded null by m = 64, which is where #1809 started measuring. Block-16 decode reproduces at 1.012x on current main, after #1729 and #1794 both moved decode placement and default width underneath it. 61 independent launches per arm, interleaved and rotated, pinned to one physical core away from cpu 0's permanent competitor, gated on each launch's own rusage CPU efficiency rather than a runnable count sampled at run boundaries. The per-launch spread at m = 1 is 102% while its median A/A null is 0.14%, so a single careful pairing on this host can be off by 2x. No kernel change: #1809's code was already correct. What ships is the corrected scope, plus the tooling that makes the claim checkable. - `int4_prefill_route_ab` prints an FNV-1a fold of the raw output bytes per row, so a source-level A/B can observe which arm ran instead of assuming it. - `int4_modulo_matrix.py --route-proof` builds a deliberately poisoned third arm and self-checks: before == after on all 16 rows (the elimination is exact, so the speedup is free), and the poison moves on every row whose route reaches the line -- except block 32 m = 1, which is bit-identical because that row takes the N-blocked decode kernel and never calls the pack. - That same row is the experiment's best null and it is free: two binaries differing only in code it never executes, still differing in layout, ASLR and page backing, reading 1.0028 CI [0.9986, 1.0042]. Refs #1809, #1676. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Opus review of the matrix tooling, three findings, all taken. 1. `int4_modulo_arms.sh` printed the three arms' sha256 for a human to eyeball. Two arms with the same bytes is the worst outcome the script has -- the matrix still runs, every row still reports a number, and the number is a null between a binary and itself -- and it is reachable without anyone doing anything wrong, because the first arm patches the source to the line already on main and cargo may treat that write as a no-op. Now a hard gate that exits 1 and names the recovery. 2. The published confidence intervals were computed by an ad-hoc script that did not ship, so a reader following the reproduce line could obtain the point estimates and not the intervals that make them readable. The bootstrap now lives in `int4_modulo_matrix.py` with a fixed seed and resample count, the A/A arm gets its own interval, and the harness self-checks that every A/A interval brackets 1.000 rather than leaving the reader to verify it. 3. `checksum()` fed `slice::from_raw_parts` a length taken from the shape while trusting the view to be contiguous. For a strided view that is a read past the allocation, not a wrong number, so row-major strides and a zero byte offset are now asserted. Also records that `--route-proof`'s block-32 m=1 expectation is a tripwire as well as a control: a failure there means the routing moved before it means the kernel broke. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2102 +/- ##
==========================================
+ Coverage 80.29% 81.06% +0.76%
==========================================
Files 426 429 +3
Lines 205244 215150 +9906
Branches 205244 215150 +9906
==========================================
+ Hits 164801 174410 +9609
- Misses 34792 34967 +175
- Partials 5651 5773 +122
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…be boring Re-took every number from the shipped tooling after the review fixes changed the binaries, added block 16 as a second block size, and three things moved. **Prefill is a gain at small m, in both block sizes.** Block 16 gives 1.0067/1.0060/1.0046 at m = 1/8/16 decaying to an exact null by m = 64; block 32 gives 1.0096/1.0060/1.0044 at m = 8/16/32, same decay. Every A/A interval in both sweeps brackets 1.000. **The decode headline was overstated, and my own harness caught it.** #1809 said 1.015x. Two sweeps here give 1.0116 and 1.0095, and in both the decode-loop A/A interval *excludes* 1.000 — with opposite signs, +0.63% at 21 launches and -0.28% at 41. That is not a bias to divide out; the floor is ~±0.6%. Reported as ~1.010x, corroborated by the single-op harness on the same route (block 16 m = 1) which does pass its own null. **Block 32 m = 1 read a 1.9% loss on a route that provably never executes the changed line.** The poisoned build is bit-identical there, so the two binaries differ on that row only in code that does not run. The same source change built against a main three commits earlier read +0.3% on the same row. So a source-level A/B here has a code-layout component reaching ~2% that is not stable across rebuilds and is structurally invisible to an A/A, which compares a file with itself — every A/A in the report brackets 1.000 while the artifact sits in the same table. That is why the m-curve is the result rather than any single row: the same `quant_prefill_gebp` runs at every row of the block-16 sweep, and a fixed layout difference cannot be +0.6% at m = 1 and exactly 0.0% across four consecutive rows at m >= 64. The pack's share can. Adds `--env` to the harness (the GEBP=0 layout control, verified off-route by the poisoned arm going bit-identical) and `--skip-prefill`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…airs
Second-round review landed on the load-bearing claim: "the 1/m decay of the
ratio rules out code layout, because a layout difference in the kernel that
runs at every row cannot be +0.6% at m=1 and 0.0% at m=512."
That is wrong. A layout difference in any fixed-cost region costs a constant
number of milliseconds regardless of m, and a constant absolute cost over a
total that grows with m produces exactly that 1/m ratio decay. Both hypotheses
predict the curve, so the curve cannot choose between them. Worse, read in
milliseconds they are the same size: the claimed saving is ~0.015 ms and the
layout artifact on the route-not-taken row is -0.014 ms.
Withdrawn, and replaced with the experiment that does discriminate: a third
pair of arms built from the same one-line change with
`-Cllvm-args=-align-all-functions=5` applied identically to every arm. It
changes nothing an instruction executes and moves where functions start --
50.6% -> 90.8% of FUNC symbols onto a 32-byte boundary, +33 KB of padding.
Across three independently built pairs:
* the row the change provably never executes swings +0.28% / -1.93% /
+0.28% -- a 2.2-point range on a row where the binaries differ only in
code that does not run;
* every row it does execute keeps its sign in all three, at the same
absolute magnitude: m=8 is +0.017 / +0.023 / +0.033 ms;
* the saving is flat in m where it resolves -- the perturbed pair reads
+0.033/+0.027/+0.032/+0.037 ms at m=8/16/32/64, which is what a
once-per-panel cost looks like. The two unperturbed pairs lose it at
m=64 because 0.02 ms against 7.6 ms is 0.26% and those intervals are
+-0.3% wide: at the resolution limit, not absent.
Route proof passes identically on the perturbed pair, m=1..512, both block
sizes, including `poison == after` at block 32 m=1. Every A/A brackets 1.000;
0 launches discarded.
So the headline is the absolute figure, not the ratio: ~0.017-0.033 ms per
packed panel, independent of m, which is 1.004x-1.014x at m=1/8/16. The
standing rule for the repository changes with it -- reproducing across two
block sizes from one pair of binaries does not substitute for reproducing
across pairs, because one pair has one layout.
Also corrects the source comment and ledger, which folded block 32 m=1 into
"gains at m=1/8/16 in both block sizes". That row is the route-not-taken
control and reaches this line at no m.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Round 2 review: one High finding, and it was the load-bearing claimA second Opus pass over The claim: "the 1/m decay of the ratio rules out code layout, because the same Why it is wrong: a layout difference in any fixed-cost region — setup, allocation, the pack itself — costs a constant number of milliseconds regardless of And read in milliseconds instead of ratios, they are the same size:
Claimed saving ~0.015 ms; layout artifact on the route-not-taken row −0.014 ms. One build pair cannot separate them, and I had presented one build pair. Withdrawn, and replaced with an experimentBuilt a third pair of arms from the same one-line change with Block 32, three pairs, ratio and the same figure in absolute ms:
Block 16, B against C: Route proof passes identically on pair C across They separate cleanly:
Deliberately moving every function in the binary did not remove the effect, flip it, or leave it untouched; it made it slightly larger. That is the argument now, not the shape of the curve. What changed in the PR
Also fixed (Medium, ×2)The source comment and the ledger both said the gain was "1.005x–1.010x at Validation on |
justinchuby
marked this pull request as ready for review
August 25, 2026 12:57
justinchuby
enabled auto-merge (squash)
August 25, 2026 12:58
Owner
Author
|
The failing step is Confirmed it is main and not this branch, both directions:
Fix is #2115 (test-merges against current main with zero conflicts; #2114 is DIRTY). Not filing a third. Auto-merge stays armed; I will merge main and re-trigger once it lands. No |
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
…ecline (#2131) ## The failure `kernels::matmul_nbits::tests::parallel_output_rows_dispatches_to_the_task_runtime` failed the `Fast (Linux x86_64)` lane on #1173: ``` the flat fan-out published no task-runtime dispatch test result: FAILED. 1469 passed; 1 failed ``` It passes on a 32-thread workstation, and I could not make it fail there by narrowing the machine. It passes at forced task-runtime widths 1, 2, 3, 4, 5, 8, 16, 32, 64; at decode budgets 1, 2, 4, 16; at `RAYON_NUM_THREADS` 1, 2, 4, 32; pinned to 4, 2 and 1 CPUs; and across three full-suite runs pinned to 4 CPUs. The route is deterministic — it is `TaskRuntime` on both host shapes, by two different paths through `flat_fan_out`. ## The cause, reproduced rather than argued `TaskPool::dispatch` returns `false` — running the body inline and publishing nothing — when it cannot claim one of its `SLOT_COUNT = 8` slots. That is documented, deliberate behaviour: > Returns `false` without running anything when the pool cannot serve the fan-out — no workers, shut down, or every slot busy — so the caller can run it serially. So routing a fan-out to the pool does not oblige the pool to take it, and `after.dispatches > before.dispatches` on a single call conflates a **routing decision** with a **scheduling outcome**. Asserting it is asserting something the runtime never promised — the same class of error as reading a route off a host-dependent width. I held all eight slots and ran the test's body verbatim: ``` dispatches+0 slot_exhausted+1 panicked at matmul_nbits.rs: the flat fan-out published no task-runtime dispatch ``` Exact CI signature, and **every output row was still written correctly** — the row-verification loop ran before the dispatch assert and passed. Only the counter was unmoved. A/B under one controlled 200 ms transient, same harness, same binary: | pattern | outcome | |---|---| | old, single-shot assert | **FAILED** — "the flat fan-out published no task-runtime dispatch" | | this PR | ok | Why a narrow runner and not this box: **the test creates the condition itself.** It installs a 16-thread Rayon pool on four cores. Oversubscription is exactly what makes a dispatcher get descheduled while holding its slot, so the rest of a 1470-test suite running four-wide can hold the other seven long enough to collide. ## The fix Split the two claims the test was making at once. **The route** is now asserted directly, against both widths `flat_fan_out` reads, observed inside the pool rather than assumed. That is the actual regression guard — it is what a code change would break — and it is deterministic on every host shape I can produce. **The dispatch** is attributed to the call that made it. `for_each_range` already returns the `Backend` that served it, and says why: > Returned rather than logged because scheduling is the thing under test here. That return value was being discarded. It is now recorded in a `#[cfg(test)]` thread-local next to the `#[cfg(test)]` counter this function already carries, and the verdict is taken from it: `Native` passes, `Serial` retries the transient slot decline against a wall-clock bound, `Host` fails hard, and `None` — never set, so the call never reached the runtime at all — is a routing failure reported as one rather than waited out. The first pass of this PR used a retried counter delta instead, and review was right to reject it. `PoolCounters` are process-global on purpose — resetting them would make concurrent tests lie to each other — so a delta taken around one call cannot attribute a dispatch to that call. With 1470 tests in flight on four cores, a concurrent test's dispatch can satisfy a delta the caller never earned, and a concurrent test's slot exhaustion can excuse a decline the caller never suffered. That is unsound for precisely the reason the original assertion was unsound, with a narrower window. `Backend` is per call and per thread and has no such window. The counters survive only in the timeout diagnostic, labelled process-wide. **Retrying is sound only because the permanent alternatives are eliminated first.** `for_each_range` returns `Serial` for five reasons besides the pool declining: an empty range, a forced-serial guard, a nested task, a host task, and a partition that would not split. None of those clears on a retry. `testing::planned_backend` answers exactly that question without running anything, reading the same thread-locals and the same pool width the fan-out will, so it is asserted next to the route. A `Serial` inside the loop is then attributable to `dispatch` returning false *by elimination* rather than by assumption. `dispatch` returning false still has three causes — busy slot, no workers, shut down — and only the first is transient. The timeout message reports what was observed and branches on the one signal that separates them: over thousands of attempts, a pool that never had threads leaves `slot_exhausted` completely still. It no longer asserts a mechanism it did not measure. The wall-clock bound is deliberate: the thing being waited out is a descheduled dispatcher, which is milliseconds, while a few thousand declined fan-outs are microseconds. A retry count alone expires before the condition it exists to outlast. ### Controls Six perturbations of this tree, each restored afterwards: | control | perturbation | result | |---|---|---| | route poison | `flat_fan_out` forced to return `Wide` | both tests **fail** at the route assertion | | execution poison | the arm's `== PrefillFanOut::Wide` flipped to `!=`, so the route still reports `TaskRuntime` while the fan-out takes Rayon | both tests **fail immediately** with "never reached the task runtime" — not masked, not retried | | elimination poison | `planned_backend` forced to `Serial` | both tests **fail** at the elimination assert (`:23590`), so it is live rather than vacuous | | no-workers poison | `dispatch` forced to return `false` without touching `slot_exhausted` | **fails** after 5s: "ran 4375 consecutive fan-outs inline … and the pool recorded no slot exhaustion at all, which points at a pool with no workers or a shut-down pool rather than a busy one (pool width 16, process-wide slot_exhausted +0)" | | transient decline | all 8 slots held, released after 200 ms | **ok**, `elapsed=199.4 ms` (the single-shot pattern fails under the identical harness) | | permanent decline | all 8 slots held forever | **fails** after `elapsed=5.0006 s` naming 4244 declined fan-outs with `slot_exhausted +4244` — the other branch of the same message | The execution poison is the load-bearing one. It keeps the route honest and breaks only the execution, which is exactly the case the old assertion could not distinguish and exactly the case a blanket retry would swallow. Both halves of the assertion are demonstrated live. ## Two more, found in the same place **The doc comment was wrong, in a way I have some history with.** It claimed pinning the Rayon pool "makes the decision under test the same one on a 4-vCPU runner as on a 32-thread workstation, instead of silently testing the host." `flat_fan_out` reads *two* widths and the pool pins one: `lanes` is `task_runtime::width()` and still reads the host, so a wide host returns at `wide <= lanes()` while a narrow one falls through to the `HOT_FAN_OUT_*` test. Same verdict at `calls == 1`, different path — true by luck, not by construction. Caught by Gaff; it is the same "named the wrong route" failure I built a poisoned-binary control for in #2102, one level up, in a comment. **The sibling test had the same defect plus a second one.** `a_deferred_decode_kernel_still_dispatches_to_the_task_runtime` carried the identical dispatch assertion, and evaluated its `output_chunk_len` precondition **outside** the `DeferredDecodeGuard` — against the ambient Rayon width instead of the deferred width the fan-out actually uses. On `main`: ``` RAYON_NUM_THREADS=1 → panicked: the partition policy declined to split 6144x4096 ``` With the precondition moved inside the guard it passes. Verified both directions. ## Validation - `cargo test -p onnx-runtime-ep-cpu --lib` — 1823 passed, 0 failed, both wide (32 threads) and pinned to 4 CPUs - `cargo clippy -p onnx-runtime-ep-cpu --all-targets --all-features -- -D warnings` — clean - `cargo fmt --all --check` — clean - Route assertion holds at task-runtime widths 1..64, decode budgets 1..16, Rayon widths 1..32 Test-only; no production code changed. Saturating runs held `scripts/hostlock.sh` (#2087). --------- 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.
Finishes the matrix #1809 could not: the two rows it withheld (
m = 1,m = 8, pulled for failing their A/A null at 5.31% and 4.62%) were the rows the effect lives in. The pack is amortized overm, so the mechanism puts the effect at smallm— the sweep had a hole exactly where the answer was.No kernel change. #1809's code is correct and already on main. What ships here is the corrected scope, plus the tooling that makes it reproducible.
Result
The elimination saves a fixed ~0.017–0.033 ms per packed panel, independent of
m— a once-per-panel cost, which is what the mechanism predicts. As a ratio that is 1.004x–1.014x atm = 1/8/16, fading below the instrument's ~0.3% resolution bym = 64.The absolute figure is the result; the ratio is just that constant over a total that grows with
m.Decode, block 16: ≈1.010x, not #1809's 1.015x — the decode-loop A/A excludes 1.000 in both sweeps taken here with opposite signs (+0.63% at 21 launches, −0.28% at 41), so its real floor is ~±0.6%.
The finding that outlives this PR
Block 32
m = 1routes toborrowed_affine_int4_matmul_nblockand never reaches the changed line — the poisoned build proves it, staying bit-identical there. On that row the two binaries differ only in code that does not execute. It read −1.9% in one build and +0.28% in two others.So a source-level A/B on this tree carries a ~2% per-build code-layout component, it is not stable across rebuilds, and it is structurally invisible to an A/A — the A/A arm is the same file, so it measures everything except what differs between builds. Every A/A in this PR brackets 1.000 while a 1.9% artifact sits in the same table.
Standing rule now in the ledger: include a row the change provably cannot reach, prove it with a poisoned build, read it as the floor. Where none exists, require the result to reproduce across independently built pairs — cheapest via
-Cllvm-args=-align-all-functions=5, which perturbs layout and nothing else.An argument I withdrew
An earlier revision claimed the 1/m decay of the ratio ruled out layout, since the same kernel runs at every row. That is wrong. A layout difference in any fixed-cost region is a constant number of milliseconds, and a constant over a growing total gives the identical 1/m curve. Both hypotheses predict it. In milliseconds they are the same size: +0.015 ms claimed, −0.014 ms artifact.
What discriminates is a third arm pair built with every function 32-byte aligned (50.6% → 90.8% of
FUNCsymbols; +33 KB padding; no instruction changes). Across the three pairs: the route-not-taken row swings 2.2 points, every executed row keeps its sign and magnitude, and the perturbed pair reads a flat +0.033/+0.027/+0.032/+0.037 ms atm = 8/16/32/64.Route proof
before == afterbit-identical on all 16 rows, both block sizes, both build pairs — the elimination is exact, so the speedup is free rather than bought. A deliberately poisoned third arm (drops the+ q) moves the checksum on every row whose route reaches the line and nowhere else, which is what proves the poison isn't just perturbing the binary.--route-proofexits non-zero on any deviation.Method
61 launches/arm (41 for the perturbed pair), not one careful pairing — the per-launch spread at block 32
m = 1reaches 102% while 61 launches agree on a median to 0.14%. Per-launch CPU-efficiency gate (os.wait4rusage(utime+stime)/wall, floor 0.95). Percentile bootstrap, 20 000 resamples, fixed seed. The A/A arm is a separate file, so it pays every per-launch cost.int4_modulo_arms.shfails hard if any two arms are byte-identical. Whole sweep underscripts/hostlock.sh, pinned to cpu 4.Validation
cargo test -p onnx-runtime-ep-cpu --lib x86_sgemm— 20 passedcargo clippy -p onnx-runtime-ep-cpu --benches --all-features -- -D warnings— cleancargo fmt --all,shellcheck -S warning— cleanint4_modulo_matrix.py --route-proof— PASS on both build pairsTwo rounds of Opus review; all findings taken, including the withdrawal above.
Refs #1809, #1676.