Repository navigation
npu: a batch past the activation block is refused by name instead of corrupting memory (Refs #1372) - #1373
Conversation
…corrupting memory I8Ctx::quantize_async_rows() holds exactly max_batch_rows() (the M=128 prefill design) and, for a larger batch, prints "am=N exceeds bA capacity MD=128 -- refusing to write past the activation BO" and then hands the plain buffer back -- so the caller launches anyway and poison/reads past the BO. Measured on the d3 pack at its real question length (225 tokens): four fresh device processes, four SIGSEGVs at the same faulting offset; and one run that instead answered, from rows the layers never processed. step_batch() now refuses M > max_batch_rows() on the device path. A causal caller is told to chunk (the chat path already does, at that same number); a noncausal pack cannot chunk -- it encodes the whole prompt in one pass by contract -- so it is told that this is the limit and that host GEMM answers it. The route then answers 500 with that reason instead of dying. Verified on the array, one npu-hold ticket (8 s queue, 22 s held, die 64 C, amdxdna errors 0): the real 225-token case now returns 500 "a batch of 234 rows exceeds this geometry's M=128 activation block, and attention.mode noncausal_full_attention ... cannot be chunked ... (blocked noncausal prefill is missing, engine#1372). NPU_INFER_HOST_GEMM=1 answers it"; a 102-token contract question still answers 200 (choice b, 0.5336); the process is alive after both and journalctl shows no segfault after the fix. The capability itself -- a blocked noncausal prefill, so a d3 pack can encode a prompt longer than one launch -- is left open on #1372 rather than smuggled into this refusal. Refs #1372, refs #796
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f15bbf3a91313814d37cc0c2f2a8cd8df1151377. The guard is in the right place, covers both batch entry points, and its two refusals say the true thing for each caller.
- Placement: the
M > max_batch_rows()check (q4nx_forward.cpp:3160) is the first executable statement instep_batch()after the comments, before any quantise or launch.step_batch_all()(:3114, the verify-batch entry) callsstep_batch()(:3131), so it's covered too. Host-GEMM is exempt, and correctly so: it never usesbA. - The two messages are right: a causal caller is told to chunk at
max_batch_rows()(whichforward_serve's chat path already does). A noncausal pack is told it cannot chunk (#847'sstep_batch(start_pos > 0)refusal), that blocked noncausal prefill is #1372, and thatNPU_INFER_HOST_GEMM=1answers. That'sRefs, notFixes, which is right while the capability stays open. - One file, +18.
copyright.pyrc 0 per the body.
Worth a follow-up (not this PR): the root behaviour is unchanged in I8Ctx. quantize_async_rows() prints its refusal and returns, and launch_async_with_bo_rows() launches anyway, then poison_canary/readback walk am rows of an MD-row BO. Today every caller bounds am itself: this guard for step_batch, and #1348/#1364's rows ≤ MD for the batched head. But the next caller that doesn't will corrupt memory exactly as the d3 run did. Making the refusal propagate (return a failed run, or bool + why, so the launch never happens) would close the class, not just this instance.
Not re-run: the device row (234-row noncausal → named 500, the 102-token fixture → 200, 0 segfaults).
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f15bbf3a83b04b1e5dcbd24a70a832f3b0a0b0c9
A bounds fix on the NPU path, and the three things that make one trustworthy are all present: it compiles at this head, the guard sits before the work it protects, and the bound it compares against is the target's own capacity rather than a constant.
g++-16 -std=c++20 -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN … q4nx_forward.cpp -> 0 errors
guard at :3160 if (!host_gemm_ && M > max_batch_rows()) { err_ = …; return false; }
max_batch_rows() = xd_ ? xd_->max_rows() : 1 // the target's kernel width, 1 when there is no target / host-GEMM
So the refusal precedes the quantise/launch path instead of reporting after corruption, and host_gemm_ is exempted for the right reason (host mode does not use the device activation block). The measured justification is in the comment and it is the kind that earns a refusal: on the real d3 pack at its question length, 4/4 SIGSEGVs at a fixed offset plus one run that answered from rows the layers never processed (engine #1372) — a wrong answer that looks like an answer, which is worse than the crash.
The message is split by attention mode, and both halves are useful: a causal caller is told to chunk at max_batch_rows() (which forward_serve's chat path already does), while a noncausal pack is told it cannot chunk — the format encodes the whole prompt in one pass by contract — so the pack is not answerable at that length on the NPU until blocked noncausal prefill exists, with NPU_INFER_HOST_GEMM=1 as the escape. Telling a noncausal caller to "chunk the prefill" would have been a wrong instruction that costs someone an afternoon.
It also composes with #1364 rather than fighting it: git merge-tree --write-tree chk1373 chk1364b reports 0 conflicts, #1364 makes the head chunk rows (so a wide batch costs 8 launches instead of 128) and this PR refuses what cannot be chunked at all. Those are the two halves of the same invariant — every launch is ≤ the activation block — and together they are the right shape.
Not run: the refusal itself against a live d3 pack at an over-wide prompt (needs the array and the pack). What I could check — compile, placement, the bound's definition, the message branches, and that it does not conflict with its sibling — is green.
|
Correction to the verdict line above: the head is VERDICT: READY at |
…o a d3 pack answers on the array
The device's activation BO holds exactly max_batch_rows() rows (the M=128 prefill design) and a wider launch
was not quantized at all: I8Ctx::quantize_async_rows printed its "exceeds bA capacity" guard and returned the
plain buffer, so the kernel read and poisoned past the BO (engine#1372 -- four segfaults out of four on the
d3 pack's real 225-token questions, plus one run that answered from rows the layers never processed).
Block at the one choke point every batched projection goes through: npu_gemm_rows() walks M rows in
max_batch_rows()-sized launches when the caller asks for more, copying each block into the caller's buffer.
Nothing else moves -- the row loops downstream (row_attn and the GDN recurrence) already worked per row and
never needed the whole prompt in one launch; only the GEMMs did. Host-GEMM returns before the new path, so
the reference arithmetic is untouched.
Measured on the published Typed Decisions split (LocalLLaMA/typed-decisions, the benchmark engine#796's rung 3
runs), device, six full_i8 designs + six _m16 twins, one npu-hold ticket (2 s queue, 410 s held, die 67 C,
amdxdna errors 0):
* 20 cases / 100 decisions of 188-687-token questions: **answered, 0 failed, 0 "exceeds bA capacity"
lines** (previously 4/4 SIGSEGV on the first request), 4.0 s/decision;
* vs the pack's own d3_runtime.py on the same 20 cases and the same q4_1 weights: **accuracy 0.4000 vs
0.4000, 0 argmax flips**, Brier 0.2628 vs 0.2717, per-decision |dp| worst **0.1455** / mean 0.0516 --
i.e. the device's int8 grid (0.1423 measured on fixtures before this change), not a different answer;
* host regression: 3 cases / 15 decisions, same rows, |dp| vs the reference stays in the host class with
0 device fds, so the blocked path did not leak into host mode.
Refs #1372. Supersedes the refusal in #1373: with blocking, any row count is answerable, so refusing would
make this path dead code -- if #1373 lands first, this replaces it and I will rebase.
…ks (Refs #1372) The refusal was the safety net for a path that could not answer at all. With the blocked GEMM in the line before this, any row count is answerable, so the guard would have made the new path dead code: a device prefill wider than max_batch_rows() now runs as several launches instead of returning a 500.
…o a d3 pack answers on the array (Refs #1372) (#1382) * npu: a prefill wider than the activation block is walked in blocks, so a d3 pack answers on the array The device's activation BO holds exactly max_batch_rows() rows (the M=128 prefill design) and a wider launch was not quantized at all: I8Ctx::quantize_async_rows printed its "exceeds bA capacity" guard and returned the plain buffer, so the kernel read and poisoned past the BO (engine#1372 -- four segfaults out of four on the d3 pack's real 225-token questions, plus one run that answered from rows the layers never processed). Block at the one choke point every batched projection goes through: npu_gemm_rows() walks M rows in max_batch_rows()-sized launches when the caller asks for more, copying each block into the caller's buffer. Nothing else moves -- the row loops downstream (row_attn and the GDN recurrence) already worked per row and never needed the whole prompt in one launch; only the GEMMs did. Host-GEMM returns before the new path, so the reference arithmetic is untouched. Measured on the published Typed Decisions split (LocalLLaMA/typed-decisions, the benchmark engine#796's rung 3 runs), device, six full_i8 designs + six _m16 twins, one npu-hold ticket (2 s queue, 410 s held, die 67 C, amdxdna errors 0): * 20 cases / 100 decisions of 188-687-token questions: **answered, 0 failed, 0 "exceeds bA capacity" lines** (previously 4/4 SIGSEGV on the first request), 4.0 s/decision; * vs the pack's own d3_runtime.py on the same 20 cases and the same q4_1 weights: **accuracy 0.4000 vs 0.4000, 0 argmax flips**, Brier 0.2628 vs 0.2717, per-decision |dp| worst **0.1455** / mean 0.0516 -- i.e. the device's int8 grid (0.1423 measured on fixtures before this change), not a different answer; * host regression: 3 cases / 15 decisions, same rows, |dp| vs the reference stays in the host class with 0 device fds, so the blocked path did not leak into host mode. Refs #1372. Supersedes the refusal in #1373: with blocking, any row count is answerable, so refusing would make this path dead code -- if #1373 lands first, this replaces it and I will rebase. * npu: drop the #1373 refusal now that a wide prefill is walked in blocks (Refs #1372) The refusal was the safety net for a path that could not answer at all. With the blocked GEMM in the line before this, any row count is answerable, so the guard would have made the new path dead code: a device prefill wider than max_batch_rows() now runs as several launches instead of returning a 500. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
… so a verify batch cannot outrun its activation BO (#1387) * npu forward: chunk the tri-ternary head rows path at the head context's MD, so a verify batch cannot outrun its activation BO `run_lm_head_rows()`'s pb16i path launched all M rows into a head context built on the decode design (MD = 16), and nothing checked M <= MD. `quantize_async_rows` refuses am > MD and the launch runs anyway, so rows 16..M-1 were read and written past the activation BO -- the #1372 failure on the head instead of the layers. #1373's refusal covered M > 128 (never rows 17..128) and #1382 removes it, so the pb16i head was left with no bound at all. Reachable from step_batch_all() -> step_batch() -> run_lm_head_rows() with M = the verify batch, i.e. any pb16i pack with a draft head and 17+ draft rows. Chunk the tiles loop at the context's own MD, which is what the int8 packed head has done since #1364: the weights are already resident, so a chunk costs one launch (M=128 becomes 8 launches instead of 128), and each row keeps its own activation scale, so the chunk boundary cannot move a value -- the numeric gate is the per-row comparison against the single-row path the issue asks for. Same file, different function from pi-npufeat's #1372 work, which owns the step_batch() body. Fixes #1386 * npu forward: a short pb16i head chunk is a named failure, not a quiet row Review point on #1387 (the chunking copied the int8 head's loop but not its guard): chunking is exactly what makes canary_short() worth having here, because a chunk that comes back short would otherwise feed dequant_pb16i rows nobody wrote -- poison_canary() plus a zeroed buffer make a plausible-looking row, so the logits go quietly wrong instead of printing one line. The guard now mirrors the int8 head's: the same `[canary] SHORT RESULT (head rows)` line, plus err_ naming the chunk and the short row so the serve error says which. With the return in place the std::fill(got, 0) ahead of dequant_pb16i was dead work (dequant writes all rows*T), so it is gone -- it was only covering the stale rows a short readback would leave. Refs #1386 --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Refs #1372 (the immediate half; the capability stays open there).
I8Ctx::quantize_async_rows()holds exactlymax_batch_rows()rows (the M=128 prefill design) and for a larger batchprints
am=N exceeds bA capacity MD=128 -- refusing to write past the activation BOand then hands the plain bufferback. The caller launches regardless, so the kernel is run and
poison_canary/readback walkNrows of a 128-row BO.On the d3 pack at its real question length (225 tokens) that is memory corruption, not a refusal:
full_i8designs + six_m16twins,amdxdnaerrors 0)LocalLLaMA/typed-decisionssplit, fresh process eachjournalctl -k, npu-hold ticket per chunk)am=234 exceeds bA capacitylines, i.e. the same overrun landing in mapped memoryChange (1 file, 20+/0−)
step_batch()refusesM > max_batch_rows()on the device path, with a reason that depends on the one thing thatdiffers between callers:
app/forward_serve.cpp:325-357already chunks at that samenumber — this is a guard against a caller that does not, not a new limit);
step_batch(start_pos > 0)is refused for it by contract (engine#847), becauseevery full-attention layer must see the whole prompt in one pass. So it is told that this is the limit, why, and that
NPU_INFER_HOST_GEMM=1answers it.Host-GEMM is untouched — it never uses that buffer, which is why the same 225-token questions answer there in 24 s.
Verified on the array (one
npu-holdticket: 8 s queue, 22 s held, die 64 °C,amdxdnaerrors 0)tools/copyright.py --checkrc 0;:18097untouched.Not in this PR
The capability — a blocked noncausal prefill so a d3 pack can encode a prompt longer than one launch (per layer: fill
K/V for every block, then attend per block) — is a real piece of work and stays on #1372. Until it lands, the honest
statement about the route is: d3 on the NPU answers prompts up to 128 tokens; longer questions are refused by name,
and host GEMM answers them.