Skip to content

docs(perf): record the acc0 route, the SMT placement finding and the f16 layout divergence - #1701

Merged
justinchuby merged 3 commits into
mainfrom
roy/matmul-ledger
Aug 22, 2026
Merged

justinchuby merged 3 commits into
mainfrom
roy/matmul-ledger

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Three entries in docs/performance/CPU_MATMUL_ASSIGNMENT.md for work that landed or was closed today. Documentation only — no code.

§23 and §25 contain the same error in two disguises — an instrument that changed what it measured, and a numerics test built from exactly-representable operands (*0.125) that could not see reassociation at all and reported a confident zero. Both are written up as such, next to §18's version of it.

All seven repo policy scripts pass (check_publish_order, check_profile_table, check_platform_naming, check_dispatch_reachability, check_dispatch_manifest, check_feature_gate_coverage, verify_documented_env_vars).

justinchuby and others added 3 commits August 22, 2026 00:25
…f16 layout divergence

Three entries in the shared CPU matmul ledger for work that landed or was
closed today, kept together because two of them are the same lesson.

**§23 — the acc0 route (`99f105d52`).** #1104's register-blocked int4 kernel
shipped default-off "until the win is measured" and the measurement never
happened, so `accuracy_level = 0` — the production default — took the
per-column path for the kernel's whole life. Route counters proved it
(`nblock = 0`). Records the corrected attribution too: the first one was
inflated ~3x because the probe's `fetch_add` was in the timed path. With a
clean instrument the hreduce removal is worth 1.02x, i.e. nothing, and the
whole 1.48x is four-column activation reuse. The numerics regress up to 3.70x
relatively; that is disclosed, not buried.

**§24 — the t=8 "wash" (#1680).** The premise does not reproduce: the win is
flat through pool width 8 and collapses at 12+. Root cause is
`decode_spmd.rs::node_shards` pinning worker *i* to `allowed_cpus()[i]` in
logical order, so 16 workers land on 8 physical cores. Bandwidth and task
grain were tried first and discarded. No kernel change — handed to the runtime
owner with the measurement.

**§25 — the f16/bf16 layout divergence (`2e1cfb67c`).** Same math, same bytes,
three prices; `[K,N]` crosses a page every `p`. Software prefetch was tried
first and is a negative result. Accuracy moves the same way (`nk` is 2.7-9.3x
better), so there was no trade to weigh. Also records the memory-plan coupling
that could have gone badly — the #1056 predictor was Apple-only for `MatMul`,
so the transpose would have been invisible to the plan on x86.

§23 and §25 both contain the same error, caught in different disguises: an
instrument that changes what it measures, and a numerics test built from
exactly-representable operands that could not see reassociation at all. Both
are written up as such.

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

Adversarial review found two numbers stated more strongly than the evidence
beneath them and one convention break. All three are fixed.

**§23's headline numerics multiplier was wrong.** 2.422e-5 / 6.739e-6 is
**3.59x**, not 3.70x, and the overstatement was repeated in the summary
sentence. This is the one number in that section that has to be exact — it is
the cost side of "1.48x speed for X worse accuracy" — so it is corrected in
both places and pinned to "the worst cell measured" rather than an unqualified
"up to".

**§24 quoted a bandwidth figure this file already refuted.** My all-thread
sweep read 83 GB/s; §22 measured this host at 31-36 GB/s within a CCX and
~56.6 GB/s across both, and §22's numbers are the ones taken with the access
pattern decode actually uses. Against those, a 41 GB/s draw is *above* the
within-CCX ceiling — the opposite of "not bandwidth-bound". §24 cannot dismiss
bandwidth on a number §22 refutes. Rewritten to state the conflict and rule
bandwidth out on the placement A/B instead, which holds shapes, bytes, thread
count and binary constant and varies only which CPUs the workers sit on. The
conclusion is unchanged; its support is now sound.

**Every earlier section links a `docs/benchmarks/` full record; these three
did not.** That left the numbers a skeptical reader would want unbacked — the
kernel A/B range in particular was not reconstructable from anything quoted.
Adds the three records with the full matrices, the controls, the discarded
hypotheses and the negatives, and links them.

Also: dropped a "cf. §18" that claimed a parallel §18 does not support (§18's
probe was real kernel overhead that slowed the shipping kernel; §23's was
measurement-only and corrupted its own baseline) — the distinction is now
spelled out instead; fixed an ordinal that said "fourth" while citing four
priors; replaced a "model-shaped rows are 1.2-1.6x" generalisation that its own
table contradicts at attn_out with the large-row claim the data supports; and
made one "an Opus review" match the file's "adversarial review" voice.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby marked this pull request as ready for review August 22, 2026 00:44
@justinchuby

Copy link
Copy Markdown
Owner Author

Adversarial review (Opus) returned REQUEST CHANGES with three Medium and three Low findings. All six are fixed in 6353ee651; origin/main merged in 0e331e4be. Marking ready.

Two of the Mediums were numbers stated more strongly than the evidence sitting directly beneath them, which in this file is the whole failure mode:

  • §23's numerics multiplier was wrong. 2.422e-5 / 6.739e-6 = 3.59x, not the 3.70x I wrote, and I repeated the overstatement in the summary sentence. That is the cost side of "1.48x speed for X worse accuracy" — the one number in the section that has to be exact. Corrected in both places and pinned to the worst cell measured rather than an unqualified "up to".
  • §24 quoted a bandwidth figure this file already refuted. My all-thread sweep read 83 GB/s; §22 measured this host at 31–36 GB/s within a CCX and ~56.6 GB/s across both, with the access pattern decode actually uses. Against §22, the 41 GB/s draw is above the within-CCX ceiling — the opposite of what I concluded. §24 cannot dismiss bandwidth on a number §22 refutes. Rewritten to state the conflict openly and rule bandwidth out on the placement A/B instead, which holds shapes, bytes, thread count and binary constant and varies only which CPUs the workers sit on. The conclusion is unchanged; its support is now sound.

The third Medium was a convention break with real consequences: every earlier section links a docs/benchmarks/ full record and these three did not, so the kernel A/B range in §25 was not reconstructable from anything quoted. Added the three records — full matrices, null controls, the discarded hypotheses and the negatives:

Lows, all taken: dropped a cf. §18 that claimed a parallel §18 does not support — §18's probe was real kernel overhead that slowed the shipping kernel, §23's was measurement-only and corrupted its own baseline; the distinction is spelled out now instead of asserted as a repeat. Fixed an ordinal that said "fourth" while citing four priors. Replaced "the model-shaped rows are 1.2–1.6x", which its own table contradicts at attn_out (2.25x/2.96x), with the large-row claim the data actually supports. Matched the file's "adversarial review" voice.

The reviewer explicitly checked and confirmed clean: every ratio in §23's table, the 1.45x group-1→group-4 decomposition, all nine §25 production speedups, §24's internal consistency against §22's SMT model, and that the four disclosures I was most worried about softening (§23's shipped numerics regression, §25's 272 MB / worst-at-1.21x lm_head, §25's non-neutral cache admission, the square row not being representative) are all present as first-class bold headings.

