Skip to content

experiment(cpu): split k in the fused decode GEMM — NEGATIVE RESULT, not for merge - #1617

Closed
justinchuby wants to merge 1 commit into
mainfrom
squad/roy-qgemm-ksplit
Closed

justinchuby wants to merge 1 commit into
mainfrom
squad/roy-qgemm-ksplit

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Reproducibility branch for a refuted hypothesis. Do not merge.

Hypothesis

qgemm's fused path (m <= MR, decode) reads every byte of B once, and the parallel plan splits columns — at n = 3584 / 8 workers each worker walks 448 contiguous bytes out of every 3584-byte row, a stride past a 4 KiB page, with no A reuse to hide the latency. Splitting k instead gives each worker whole rows to stream, paid for with one private m*n i32 accumulator per band plus a reduction.

Result: correct, not faster

Accumulation is wrapping i32, so summing bands in band order is bit-identical to one pass — asserted at every thread count, both plan gates mutation-checked, 11/11 qgemm tests green.

36 cells (6 llama/qwen shapes x t=1,2,4,8,16,32), 3 reps, two prebuilt test binaries alternated, portable drift control 0.999, t=1 rows as a null control (identical code at one thread).

  • v1 hybrid band x column, serial reduction: 8 wins, 6 losses, 16 neutral. Largest single effect in the matrix is a loss (0.40x).
  • v2 pool-aligned bands + parallel reduction (the Amdahl fix, bands*threads/k ≈ 6%): 4 wins, 14 losses.

The Amdahl fix inverting is the tell: the scratch contends for the same L3 the weights stream through, and at m <= MR there is no reuse to absorb it.

What it rules out

With the earlier aspect-ratio sweep (12.85 MB at 17–20 GB/s at every aspect ratio; a 1 MB L2-resident weight at the same 19.5 GB/s), this is the second independent measurement that the fused decode kernel is instruction-bound inside L3. The residual ~2.4x at u8 M=1 is not reachable by memory-layout work — it is the vpmaddwd budget (~10 uops / 32 B of B), and vpmaddubsw cannot replace it at full range.

Full matrices, controls and one unexplained 0.17x anomaly land separately in docs/benchmarks/2026-08-22-qgemm-k-split-negative.md + ledger §17.

…o not merge)

Hands each worker a contiguous horizontal band of B instead of a narrow
vertical stripe, with one private m*n i32 accumulator per band and a
band-order reduction that is bit-identical to a single pass.

Correct, and measured not faster: 8 wins / 6 losses / 16 neutral over 36
cells, and the pool-aligned variant with a parallel reduction is worse
still. Kept on a branch for reproducibility only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby

Copy link
Copy Markdown
Owner Author

Closing unmerged: hypothesis refuted by its own A/B. Branch kept for reproducibility. The finding lands in the ledger as §17 and rules out memory-layout work as the route to the u8 M=1 gap.

@codecov

codecov Bot commented Aug 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.07074% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.43%. Comparing base (2f0d08a) to head (f21524d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...es/onnx-runtime-ep-cpu/src/kernels/qgemm_native.rs 98.07% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1617      +/-   ##
==========================================
- Coverage   81.49%   81.43%   -0.06%     
==========================================
  Files         383      382       -1     
  Lines      179257   175121    -4136     
  Branches   179257   175121    -4136     
==========================================
- Hits       146086   142618    -3468     
+ Misses      28231    27605     -626     
+ Partials     4940     4898      -42     
Flag Coverage Δ
cli-ort-linux 82.60% <ø> (ø)
cli-ort-windows 82.11% <ø> (ø)
mlas 85.32% <ø> (+0.13%) ⬆️
offline 81.32% <98.07%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...es/onnx-runtime-ep-cpu/src/kernels/qgemm_native.rs 92.12% <98.07%> (+2.27%) ⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

justinchuby added a commit that referenced this pull request Aug 21, 2026
…osed fix (#1618)

Docs-only. Closes out the hypothesis §16 named, with the evidence, after
an adversarial review corrected both tallies and the scope of the
conclusion.

## Result
36 cells (6 llama/qwen shapes × t=1,2,4,8,16,32), 3 reps, two prebuilt
test binaries alternated, `portable` drift control 0.999, `t=1` rows as
an identical-code null control.

That control **fails on `1x1024x3072`** (−29% in one matrix, **+14%** in
the other, on code that is identical at one thread), so all five of that
shape's cells are dropped — its wins *and* its losses — leaving **25
counted cells**:

- **v1** hybrid band × column, serial reduction: **8 wins, 5 losses, 12
neutral, geomean 0.997** — a wash. And it *inverts its own prediction*:
the gain should grow with thread count as the stripe narrows; `t=4` is
the best column and `t=32` the worst.
- **v2** pool-aligned bands + parallel reduction (the Amdahl fix,
`bands*threads/k` ≈ 6%): **3 wins, 13 losses, 9 neutral, geomean 0.846**
(3/12/9, 0.904 with one unexplained 0.17x cell also excluded).

A second parallel plan, a scratch allocation and a reduction have to
earn their place. 0.997 does not buy them. **Code #1617 is closed
unmerged.**

## Kept on the record
Accumulation is wrapping i32, so summing bands in band order is
**bit-identical** to one pass — a `k` split owes an equality, not a
tolerance. Asserted at every thread count; both plan gates
mutation-checked.

## Scope, stated narrowly
The experiment moved **two** things at once — the access pattern *and*
the addition of scratch — and the scratch is the best explanation of the
v2 result. So the streaming layout was **never measured in isolation**,
and this does **not** rule out prepacked/reordered `B`, huge pages,
software prefetch, NUMA first-touch, or gating the split to only the
sub-page-stripe case.

The "instruction-bound inside L3" reading rests on §16's aspect-ratio
sweep, **not** on this — a confounded A/B corroborates, it does not
independently confirm. The planning consequence is a priority call, not
a proof that layout work is dead: the instruction budget is the measured
term (`vpmaddwd` at ~0.31 total uops/byte of `B`, ~0.25 vector-only), so
paths cutting bytes *and* uops per weight together — the packed-nibble
int4 kernel at 0.5 B/weight — are the better next spend.

Docs-only: no code, no test or CI gate reads these files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant