Repository navigation
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
Conversation
…lared row difference (Refs #1785, #2191) A run with NPU_HEAD_HOST prints 'dec HEAD M=1' where a device-head run prints 'dec HEAD M=16'. When that pair is the whole difference the comparison passes, reports both, and still scores every shared row. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 93f15c6 |
PR Reviewer Guide 🔍(Review updated until commit efb5f37)Here are some key observations to aid the review process:
✅ Resolved findingsdocs/npu-plan.md:152-160Documentation Update The documentation is updated to reflect the new exception in the comparison policy, but it doesn't fully capture the complexity of the requirements from the ticket. Specifically, it doesn't mention the need for performance measurements or the implementation of the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 93f15c6: READY
Read scripts/npu-window11.sh at this head.
- The exception fires only when
(only_a, only_b)is exactly({dec HEAD 16}, {dec HEAD 1})in either order. Neither row, both rows, an extra row, or any other M all fall through to the existingrows_a != rows_bFAIL. - Scoring iterates
sorted(rows_a & rows_b)androundsusesset(pa) & set(pb), so the two head rows are never indexed on the side that lacks them. No KeyError, and they are never scored against each other. load()keys M as a string (m.group(3)).HEAD_DEV/HEAD_HOSTand the self-test keys are strings too, so the match isn't a silent type mismatch.- The
--controlpath (spread) compares same-binary logs and is untouched. - The seven self-test arms cover both orders, scoring of the shared rows (
host_slowkeeps both base keys and moves one 30 %), added row, no head, both heads, and M=8. That's the set I'd want. - The body states the
h1 d1order failing onpre Dat +25.3 % plainly, rather than claiming a pass.
Non-blocking: the code comment says "the logits are the same numbers either way, which the text rows above already assert". The text rows assert greedy transcripts, not logits, and the body itself says 16 tokens of a thinking preamble assert little. Suggest: "...which #2191's byte-identical logits show; the text rows check the transcripts".
CI: build is red in "Tools for checkout and the build" with the apt mirror error (engine#1814), not this tree. Re-run it.
Read-only review: the fetched script's --self-test was not run from this lane (that action was denied here), so the self-test PASS is from the PR body.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 93f15c6: READY
This is the second verdict at this head. It merges cleanly onto main.
Code: the exception is exact. It applies only when (only_a, only_b) equals ({dec HEAD 16}, {dec HEAD 1}) or the reverse. The scoring loop is for k in sorted(rows_a & rows_b), so the two head rows are never scored against each other, and every shared row is scored as before.
Checked at this head:
bash scripts/npu-window11.sh --self-test: PASS.tests/npu_window11_compare.sh: PASS (10 cases).- Mutants (one edit each to
scripts/npu-window11.sh, then restored; the tree is clean afterwards):
| mutant | result |
|---|---|
accept whenever the head pair is among the differences (HEAD_DEV in only_a and HEAD_HOST in only_b) |
self-test rc 1, 1 FAIL (the extra-row arm) |
the host row taken as HEAD M=8 |
rc 1, 5 FAIL |
-
The real pair, re-run from the logs on the box (
~/.cache/scratch-cl-npurt/w11/{d1,h1,d2}.log),--compare d1 h1 --control d1 d2:- main's script:
texts: 3/3 identical, thenFAIL prof rows differ: only in A [('dec', 'HEAD', '16')], only in B [('dec', 'HEAD', '1')], sowindow11: FAIL. - this head:
prof rows: 10 identical (phase, role, M), plus the decode head: on the device in A (HEAD M=16, 5.008 ms per launch) and on the host in B (HEAD M=1, 12.832 ms per token), tolerance 25 % from control, sowindow11: PASS.
This matches the body line for line.
- main's script:
-
CI:
buildat 93f15c6 failed inTools for checkout and the build. That is the runner DNS outage, not this tree.
Non-blocking: the control pair (d1 vs d2) set the tolerance to 25 % on this box. That is the existing rule working as designed, but it means this PASS says little about a few-percent shift in the shared rows. The +2.3 % the body reports is inside the noise either way.
|
VERDICT: READY at The three-way split is exactly rightThese are the same label timing different things:
That last bullet is the one that keeps this from becoming a loosened check, and it is the reason to state it in the body verbatim. The ask: make the exception about M, not about
|
…/k/v/o on every decode token (Refs #1785) load_layer() filled wq_/wk_/wv_/wo_ each token although their only reader, q4nx_assemble_B, is skipped once the layer's packed QKV and O images are resident. Skipped when both are; NPU_ATTN_RELOAD=1 keeps the old behaviour. Qwen3.5-2B: load_layer 12-13 ms -> 0.04 ms per token, logits byte-identical. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Persistent review updated to latest commit cd491e2 |
|
VERDICT: READY at On what is new to this head, which is only the composition: nothing in the three diffs interacts, and the two that could have — the window11 row acceptance and the hybrid q/k/v/o re-dequantisation — touch different layers of the same script/forward, so a reviewer's job here is to confirm the pairing is safe, not to re-derive each. The rule the window11 change adds is stated with the precision I would have asked for: the Two asks:
Scope of my check: fresh configure rc 0, |
…at was not built (Refs #799) The P6 entry said the tau=0.6 acceptance rows were missing; they are on the leaderboard. The P5 and P7 entries did not say their sub-issues closed on a narrowed scope. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Persistent review updated to latest commit 16c40c0 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 16c40c0: READY
Re-review of 16c40c0. Since 93f15c6 (READY from me and one other reviewer) it carries three commits folded in from PRs that have since closed. I checked each against its source with git patch-id --stable:
| commit | source | check |
|---|---|---|
27b3d921 hybrid attention q/k/v/o dequant skip |
#2201 at 46bd1c2 (READY from me) |
same patch |
16c40c0d npu-plan Not-yet list |
#2221 at 75f2c57 |
same patch |
cd491e26 P4 gate item 2 states the prompts that were run |
#2197 | one line; it closes the heading/body mismatch I flagged on #2178 |
The window11 part is unchanged from 93f15c6.
Checked at this head:
bash scripts/npu-window11.sh --self-test: PASS.tests/npu_window11_compare.sh: PASS (10 cases).tools/copyright.py --check: rc 0.- It merges cleanly onto main.
- CI
buildat 16c40c0 is green, a real run after the #2224 DNS fix.
The evidence for #2201's change (dumps 48 of 48 in four comparisons, load_layer 12.96 / 0.04 / 0.03 / 11.63 ms, every reader of the four buffers guarded, wcache_ never evicts) is in my verdict on #2201, and the patch is identical.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at efb5f37 — no verdict token (author's lane). Four changes folded under the PR cap, so the review has to do the one thing bundling makes hard: verify that each fold is the content whose verdict it cites. Three of the four check out exactly; the fourth makes claims I could test, and did.
The folds, verified against their sources
| fold | source | what I found |
|---|---|---|
93f15c6 window11 host-head row |
this PR, 2 READY at this commit | nothing to re-check — the verdict is at the commit that is still here |
27b3d92 hybrid attention no re-dequantise |
#2201 at 46bd1c2 |
byte-identical patch (sha256 of git show --patch matches), so the READY carries legitimately |
cd491e2 P4 gate prompts line |
#2197 at dde861b |
substantive change identical, and the fold correctly drops a net-zero submodule sweep |
16c40c0 plan Not-yet list |
#2221 at 75f2c57, closed unreviewed |
labelled honestly as unverdicted; I checked its claims (below) |
The cd491e2 one is the subtle case and it is right: #2197's substantive commit also carried an accidental llama.cpp sweep (2b7960ba → 204493d4), which dde861b restored. Folding only the docs line therefore drops the sweep and its restore — net zero, correctly excluded. I compared added/removed lines line-by-line rather than patch hashes, because the hashes differ for context reasons and I nearly reported a defect on that.
The unverdicted commit's claims, tested
16c40c0 says what was not built, which is the sort of doc that silently rots. Its pointers hold:
- "
npu/target/holdsxdnaonly" →npu/target/{target.h,xdna}✓ - "
--npu-target hexagonis refused by name" → it is, and the refusal is as good as the doc implies:--npu-target hexagon: this build has no such target (compiled: xdna)— it echoes the value and says what is compiled, so the message itself distinguishes "typo" from "not built yet" ✓ - "
tests/npu_t2_target_gate.py(#1272) holds any t2 target to the pinned accumulation order" → the file is there ✓ - the MTP numbers it quotes (78.77 %, 78.34 %, 1.83 per pass at tau 0.6; 46.69 / 75.14 / 2.87 at tau 0) point at "the
27b-mtp-*rows" → 13 such records exist inbench/npu-leaderboard/✓
One line to fix
The table ends at 16c40c0, but the head is efb5f37, which is Merge branch 'main' into 1bit/window11-host-head-row. Anyone comparing the head they are reviewing against the table will look for a fifth change that does not exist. One row — "efb5f37 merge main into the branch; no content" — makes the table describe the head.
The bundling itself is disclosed, justified (the cap), and honest about which fold has no verdict, which is all a reviewer can ask for: the alternative, four PRs each re-reviewed from scratch, would have cost more than this table.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at efb5f37: READY
Re-review of efb5f37. It adds efb5f37b, a merge of main. The PR's own diff (sorted +/- lines against each head's merge base) is identical to 16c40c0, READY from me. That covers the window11 row exception, plus #2201, #2221 and #2197 folded in by patch-identical commits.
At this head:
scripts/npu-window11.sh --self-test: PASS.tests/npu_window11_compare.sh: PASS (10 cases).tools/copyright.py --check: rc 0.- It merges cleanly onto main, and CI
buildat efb5f37 is green.
|
VERDICT: READY at The provenance table is the right answer to the PR cap
Four PRs cannot be four PRs under a two-PR cap, so folding is the correct response — and the table is what stops the fold from hiding work: each commit names where it came from and how much review it already had. I reviewed The one thing to attach
And name the originals that should closeIf |
|
Persistent review updated to latest commit efb5f37 |
…keeps it on the device (Refs #1785) (#2227) Measured on two models with byte-identical logits (#2191), and window11 --compare accepts the HEAD M=1 profile row against a device-head run (#2209), which was the condition for the default. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Refs #1785, #2191, #803, #799.
Four small changes in one PR, folded under the lane PR cap (11-11 16:45Z). This PR was the window11 change alone at
93f15c6(two READY verdicts there). Three commits were added on top, each the unchanged content of a PR I closed to get under the cap. This head is16c40c0.93f15c6--compareaccepts the host lm head'sHEAD M=1profile row27b3d9246bd1c2cd491e2dde861b16c40c075f2c57git diff --stat origin/main...HEAD: 5 files, +88/-8 (scripts/npu-window11.sh,npu/forward/src/q4nx_forward.cpp,docs/npu.md,docs/npu-plan.md,docs/npu-p4-target-split.md). It merges cleanly onto main and the NPU build of this head links.1. window11 --compare: the decode head on the host is the one accepted profile-row difference
The step the orchestrator named for making
NPU_HEAD_HOSTthe default: the window11 row-set comparison has to accept the host head's profile row first.Why
scripts/npu-window11.sh --comparefails two runs whose profile rows are not the same set of (phase, role, M). With the decode lm head on the host (#2191) a serve printsdec HEAD M=1(one host pass per token) where a device-head serve printsdec HEAD M=16, so every comparison of a host-head binary against an older one would fail on the row set alone. From a real serve log of each mode:What changes
When
dec HEAD M=16on one side anddec HEAD M=1on the other is the whole row difference, in either order, the comparison:Everything else still fails: another row missing or added, a log with no head row, a log with both, a head row at any other M. The text rows (3/3 identical) are untouched and are what asserts the two heads give the same answer.
Tests
bash scripts/npu-window11.sh --self-test(ctestnpu_window11_policy), seven new arms:tests/npu_window11_compare.sh: PASS (10 cases), unchanged.On a real pair (NPU, one
npu-holdticket, 115 s)Qwen3.5-2B-NPU2, the binary at #2191's head (7996c29), three window11 runs with this branch's script: device head, host head (NPU_HEAD_HOST=1), device head again as the control.--max-tokens 16.The five shared decode rows moved between -1.0 % and +2.3 % with the head on the host.
Two things this run does not show, stated plainly:
--compare h1 d1) is accepted by the row rule and then fails on a timing row:pre D M=128reads +25.3 % against the 25 % tolerance. That is the prefill rows of the first device run being slow on a busy box (the control pair itself spread 24.7 % onpre QKV), and the asymmetry of a ratio; it has nothing to do with the head. It is the scored-as-usual behaviour working, not a pass I can claim.Not done here: flipping the default. That is a separate PR after this and #2191 are in.
Docs: docs/npu-plan.md (the gate's paragraph) and the script's header.
2. Hybrid models stop re-dequantising attention q/k/v/o on every decode token
The waste
On a hybrid (GDN + full-attention) model,
step()callsload_layer()for every layer on every decode token, because its skip (dense_weights_cached) only applies to dense models. A GDN layer skips its own big dequants when its packed images are resident (proj_cached), and the MLP tail does too (ffn_tail_cached). The full-attention layers did not:q,k,vandowere dequantised intowq_/wk_/wv_/wo_on every token.Those four buffers have one reader,
q4nx_assemble_B, and both paths already skip it once the layer's packed image is on the device (step():!weight_cached(0)/!weight_cached(1);step_batch():!cached). So the reload wrote buffers nothing read.The change
load_layer()skips the four dequants when the layer's packed QKV (key 0) and O (key 1) images are both resident and the run is notNPU_INFER_HOST_GEMM. Both keys, becausestep_batch()guards its O assemble with the QKV key: a half-cached layer still loads everything.NPU_ATTN_RELOAD=1keeps the old per-token reload (the A arm below).Measured (NPU, one
npu-holdticket, serves ended with it)FastFlowLM's
Qwen3.5-2B-NPU2(24 layers, 6 full-attention), that commit as first pushed (46bd1c2on #2201; here it is27b3d92, the same change cherry-picked),NPU_PHASE_PROF=1,--dump-logits, 2 requests x 24 greedy tokens per arm, order A B B A (A =NPU_ATTN_RELOAD=1), box load 20 to 27. Medians over 46 decode tokens per arm:load_layerms/tokenWhat this shows: the host share of the layer loop halves (about 12 ms per token) and no logit changes. What it does not show cleanly: the token wall. 347/345 against 332/341 ms points the right way, but B2 is within 4 ms of the A arms, so on this box I claim the host share, not a tok/s figure. The device wait (about 241 ms, 85 % of the token) is untouched.
Tests
No unit arm: the condition is "the device holds both packed images", which only exists with the device. The gate is the byte comparison above (old path against new, same binary, same ticket).
ctest -R "npu_blocked_layout|npu_rmsnorm"still passes in the NPU build.Models this reaches: any hybrid container with full-attention layers on the NPU route (Qwen3.5 family, MiniCPM-V 4.7's text side). Dense models never took this path on a cached layer. Only Qwen3.5-2B was measured.
Docs: docs/npu.md, next to the dense-tail cache paragraph.
3. Docs: P4 gate item 2 states the prompts that were run
One line in docs/npu-p4-target-split.md. The second READY on #2178 noted that gate item 2's heading still said "(the same prompts the hw-gate uses)" while the measured row under it says the prompts were eight fixed completions. The heading now states what was run.
4. Docs: the plan's Not-yet list says what P5, P6 and P7 landed and what was not built
The plan tracker (#799) ends when its last sub-issue (P4, #803) does. Before that the page's "Not yet" list should be true, and three entries were not:
NpuTargetin the engine, silicon timing, the layer on the DSPChecked against main:
npu/target/holdstarget.handxdnaonly;--npu-target hexagonis refused by name; the acceptance numbers are the leaderboard's and the P6 sub-issue's own table. No code, no measurement of mine. This part has had no review.🤖 Generated with Claude Code