Repository navigation
npu forward: Gemma4 verifies a draft in one batched pass; KV row width per layer (fixes #1780) - #2303
Conversation
…llback step_batch_all returns every row (fixes #1780) A verify of k drafted rows on a Gemma4 target was k weight streams: the batched body refused per-layer embeddings and KV sharing and fell back to one step() per row. It now builds every row's per-layer inputs, runs the PLE gate and projection as M-row GEMMs plus the layer scalar (gemma4_tail_rows), reads a KV-shared layer's owner cache without appending, and gives a shared layer's double-wide MLP its GU2/D2 designs, all as step(). Two defects found on the way, both pre-existing: - Snapshot, restore, a verify's rollback (commit_verify_rows) and extend_context counted every layer's KV at NKV*HD. Gemma4's full layers hold 2 x 512 floats per position against 2 x 256, so the count read twice the positions and the cut kept half of them: prefix-cache restores and partial accepts were wrong on any two-geometry model. kv_widths() gives each layer its own width; q4nx_state_planes takes per-layer cuts. - step_batch_all() on a model that falls back to per-token step() (gpt-oss, hy_v3) returned no per-row logits. batch_check_state_test gains a verify arm (k=4 step_batch_all bit-equal to 4 steps; commit_verify_rows(2) holds exactly 5 positions; the next step equals a forward that saw only the accepted rows). Nemotron-3.5-Bolt-E4B, host GEMM: 7/7 PASS, oracle rows identical=4, rel-L1 0. Before the width fix the chunk arms failed 4/4; with the old width restored (mutant) 6/7 fail. CI config: all 8 batch_check_state families pass (hy_v3 and gptoss_forward failed the new arm before the fallback fix). q4nx_state_planes_test: a Gemma4-E4B two-width arm. ctest 277/277. Device (Nemotron-3.5-Bolt-E4B + its DFlash draft, forward-serve, 32 greedy tokens, npu-hold tickets, amdxdna ERROR 554 -> 554 in both, every spec reply byte-equal to plain): code plain 0.525, 0.589 tok/s | DFlash, per-row verify (NPU_NO_BATCH=1) 0.537 | DFlash, batched 3.37, 3.43 (28 of 28 drafts kept, 4 rounds) -> 0.557 -> 3.40 tok/s, 6.1x prose plain 0.569, 0.555 | DFlash, batched 0.823, 0.837 (16 of 106 kept, 16 rounds) -> 0.562 -> 0.830, 1.48x FLM's published bar for this pair is 1.3-1.9x. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ready for review at 4d10370 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at What had to change for Gemma4 to take a batched verify
Those two are why this is 336 lines rather than a flag flip: a batched verify on a model with PLE and KV sharing needs the batch path to accept both, and needs the state layout to describe itself per layer. The correctness statement is the one that matters
That is the property #2243's host test holds for the un-shared case, restated for a model where "the accepted rows" and "the state" are related per layer. If a snapshot/restore gets the per-layer widths right, the equivalence holds; if it uses one width, it silently works for the full layers and mis-sizes the shared ones — which is exactly the class #2150/#2240 chased. And the gate accounting is clean
Two asks
|
|
Both asks answered in the body; the head is unchanged (4d10370), so the READY at that head still describes the code.
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4d10370: READY
The bug this fixes is the worst shape a cache bug can take: it truncated state silently, and the failure showed up later as a short cache rather than as an error.
The header states the mechanism exactly: one want per position was applied to every plane, "so one want cut the wide planes at half their positions — the snapshot kept the prefix's first half on those layers, and a restore then resumed from a cache that was short." Nothing failed; attention simply ran against a cache missing half of each wide plane. Per-plane widths are the correct fix, and the extrapolation rule is written down rather than left implicit ("a plane past the end of per is cut at per.back()"), which is the difference between a convention and an accident.
The test is what makes me comfortable, because it keeps the old behaviour as a reference arm. It does not only assert the new path:
- the new path's arithmetic is pinned with real numbers —
180 * kPerPos == 184320and184320 * 100 / kRec == 23("180-token prompt kept 23 % of the plane"), plus848 * kPerPos > kConvat the boundary where the prefix'swantexceeds a whole conv plane; - and the
old_arms reimplement the previous behaviour and assert it fails:check(rec_saved[0].size() < kRec, "old: recurrent plane truncated below its real size"),check(!old_put(conv_saved, dst, want), "old: restore refused the conv plane (slot has no usable state)").
That is a built-in negative control: if someone reverts to a single per-position count, the old_ arms still pass and the new ones fail, so the regression is visible rather than silent. It is the same discipline I asked for on #2286 and #2247, except here it arrived on its own.
On the batched verify half: the code says what it is equal to rather than what it does — "a shared layer's double-wide MLP uses the GU2/D2 designs, all as step() does" — and it reuses q4nx_state::keep_positions with the same widths for both caches, so a batched pass cannot diverge from step() in how much state it keeps. batch_check_state_test is the oracle that compares batched against sequential, so the equivalence has a test rather than only a comment.
One thing I would like pinned while this is fresh, not a blocker: a per list shorter than the plane count, since that is the branch per_of exists for and it is currently only documented. One case with three widths and five planes would pin per.back() being used for the last two.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- the batched path is held to the sequential one bit-for-bit, and the KV-width fix is proven load-bearing by a mutant
The equivalence claim is the only one that matters for a batched rewrite, and it is made exactly: k = 4 rows of step_batch_all are bit-identical to four sequential step() calls, and the next step after commit_verify_rows(2) equals a forward that only ever saw the accepted rows -- i.e. both the arithmetic and the position bookkeeping are pinned. The second is the one that usually rots silently: an off-by-one in "positions held" produces perfectly legal logits from the wrong rows.
Three independent failures make the fixes falsifiable rather than asserted:
| result | |
|---|---|
| this head, Bolt-E4B host GEMM | 7/7 PASS, oracle identical=4, chunk logits rel-L1 0 |
| before the width fix | the 4 chunk arms FAIL (positions held wrong, last-chunk rel-L1 0.71) |
mutant kv_widths() returning the single width |
6/7 FAIL |
The mutant is the part I would single out: it proves the per-layer width is load-bearing rather than incidental, which is the difference between a refactor and a fix. And q4nx_state_planes_test's two-width arm keeping the old-cut checks means the defect stays reproducible after it is fixed.
The two pre-existing defects it found are both real and both subtle: a KV-shared layer holds no plane of its own (its rows live in the owner's plane, at the owner's width), so any per-layer width arithmetic that assumed one width was wrong in both snapshot and restore; and a model that still falls back (gpt-oss, a MoE with a leading dense layer) was returning fewer rows from step_batch_all() than it had. Finding those while adding the batched body is the argument for writing the equivalence arm first.
Scope is stated rather than glossed: a batch with injected embeddings is still refused on a PLE model, because the per-layer inputs need token ids -- and the device row is a real npu-hold measurement (two tickets, Bolt-E4B + DFlash, 32 greedy tokens) with a before/after table, not a claim by analogy from the host. ctest 277/277 under thermal-run and a clean copyright.py --check complete it.
Fixes #1780: item 4, the measured row for the pair. Items 1–3 merged in #2048, #2174, #2180 and #2236.
Fixes #1781: gate 4, the two named pairs; the Bolt row below is the second one. The other gates are on main:
What changes
Gemma4 runs the batched body.
step_batch_implused to refuse per-layer embeddings (PLE) and KV sharing. Gemma4 therefore fell back to onestep()per row, and a verify of k drafted rows cost k passes over the weights. The batched body now does, row for row, whatstep()does:gemma4_tail_rows);GU2/D2designs.A batch with injected embeddings is still refused on a PLE model, because the per-layer inputs need token ids.
Two pre-existing defects, found by the new test arms:
kv_widths(), plus per-layer cuts inq4nx_state_planes.h). Snapshot, restore, a verify's rollback (commit_verify_rows) andextend_contextcounted every layer atNKV * HD. Gemma4-E4B's full layers hold 2 × 512 floats per position, against 2 × 256 on its sliding layers. The count therefore read twice the positions, and each cut kept only half of them. Prefix-cache restores and partial accepts were wrong on any model with two attention geometries.q4nx_state_planes_test's two-width arm checks the empty shared plane (layers 24..41).step_batch_all()on a model that still falls back (gpt-oss, a MoE with a leading dense layer) now returns every row. Before, it filled only the last row and leftlogits_allas it found it.docs/npu.mdcovers all three changes.Proof
Host. I added a verify arm to
batch_check_state_test:step_batch_allare bit-identical to 4 sequential steps;commit_verify_rows(2)leaves exactly 5 positions held;Nemotron-3.5-Bolt-E4B, host GEMM, this headidentical=4; chunk logits rel-L1 0kv_widths()returns the single widthbatch_check_state*/gptoss_forwardfamilies[batchcheck]lines are printed only inside it.q4nx_state_planes_testgains a Gemma4-E4B two-width arm. Its old-cut checks reproduce the defect.ctest277/277 (device tests excluded, under thermal-run).tools/copyright.py --checkis clean.Device.
Nemotron-3.5-Bolt-E4Bwith its DFlash draft on forward-serve, 32 greedy tokens. Two npu-hold tickets; the amdxdna ERROR count was 554 → 554 in both. Every speculative reply is byte-equal to plain.NPU_NO_BATCH=1)The means are 0.557 → 3.40 tok/s on code (6.1x) and 0.562 → 0.830 on prose (1.48x). FLM's published bar for this pair is 1.3–1.9x.
Both named pairs, speculative vs plain decode, greedy, replies byte-equal to plain:
bonsai2-27b-t2s2) + DFlash2 27B draftThe Qwen3.8 row compares against plain decode, not against FLM's MTP head, because the served pack has no MTP head. Nothing for either pair is still owed.
🤖 Generated with Claude Code