Verification on the merged base: cargo fmt --all -- --check clean, all eight repo policy scripts pass (check_publish_order, check_profile_table, check_platform_naming, check_dispatch_reachability, check_dispatch_manifest, check_feature_gate_coverage, check_documented_commands, verify_documented_env_vars), and a link check resolves all relative links in the ledger.

Follow-up split out rather than folded in: #1702 (FusedMatMulBias takes no 16-bit GEMV on x86 — 2845 µs vs MatMul's 1830 µs on qkv), which carries the warning that the #1056 predictor's deliberate exclusion of that operator inverts the moment the GEMV is enabled.

@justinchuby

Copy link
Copy Markdown
Owner Author

Merging under Justin's direct-merge directive, with disclosure. The one red check is inherited from main and is not producible by this change.

Change scope: Detect change scope classified this docs-only and skipped every code job (Rust quality, Fast, EP conformance, CUDA compile x2, Rust Windows ARM64, coverage, CLI ORT). That is correct — the diff is four .md files and nothing else.

The failure: Mobius metadata packages.

invalid: validation/generated/diffusion: Parse error: invalid pipeline spec: ["workflow image output 'image' must declare value_range"]
invalid: validation/generated/diffusion_guided: Parse error: invalid pipeline spec: ["workflow image output 'image' must declare value_range"]

Attribution. Byte-for-byte identical on main's own run at 1842c2b90 (job 96950982436), a base that does not contain this branch. Main has been red on this job since between bdab01b85 (last success) and 9fcb0aa84. A pipeline-spec parse error is not reachable from a .md diff, and this PR is about as clean a null control for it as exists. Filed separately as #1706 with the bisect window, since it blocks everyone's gate rather than just mine.

Local verification on the merged base (0e331e4be, origin/main merged in, not stale):

gate result
cargo fmt --all -- --check clean
check_publish_order / check_profile_table / check_platform_naming PASS
check_dispatch_reachability / check_dispatch_manifest / check_feature_gate_coverage PASS
check_documented_commands / verify_documented_env_vars PASS
relative-link resolution across the ledger 0 broken
Deletion ratio, Detect change scope (Actions) pass

Adversarial review returned REQUEST CHANGES, all six findings fixed in 6353ee651, detailed in the comment above.

No test, kernel or build input is touched by this PR, so there is no code failure to investigate before merge — only main's, which is now tracked.

@justinchuby
justinchuby merged commit 62d6966 into main Aug 22, 2026
10 of 11 checks passed
@justinchuby
justinchuby deleted the roy/matmul-ledger branch August 22, 2026 00:48
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