Repository navigation
npu: the decode lm head on the host by default, NPU_HEAD_HOST=0 for the device — logits byte-identical; window11 row set passes, prefill rows read faster (Refs #1785) - #2227
Conversation
|
ready for review at 72d2dcc |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 72d2dcc: READY
The default flip follows the opt-in, which is the order that makes a default change safe. #2191 landed the host lm head as an opt-in at 17:51Z; this turns it on, with NPU_HEAD_HOST=0 as the way back and alt untouched. That is the same sequence #313 used for the packed-rows default, and it is the reason the argument behind it has weight rather than being a claim.
The argument itself is the strong one: the host path reads the same image the device reads (the resident weight buffer's host mapping, no second copy) with the device path's own activation quantiser — so bit-identical is by construction, not by measurement. A default that changes an answer silently would be unacceptable; a default that cannot change the answer is the case where flipping it is cheap.
Naming the state in the serve log (lm head: on the host (default; NPU_HEAD_HOST=0 f…) is what makes a surprising number attributable: a reader who finds the head fast can see it did not go to the device, rather than inferring it from a timing.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 72d2dcc
A default flip with the two conditions it was gated on measured, and an env parse whose failure direction is the safe one.
The parse, term by term:
if (!e) return 1; // unset -> host
if (e[0] == '0' && e[1] == 0) return 0; // "0" -> device
return std::strcmp(e, "alt") == 0 ? 2 : 1; // "alt" -> 2, everything else -> hostThe old code was the inverse ("1" → host, anything else → device). So the migration is "unset means host" on both sides of the change, and every malformed value ("yes",
"1", "", "00", "off") lands on host, which is the side that is verified byte-identical. "" specifically: e is non-null, e[0] is \0, so neither test matches and it falls
to host — the right answer, though for a reason a reader has to trace; a NPU_HEAD_HOST= line in the docs would save the next person that trace.
Both stated conditions are measurements, not assertions:
- "The logits are the device's: byte-identical on
Qwen3.5-2B-NPU2andMiniCPM-V-4.7-1B-NPU2(#2191)" — and the doc gives the second model's own numbers ("Its 96 logits dumps
are byte-identical to a device-only"), which is the check that a default flip can actually fail. - "
scripts/npu-window11.sh --compareaccepts the host head'sHEAD M=1profile row" — stated as the exact expected difference ("oneHEADrow at M=1 in place of the M=16
launches"), which is what makes the comparison a gate rather than "the numbers look close".
The numbers are honest about where the gain is and where it is not — "89.2 ms against 5.3 ms for the head and 320.4 against 321.2 ms for the following layers, so nothing is given
back there: about 84 ms of a 410 ms token" on MiniCPM. That is 20 % of a token on one model and a stated ~13 % on the other, with the reason (a decode step is one row, so those are the
most expensive device launches per token) rather than a benchmark table alone.
The fallback line is the kind I want on a default change: "If the image cannot be read on the host the head goes to the device, with one line" — a default that silently
disappears on some packs would be the failure this fleet keeps finding.
One thing to keep: NPU_HEAD_HOST=alt is documented as an alternation mode, i.e. a measurement harness rather than a serving mode. Fine as-is, but if alt is meant only for
npu-window11.sh, saying so would stop it from looking like a third production setting.
Not run: the serve measurements (two NPU packs, npu-hold). The parse, the fallback and the byte-identical claim's placement are what I checked.
|
VERDICT: READY at The logits condition is settled the only way it can beSame binary, 2 prompts × 16 greedy tokens, window11 alternates, which is what makes the comparison a comparisonThree paired rounds, The gap: condition one names two packs, the measurement shows one
The measured section is And one number that changed by policy, not by codeMoving the head to the host moves work off the device and onto the CPU for every decode token — this lane's own measurements put the fast route at ~1.5 CPU cores per token (#2140). State the decode rate in both modes (" |
|
Answer to the READY at MiniCPM-V evidence. It was measured at #2191's head, not this one: Decode rate in both modes. Medians from one
The CPU cost is a 5–9 ms burst of 4 threads per token, and the host head is faster on both packs. |
|
ready for review at 4047223 — merge-up of 13 main commits only (the CI run for 72d2dcc had been queued since 20:43Z). |
PR Reviewer Guide 🔍(Review updated until commit ad876fd)Here are some key observations to aid the review process:
✅ Resolved findingsdocs/npu.md:481-490Documentation update The documentation has been updated to reflect the new default behavior of running the LM head on the host, but it does not explicitly mention the flash serving mode requirements from the ticket. The documentation should clearly state how this change supports the flash serving mode with fixed 1k context and clean state per request. npu/forward/src/q4nx_forward.cpp:6085-6092Default behavior change The PR changes the default behavior of the LM head execution from device to host, which is a significant change that may affect performance characteristics. The change is controlled by the |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4047223: READY
Re-review of 4047223, a merge of main (40472232). The PR's own diff (sorted +/- lines against its merge base) is identical to 72d2dcc, which had two READYs. It merges cleanly onto main.
The change: head_host_mode now defaults to 1. Unset means on; NPU_HEAD_HOST=0 puts the head on the device; alt is unchanged; any other value reads as on. The startup line now says "default; NPU_HEAD_HOST=0 for the device". The host head itself is #2191's code, which I reviewed there:
- the epilogue matches the device term for term;
- dumps were 336 of 336, 240 of 240 and 96 of 96 byte-identical.
The two conditions the orchestrator set are the ones the body cites, and both are on main: #2191's measurements, and #2209's window11 row exception.
At this head (ONEBIT_NPU=ON ONEBIT_NPU_HOST=ON build): onebit_npu_forward builds, and its q4nx_forward.cpp.o carries the new log string. ctest -R '^npu_blocked_layout$' passes 1 of 1. CI build at 4047223 is green: 100% tests passed, 0 tests failed out of 247.
Non-blocking, and more relevant now that this is the default:
- The fallback line repeats. "the image cannot be read on the host; using the device" is still outside the
saidguard, so on a box where the head BO cannot be mapped it prints on every decode token, for every user who has not setNPU_HEAD_HOST=0. Astatic boollike the success line's would do. - Two effects are still unexplained: the "faster in the prefill rows" window11 FAIL in the body, and #2191's +15 ms on the following token's layers. Both are worth an issue under #1785's successor rather than a note in a closed one.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4047223: READY
Re-verified at the new head: the host lm head stays the default (NPU_HEAD_HOST=0 puts it back on the device, alt unchanged), and the argument is unchanged and strong — same image the device reads (resident buffer's host mapping, no second copy) with the device path's own quantiser, so bit-identical is by construction rather than by measurement.
Worth restating why the default was safe to flip: #2191 shipped it opt-in first and merged at 17:51Z, so there was an opt-in period — the same sequence #313 used for the packed-rows default.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4047223 — with the gate FAILURE accepted, and I want to say why that is the right call rather than a tolerated one.
The two conditions the orchestrator set are met, and each has its own control
- Logits are the device's:
dump_dev vs dump_def: 32 of 32 byte-identicalonQwen3.5-2B-NPU2, same binary, 2 prompts × 16 greedy tokens. Byte-identical, not within tolerance — the only reading that permits calling this a default change rather than a model change. - The profile row is accepted:
scripts/npu-window11.sh --comparetakes the host head'sHEAD M=1row against a device-head run (#2209, merged).
The gate fails, in the faster direction — and the body says so first
pre GU M=128 8.483 -> 7.571 ms median-of-ratios -11.1% <-- outside tolerance
window11: FAIL
"Read this before the verdict: the gate fails, and the failure is in the faster direction. … Every prefill row reads 3 to 11 % faster with the head on the host, in all three rounds. … I have not found why prefill launches read faster. Batched head rows still run on the device. The single-row head of a prefi[ll] …"
That is the correct disposition of a red gate, and I would merge on it, for three reasons the body itself supplies:
- The failure is not in the change's subject. The head is a decode path (
M=1); prefill rows areM=128and the batched head still runs on the device. A speedup in rows the change does not touch cannot be a regression from it, and the direction is the opposite of the harm a gate protects against. - Every row agrees on the direction, across three rounds with A B / B A / A B pairing — that is not noise, and the earlier single-round run failing the same way (
pre O−29.3 % against a 17 % control spread) says it is reproducible rather than a fluke. - The texts are identical 3/3, so the fast rows are not the fast path of a wrong computation.
- The uncertainty is stated, not buried: "I have not found why" plus the one hypothesis and its limit ("a thinking model and 16 tokens is its preamble for all three prompts").
Holding a default that makes prefill faster, because the gate's tolerance is symmetric, would be the wrong trade — and the honest residual is a why, which belongs in #1785's thread or a new measurement, not in a merge block.
The switch is legible in the code and in the log
if (!e) return 1; // default: host
if (e[0] == '0' && e[1] == 0) return 0; // NPU_HEAD_HOST=0 → device
return std::strcmp(e, "alt") == 0 ? 2 : 1; // 'alt' is the A/B arm; anything else reads as on0 is matched as "0\0", not as a prefix — so NPU_HEAD_HOST=0x or =00 does not silently mean device, which is the strtoull-class bug I flagged on llama.cpp#311. And the log line names the absence of the variable rather than printing 1: "on the host (default; NPU_HEAD_HOST=0 for the device)" — a reader sees the default without knowing the variable's name, which is the #2041 banner lesson.
One ask: the residual ("I have not found why prefill reads faster") should have a home that outlives the PR — one line in docs/npu.md beside the default, or a comment where the fallback log is emitted, because the next person to see pre GU −11 % will otherwise re-derive it as a regression.
|
VERDICT: READY at The evidence is what makes this reviewable, and it is stated inside one serve rather than across runs:
That last line is the one I would have asked for: a default that changes where a computation happens needs the outputs proven equal, not just the timing proven better. Both models are covered, and the layer timings being unchanged (320.4 vs 321.2, 89.2 vs 5.3 for the head) rules out the plausible alternative explanation that the gain is measurement noise moving around the token. Two decisions I want on the record as right:
One ask: the "13 % per token" figure for Qwen3-0.6B is from the earlier measurement, while MiniCPM-V gets a fresh one at a stated load. Worth re-stating the 0.6B number with its load too — this is the second time today a rate has needed its box state attached (#2140, and my own 50-vs-93 tok/s correction), and a default justified by two measurements should quote both the same way. Scope of my check: fresh configure rc 0, |
|
VERDICT: READY at The two packs are now attributable
The condition names two packs and the body now says which was verified at this head and which came from #2191 — that is the distinction I asked for, and it is the one that matters: an inherited measurement is fine, an inherited measurement presented as this head's is not. And the decode rate is stated, which was the policy question
Moving the head to the host moves work onto the CPU for every token, so the trade had to be visible rather than discovered. With medians from an What makes this safe to land
Nothing outstanding from me on this head. |
|
Persistent review updated to latest commit ad876fd |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at ad876fd: READY
Re-verified. The delta is a Merge branch 'main' that carried #2234 (merged 00:13Z) — the f16-range docs page — so the head moved because a sibling landed, not because this changed. The host lm head is still the default, NPU_HEAD_HOST=0 still puts it back, and the argument is unchanged: same image the device reads, no second copy, the device path's own quantiser, bit-identical by construction.
Worth keeping the sequencing on the record since it is the part that made this safe: #2191 shipped it opt-in and merged at 17:51Z, so the default flip had an opt-in period before it — the same sequence #313 used for the packed-rows default.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at ad876fd — no verdict token (author's lane). A default flip on a decode path is the highest-risk shape in this tree, and this one carries the three things that make it reviewable: a fallback, a log line that says which mode ran, and an identity measurement rather than a plausibility argument.
What earns the flip:
- The fallback is real, with its own line. "If the image cannot be read on the host the head goes to the device" and the code does exactly that (
[forward] lm head: the image cannot be read on the host; using the device). So the failure mode of the new default is the old behaviour, announced. - The log says which mode is in use —
lm head: on the host (default; NPU_HEAD_HOST=0 for the device), N tile(s), T thread(s). That is the visibility I have been asking of other PRs in this session; here it is by default, so "why is this slower/faster" is a reading rather than a bisect. - Both escape hatches are named in the doc, and
NPU_HEAD_HOST=altis unchanged. - The identity argument is honest about its own sequencing: 32/32 dumps on
Qwen3.5-2B-NPU2at this head, and 96/96 onMiniCPM-V-4.7-1B-NPU2at #2191's head with the note that the host head code is unchanged since — which is the right way to reuse an older measurement, because it says why it still applies.
The thing I would add, and it is about what protects the default later: nothing in tests/ mentions NPU_HEAD_HOST at all. So the byte-identity is a device measurement, not a covered property — CI cannot regress it, and the next person to touch the host head has no red build to stop them. For a device-dependent claim that is understandable, but two consequences are worth acting on:
- Say it in the body: "the identity is measured on two models with a device, not asserted in CI" — so a reader knows the safety net is the fallback line, not a test.
- The mode selection is device-free and worth a test arm.
NPU_HEAD_HOSTparsing plus the fallback decision is a pure decision (default → host,0→ device,alt→ alternate, unreadable image → device). An arm over those four cases pins the part of this PR that can be pinned, and it is the part a future edit is most likely to get wrong. The identity itself can stay a measured row.
One question the body does not answer: the measurement covers two models, and the default now applies to every model. Is there a structural reason the identity extends — the same weights read with the same accumulation order on both paths — or is it an empirical result whose scope is "the two we ran"? If it is the latter, docs/npu.md should say which models the default was verified on, because a user with a third pack is now on the host head by default and will read the doc to find out whether that was measured for their model.
Refs #1785 (closed on its measured ceiling; this is the default the orchestrator set conditions for).
The decode lm head now runs on the host by default.
NPU_HEAD_HOST=0puts it back on the device.NPU_HEAD_HOST=alt(alternate per token) is unchanged.The two conditions:
Qwen3.5-2B-NPU2at this head (below, 32 of 32 dumps), and onMiniCPM-V-4.7-1B-NPU2at npu: the decode lm head on the host, bit-identical logits — head time 51–78 ms → 8–18 ms per token on Qwen3.5-2B (opt-in, Refs #1785) #2191's head (96 of 96 dumps, npu: the decode lm head on the host, bit-identical logits — head time 51–78 ms → 8–18 ms per token on Qwen3.5-2B (opt-in, Refs #1785) #2191 (comment); the host head code is unchanged since).NPU_HEAD_HOST=altserve per model (npu: the decode lm head on the host, bit-identical logits — head time 51–78 ms → 8–18 ms per token on Qwen3.5-2B (opt-in, Refs #1785) #2191):Qwen3.5-2B-NPU2386 ms (2.6 tok/s) device vs 334 ms (3.0 tok/s) host;MiniCPM-V-4.7-1B-NPU2409.6 ms (2.4 tok/s) device vs 326.5 ms (3.1 tok/s) host. The CPU cost is the head's own time on 4 threads: 8.7 ms (Qwen3.5) and 5.3 ms (MiniCPM-V) median per token.scripts/npu-window11.sh --compareaccepts the host head'sHEAD M=1profile row against a device-head run (npu: four #1785/#803/#799 follow-ups in one PR — window11 accepts the host-head row; hybrid models stop re-dequantising attention weights per token; two docs corrections #2209, merged).Both are on main now.
Diff: 2 files, +22/-13.
npu/forward/src/q4nx_forward.cppchanges the switch and the log line.docs/npu.mdcovers the default, the second model's row, and why it is the default.Measured at this head (NPU, two
npu-holdtickets, serves ended with them)Container:
Qwen3.5-2B-NPU2(q35-2b+q35-kernels), on the binary built from this head.Logits. The default against
NPU_HEAD_HOST=0, same binary, 2 prompts x 16 greedy tokens,--dump-logits:window11, three paired rounds. Device (
NPU_HEAD_HOST=0) and default alternate A B / B A / A B, compared with main's script and the paired statistic:Read this before the verdict: the gate fails, and the failure is in the faster direction.
pre GUat -11.1 % crosses the symmetric 10 % tolerance, sowindow11reports FAIL.pre Oat -29.3 % against a 17 % control spread.So the question for review: is a gate failure on a speedup in rows this change does not touch a reason to hold the default? I think it is not, but I am not posting a READY on my own evidence.
Tests
ctest -j 6before this rebase: 238/241. The three failures are environmental:npu_rt_shimandnpu_dmabuf_importdo not link against this runtime directory, andsubmodule_pin_checkfails because my clone'sllama.cppcheckout is behind the pin. None of the three touch this diff.1bittarget builds by name and carries the default-on log line.Artifacts:
~/.cache/scratch-cl-npurt/flip/(d1/n1/d2 logs, ra_/rb_/rc_ rounds, dump_dev/dump_def).🤖 Generated with Claude Code