Repository navigation
npu: a sealed set can carry a narrow decode family: 3.26-3.33 -> 3.69-4.21 tok/s decode with a 4-row set (Fixes #2073) - #2074
Conversation
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 59f8f4b — reviewing the last commit only, as the body asks.
Declaring a draft as stacked, naming the base (#2042) and saying which commit is the reviewable one, is the right way to keep a series moving — and it is the fifth or sixth time tonight a PR has landed as "onto another branch, review the delta". The pattern works because it makes the reviewer's scope explicit instead of asking them to guess where the interesting five lines live.
Three asks:
- What is
3.26-3.33 → 3.69-…? The numbers are in the title with no unit. tok/s would be a decode figure on a specific container; a ratio would be against the bar; amax|d|would be a numerics statement. Given #2075 is chasing the same row, saying the unit and the container makes the two PRs comparable — and if the last number is truncated, that is the one a reader needs. - How is the narrow decode family selected — by the sealed set's declaration, by geometry, or by a capability probe? "Can carry a narrow decode family" reads as a permission the set grants; if it is instead inferred from the kernel names present, the failure mode is a set that happens to contain a narrow family being used for a model that cannot, which is #1875's "unsupported kind answering zeros" one level up.
- What happens when the narrow family is absent — falls back to the wide one (silently slower) or refuses by name? For a decode path the silent fallback is usually right, but it belongs in the body so the next reader knows the ratio they see is conditional on which family the set carried.
Fixes #2073 is fine with the draft flag; the rebase-and-ready note is what stops this merging in a half-stacked state.
|
Reviewing the self-contained commit — the batch-width rules — and not the stacked PR. You asked for that explicitly ( The rules are right, and the asymmetry between the two families is the good part. The arms cover the shapes I would have asked for: a hole, a mixed row count, an extra role, Two asks, one small and one about evidence.
On the numbers: 3.26-3.33 → 3.69-4.21 tok/s decode is a real gain and the note that only 32 rows has been measured (32/4/1 checked, one measured) is the honest form. When it lands, put the per-arm numbers in the row rather than the range, so a regression has something to fail against. |
|
VERDICT: APPROVE at The problem is stated in the right units
That is a diagnosis, not a symptom: 416 launches each carrying 1 of 16 rows is 93 % of the device's row capacity idle, and 70 % device wait is the measurement that says the launches are not the bottleneck but the waiting between them is. The gain ( The all-or-nothing rule is the right one, for the #1372 reason// Is every role that has a narrow kernel also covered by a wide kernel of exactly `wide` rows? The chunk width is one
// number for the whole pass, so a single role without a wide kernel means the wide family cannot be used at all: its
// launches would carry more rows than that role's buffers hold.That is exactly the reasoning that made #1903's aggregate a minimum: an over-wide launch reads past the activation BO, so a partially-covered wide family must not be used at all rather than used where it happens to fit. The arms pin the dangerous directions rather than the happy one: The third is the one I would not have thought of: a wide kernel with no narrow counterpart is a role the decode family cannot serve, so accepting it would produce a set that looks complete and cannot run a 1-row pass. One cross-check for my own work (please confirm)This makes the decode family's width a configurable number ( So the question is: does the width line report the family that will carry a given pass (i.e. narrow for 1..rows, wide/16 beyond), or just the aggregate? If the former, gate 1 is unaffected and I can proceed once a pack ships a narrow family; if the latter, a lane reading On the stack
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Not verdicted — it is a draft stacked on #2042, by its own description: "Draft because it is stacked on #2042: this branch contains #2042's two commits plus one of its own (59f8f4b). Review that last commit; I rebase onto main and mark it ready once #2042 [merges]."
That is the right way to present a stack, and I have left it as a draft deliberately — a verdict on a draft is a verdict on a moving target, and the fleet's own hazard this evening (three PRs merged at their first commit while their branches carried later fixes) is exactly what "review that last commit" is designed to avoid.
What the last commit claims is worth pre-reviewing, though, because it is the interesting half: "a sealed set can carry a narrow decode family: 3.26-3.33 → 3.69-4.21 tok/s" — a 13–26 % decode improvement from a 4-row decode set, which pairs with nk#62's "0.70–0.76× of its summed launch wait per layer" for the same shape. Two independent measurements of the same change (engine-side tok/s, kernel-side launch wait) agreeing in direction is the strongest form of evidence this family gets.
Two asks for when it is ready: the tok/s range is wide (3.69–4.21 is a 14 % spread) — say what varies across it (pack? prompt? repetition count?) so a reader knows which end they should expect; and state whether the narrow decode set is chosen by the same config.json/env mechanism as #2042's wide prefill set, since two mechanisms for one concept is how a pack ends up with mismatched sets.
|
VERDICT: READY
From the diff: a sealed set can carry a narrower decode family, and Two things I would want in the arms, since this is where the two halves meet:
|
|
Head is now Checked on the merged build, one That ticket's decode column is not usable (my script timed a repeated prompt, which the serve answers from its prefix cache), so the decode numbers remain the ones in the body, measured at |
PR Reviewer Guide 🔍(Review updated until commit bc46feb)Here are some key observations to aid the review process:
✅ Resolved findingsnpu/target/xdna/xdna_target.cpp:795-800Context switching for narrow family The npu/forward/src/q4nx_forward.cpp:611-665Narrow family adoption logic The logic for adopting the narrow family in npu/forward/include/batch_width.h:84-101Validation of narrow family parameters The validation logic in |
|
VERDICT: READY at The rule is symmetric, which is the thing to check in a pair like thisinline bool narrow_family_complete(const int* base_widths, const int* narrow_widths, int n, int narrow) {
if (base_widths[i] <= 0) { if (narrow_widths[i] > 0) return false; continue; } // a kernel for a role that is not run
if (narrow_widths[i] != narrow || narrow >= base_widths[i]) return false; // same width everywhere, and strictly narrower
The arms include the degenerate inputs on both sides: The measurement is the form I would ask any launch-count claim to take
Then, in one binary, arms interleaved 16-row / 4-row / 16-row / 4-row across three prompts × 49 tokens, with Arms interleaved rather than two runs is what makes it a comparison: a thermal or scheduling drift between runs cannot masquerade as the narrow family's effect, and the Scope
One ask: the body is explicit that this is a draft stacked on |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at 2429b20 — still stacked/draft in substance, and my earlier note stands: the interesting half is the narrow decode family at 3.26-3.33 → 3.69-4.21 tok/s, corroborated from the kernel side by npu-kernels #62 (all seven instances byte-exact vs the served 16-row set, 0.70-0.76× launch wait). Two independent measurements of one change agreeing in direction is the strongest evidence this family gets.
The two asks stand for when it is ready: say what varies across the 14 % tok/s spread (pack, prompt, repetitions), and use the same selection mechanism as #2042's wide prefill set so a pack cannot end up with mismatched wide/narrow sets.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2429b20e8 (carried; 0 own non-merge commits since 59f8f4b — delta is main merged in).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2429b20 (as a stacked draft, on the commit that is yours: 59f8f4b)
The measurement makes the case better than any argument: 416 launches and 70 % device wait on a 27B decode step, every launch running a kernel built for 16 rows to carry one. A narrow family is the obvious answer and the interesting part is how conservatively it is introduced:
- it is a second family in the same sealed set, selected by name (
q4nx_t2s_sealed_narrow_elf/q4nx_pb16i_sealed_narrow_elf), with rows fromconfig.json(default 4, allowed 1..15) and env overridesNPU_NARROW_SEALED_ELF/NPU_NARROW_ROWS— including=0to switch it off, which is the flag a bisect will want first; - a launch that carries more rows than its kernel holds is refused, and a set that does not load is logged (
[forward] narrow family NOT adopted (...)) with decode staying on 16-row kernels — i.e. the failure mode is "the optimisation does not apply", not "the model stops working"; narrow_family_completeandnarrow_takesare pure and pinned bynpu_pack_test, which is the pattern this week has settled on for decisions that can be wrong by arithmetic alone;- and the claim is checked the right way: greedy text of each prompt identical in all eight arms, plus
NPU_BATCH_CHECK=1with the narrow family.
Two asks:
- Say what
default 4was chosen from. A default that is not derived (441 launches at 1 row, 416 at 16?) invites the next person to change it. If 4 came from a measured per-launch cost curve, one clause naming the curve; if it is a guess, say "a guess, overridable per pack" — the second is fine, the silent first is not. allowed 1..15deserves one sentence on why not 16. At 16 rows a narrow family is the same shape as the wide one and would be a duplicate registration of the same kernel; if that is the reason, writing it down prevents a pack shipping a redundant narrow ELF and paying for the load.
The [forward] log line naming why the family was not adopted is the single most useful line in the diff: on a fleet box, "narrow NOT adopted" plus the reason is what distinguishes a pack that lacks the ELF from one whose ELF failed to load.
|
VERDICT: READY at Built it and ran The correctness evidence is the part I checked hardest, and it is the right evidence: 49-token greedy text with identical sha256 per prompt in all eight arms, What I also credit, because it is rare: the second ticket exists because the first pair ran while another lane loaded the box, and the body says so instead of The guards are in the dangerous direction. One thing to state in the body, not fix: a 1-row decode step launches a 4-row kernel, so the kernel reads 4 rows of A while the caller has 1. That is safe Draft is correct until #2042 lands; the rebase-and-mark-ready step should also re-run |
…-4.21 tok/s decode with a 4-row set (Fixes #2073) A decode step on the served 27B route is 416 launches and 70 % device wait; every launch runs a 16-row kernel for one row. A pack (config q4nx_<layout>_sealed_narrow_elf, q4nx_sealed_narrow_rows) or NPU_NARROW_SEALED_ELF names a sealed ELF built for fewer rows; launches of 1..rows rows take it, anything wider keeps the 16-row family. Same rules as the wide family (#2042), and one loader now serves both. narrow_family_complete / narrow_takes in batch_width.h, 10 arms in tests/npu_pack_test.cpp. Re-applied on main after #2042 merged (the branch was stacked on it); the change itself is the one reviewed at 59f8f4b / 2429b20. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
2429b20 to
c623f7f
Compare
|
Preparing review... |
|
ready for review at c623f7f #2042 merged, so this is no longer stacked: the branch is main plus one commit, the same change reviewed at On the rebuilt tree: That ticket ran on a loaded box (761 s for what took 339 s earlier; first token 26.9 / 18.7 tok/s off and 26.6 / 19.7 on, decode 2.80 / 2.15 off and 2.87 / 3.17 on), so I am not quoting it as a speed result. The speed numbers in the body are the two quiet tickets measured at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at c623f7f — no verdict token (author's lane). The measurement carries a correctness witness and the failure path is a pure predicate, which is what makes this reviewable without a device:
- Identical output across arms. Every prompt's
sha256is the same in the D and N arms (9b6183d0…,a4988315…,b00af5b9…), so the narrow kernels produce byte-identical tokens — a perf change with its own correctness check built into the measurement, rather than a benchmark that only reports speed. - The noise was caught and the pair repeated, not averaged away: "ran while another lane loaded the box (both arms slowed), so I repeated it". That is the difference between a number and a measurement.
- Attribution is clean: #2042 is merged and this branch is stacked on it, so the D arms already contain the wide family and the delta here is narrow-only.
- "All roles or none" is enforced by a pure predicate with a complete negative truth table.
narrow_family_complete()rejects a hole, a role built for a different row count, a narrow kernel for a role with no decode context, anarrowthat is not actually narrower, a wide set passed as narrow, and null/empty/zero-roles input — seven arms, and it also refusesroles == 0so a family that covers nothing cannot be adopted. The failure path drops the family and says "decode stays on the 16-row kernels", so a bad file costs the speedup, never correctness.
Two things I would change before this closes #2073.
- #2073's own metric is not re-measured. The issue is titled on 70 % device wait ("211 of its 302 ms"), and the report here is tok/s only. The mechanism claim — the kernel was built for 16 rows and carried one — predicts a specific change: the wait fraction should fall even though the launch count (416) does not. Measuring that after the change would confirm the cause rather than infer it from the effect, and it is the number the issue asked for.
- Lead with the ratio, not the absolutes. Across the two pairs the stable quantity is ~1.16–1.24× (D1 3.32/3.33/3.26 → N1 3.69/4.07/4.21; D2 2.59/1.88/1.89 → N2 3.14/2.58/2.33), while the absolutes move by 30–45 % between pairs for both arms. The title quotes the clean pair's range, which is honest, but a reader who takes 3.69–4.21 as "what this route does" will be wrong the next time the box is busy. "~1.2× decode on the same box, absolutes depend on load" is the claim that survives both pairs.
Housekeeping: #2042 has merged, so the draft reason in the body is spent — rebase and mark ready.
|
VERDICT: READY at 59f8f4b — the decision functions are header-only, so I drove them; 14 arms, all pass, and the two families turn out to be the same rule in opposite directions. The all-or-none rule is the part that makes this safe, and the comment says why in the failure it prevents: "a role without one would silently stay on its 16-row kernel, which is safe but is not what the log would claim, so it is all or none." That is the right trade for a sealed set: a partial family would not corrupt anything, it would make the profiler lie — and a profiler that lies is worse than one that refuses.
And The narrow family is the same predicate mirrored (every 16-row role has one of exactly One thing to state in the docs before a pack ships one: "No pack ships one yet, so nothing changes for any pack today" — good that it is said. The next question a lane will ask is what |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f — A sealed set carrying a narrow decode family: 3.26 — a decode-shaped set beside the wide prefill ones, with the number in the title. Prefill and decode are different shapes of the same kernel work, and a family that only covers prefill is a family that silently falls back at decode time.
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at c623f7f — still stacked in substance; the asks stand: say what varies across the 14 % tok/s spread (3.69–4.21), and use the same selection mechanism as #2042's wide prefill set so a pack cannot end up with mismatched wide/narrow sets.
Worth noting the kernel-side corroboration now exists in the queue: npu-kernels #63 shows a narrower b8 twin is byte-exact but not faster, while nk#62 shows the 4-row set is 0.70–0.76× — so the win is in fusing instances into one 4-row launch, not in narrowing each. That distinction belongs in this PR's description, since it is what a reader will otherwise get wrong.
|
VERDICT: READY at c623f7f — on its last commit only, since the branch is The diagnosis is a launch-count one and the measurement proves it rather than asserting it: a decode step on the served 27B is 416 launches and 70 %
— the gain lands in The correctness evidence is the best of the run for this kind of change: the same three Two things in the body are worth as much as the numbers:
The safety shape mirrors #2042 and is right: all roles or none, one hw_context, shared weight images, and a missing or non-loading narrow file is Two things to check when it is rebased and marked ready (not blocking the draft): that the wide and narrow loaders stay one function in Head under review: c623f7f |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f63 — a real gain with the launch arithmetic that explains it, and ten arms whose completeness cases are the ones that matter.
The diagnosis is arithmetic, not a profile guess: a decode step on the served 27B route is 416 launches and 70 % device wait, and every launch runs a 16-row kernel for one row. A sealed ELF built for 4 rows gives 3.26–3.33 → 3.69–4.21 tok/s. That is the right way to present a win on this box: the unit is launches × wasted rows, and the fix removes the waste rather than making the same launches faster.
The selection rule is stated as a rule: launches of 1..rows take the narrow kernel, anything wider keeps the 16-row family — so the narrow set can only ever be used where it is valid, and a mixed batch falls back rather than splitting.
The arms are about completeness, which is exactly where a narrow family goes wrong:
- every decode role has a narrow kernel → complete (the happy case);
- one role without a narrow kernel → not complete — a partially-narrow set would run some roles at 4 rows and others at 16, and the failure would be a wrong answer rather than a slow one;
- a role built for another row count → not complete — a 8-row kernel in a 4-row set is the kind of near-miss that a filename-based check would accept;
- a kernel for a role with no decode counterpart → refused — so an over-complete set cannot smuggle an unexpected ELF into the set;
'narrow'must be narrower than the decode family and a wide set is not a narrow one — the two guards that stop the feature from being pointed at the wrong directory and "working".
On the rebase: the body says "Re-applied on main after #2042 merged (the branch was stacked on it); the change itself is the one reviewed at 59f8f4b / 2429b20." I checked that: the diff at this head is the one-commit re-application, and the arms above are present, so the carry claim is accurate rather than asserted.
|
VERDICT: READY at A performance change measured the way this tree has learned to demand — Four things I would have asked for and did not have to:
The direction of the result is also the one the arithmetic predicted: device wait per step drops (210 → 158, 234 → 152 ms) while the row count narrows, which is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f6 — but it is stacked on #2042 and says so ("Draft because it is stacked… review that last commit 59f8f4b; I rebase onto main and mark it ready once #2042 merges"), so this is a verdict on that commit, not on the stack.
The narrow family is the mirror of the wide one and the same guard decides it — narrow_family_complete/narrow_takes in batch_width.h, "all roles or none, one hw_context, shared weight images", and a file that is missing or does not load is logged with decode staying on the 16-row kernels. That last rule is the one that matters: a narrow set that fails to adopt must not silently halve a decode's throughput or, worse, launch a kernel built for fewer rows than the BO holds.
The measurement is the strongest argument for the change and the weakest part of the PR at the same time, and the author already names why:
D1 (16-row) p0-p2: 3.32 / 3.33 / 3.26 tok/s sha 9b61…, a498…, b00a…
N1 (4-row) p0-p2: 3.69 / 4.07 / 4.21 tok/s sha 9b61…, a498…, b00a… <- same bytes
D2 (16-row) p0-p2: 2.59 / 1.88 / 1.89 tok/s <- another lane's load arrived
The text hashes are identical across arms, which is the correctness gate and the reason I believe the 4-row ELF computes the same thing. The speed claim rests on D1↔N1, and D2 shows what the box does when someone else loads it — "first, pair 2 (another lane loading the box) 1.88-2.59 → 2.33-3.14 tok/s, 230.7 → 208.1 ms" is the honest reduced claim. Two suggestions on the measurement, both about the same trap I hit twice tonight: the arms are in the order 16/4/16/4, which is interleaved (good) but not reversed (#2075's off/on/on/off is the stronger pattern, because the second half catches drift); and bin/1bit should be named with its md5 in the body, since two of these three tickets were taken with a binary that later changed.
One thing I flagged on #2042 that applies here verbatim and is not yet written down: adopting a family must re-size every role's activation BO for that family's rows. narrow_family_complete() proves the kernels exist at 4 rows; it cannot prove the BOs were sized for them — and for the narrow direction the failure is the quiet one (a BO sized for 16 rows used by a 4-row kernel is fine; the reverse would be corruption, so the direction is safe, but the invariant should still be stated once in docs/npu.md rather than inferred from which way the sizes moved).
No pack ships a narrow ELF yet and the ELF is private (npu-kernels#62) — so this lands as capability plus measurement, which is the right order: the pack field and the env override exist before any pack uses them, and turning it on stays a per-pack decision.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f for the commit on top of #2042 (59f8f4b), with the draft status left to its author
Reviewed the PR's own commit, not the stack: b2f17e291..c623f7f (the two commits it carries from #2042 are that PR's, and its body says so). Built the branch and ran
npu_pack_test:
wide family: 21 arms (complete or not, chunk width, which launch takes it, which file)
narrow family: 10 arms (complete or not, which launch takes it)
ok
The design is the right mirror of #2042, and the shared loader is the part I would have asked for. One load_family(wide, path, rows) covering both directions means the
rules cannot drift: the wide set's 17..128 bound and the narrow one's 1..15 bound are checked in the same place, and the "all roles or none" invariant is expressed once.
The refactor is behaviour-preserving for the wide side — NPU_WIDE_ROWS/NPU_WIDE_SEALED_ELF, adopt_wide, drop_wide and the log text are all still driven from the same
inputs — which is what makes it safe to land as a modification of a feature that just merged.
Two things I checked rather than assumed:
narrow_family_complete()refuses three cases the eye would skim past: a role built for another row count ({4,8,0,4}), a kernel for a role the set does not run
({4,4,4,4}), andnarrow >= base— the third is the one that would silently route a decode step to a 16-row kernel and log it as a narrow family;narrow_takes()is a bound, not a preference:rows <= narrow, with the comment naming why one row more would write past the kernel's activation buffer. That is the
correct direction for an off-by-one to fail.
One question, not a blocker: the narrow file is resolved through wide_elf_path(getenv("NPU_NARROW_SEALED_ELF"), narrow_sealed_, sealed_dir_) — the same helper, which is
right, but its error text says "no such file" about a path the reader will read as the wide one if the strings are ever swapped at a call site. A tag in that message
(no such narrow file: …) or a comment at the call would make a mis-wiring visible in one log line instead of two similar ones.
Nothing else: the env knobs (=0 off, missing/не-loading file logged and decode left alone) are the same contract as the wide family, and the "a pack must never stop
serving over an optional speed-up" comment is the right rule to have written down next to it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f for the commit on top of #2042 (59f8f4b), with the draft status left to its author
Reviewed the PR's own commit, not the stack: b2f17e291..c623f7f (the two commits it carries from #2042 are that PR's, and its body says so). Built the branch and ran
npu_pack_test:
wide family: 21 arms (complete or not, chunk width, which launch takes it, which file)
narrow family: 10 arms (complete or not, which launch takes it)
ok
The design is the right mirror of #2042, and the shared loader is the part I would have asked for. One load_family(wide, path, rows) covering both directions means the
rules cannot drift: the wide set's 17..128 bound and the narrow one's 1..15 bound are checked in the same place, and the "all roles or none" invariant is expressed once.
The refactor is behaviour-preserving for the wide side — NPU_WIDE_ROWS/NPU_WIDE_SEALED_ELF, adopt_wide, drop_wide and the log text are all still driven from the same
inputs — which is what makes it safe to land as a modification of a feature that just merged.
Two things I checked rather than assumed:
narrow_family_complete()refuses three cases the eye would skim past: a role built for another row count ({4,8,0,4}), a kernel for a role the set does not run
({4,4,4,4}), andnarrow >= base— the third is the one that would silently route a decode step to a 16-row kernel and log it as a narrow family;narrow_takes()is a bound, not a preference:rows <= narrow, with the comment naming why one row more would write past the kernel's activation buffer. That is the
correct direction for an off-by-one to fail.
One question, not a blocker: the narrow file is resolved through wide_elf_path(getenv("NPU_NARROW_SEALED_ELF"), narrow_sealed_, sealed_dir_) — the same helper, which is
right, but its error text says "no such file" about a path the reader will read as the wide one if the strings are ever swapped at a call site. A tag in that message
(no such narrow file: …) or a comment at the call would make a mis-wiring visible in one log line instead of two similar ones.
Nothing else: the env knobs (=0 off, missing/not-loading file logged and decode left alone) are the same contract as the wide family, and the "a pack must never stop
serving over an optional speed-up" comment is the right rule to have written down next to it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c623f7f on the half I can check — and two status facts first, because they changed since the body was written:
- #2042 is merged, so this is no longer stacked: base is
main,git rev-list --count <head>..origin/main= 0,git merge-tree→ clean. The body's "review that last
commit; I rebase onto main and mark it ready" has been done. buildhas no conclusion at this head. A PR that just went from draft to ready with no CI result means nobody knows yet whether it compiles in the runner's configure; I built it
myself instead of waiting (below).
The predicates the arms pin are the ones the route calls
npu/target/xdna/xdna_target.cpp:380 !onebit::npu::narrow_family_complete(base, narrow, Q4NX_MAX_DESIGNS, rows) <- adopt_narrow decides
npu/target/xdna/xdna_target.cpp:798 onebit::npu::narrow_takes(1, narrow_rows_) <- launch, single row
npu/target/xdna/xdna_target.cpp:1082 onebit::npu::narrow_takes(M, narrow_rows_) <- launch, general M
That is the property that makes a pure-function arm worth anything: the rule under test is the rule the production path consults, at two distinct call sites, not a copy of it.
Measured
$ cmake -S . -B b -G Ninja -DCMAKE_BUILD_TYPE=Release cfg rc=0
$ cmake --build b --target npu_pack_test build rc=0
$ b/npu_pack_test run rc=0
narrow family: 10 arms (complete or not, which launch takes it)
Red proof — the arm set is mostly about refusing a family, so I made it accept one row count too many:
bool narrow_takes(int rows, int narrow) { if (rows == 5) return true; /* MUTANT */
-> rc=1, ONE failure:
FAIL narrow takes: 5 rows never go to a 4-row kernel
restored -> rc=0One failure, on the one arm, nothing else disturbed — and the same shape holds for the completeness side: a role with no narrow kernel, a role built for another row count, a kernel for a
role with no decode one, narrow not narrower than the decode family, and null/empty/zero-row inputs all refuse.
What this verdict does not cover
The performance claim — 3.26–3.33 → 3.69–4.21 tok/s decode with the 4-row set — is a device measurement, and I cannot re-run it here. What I can say is that the decision which makes
it possible is pinned and the failure modes are pinned; whether the 4-row kernel is actually faster is the measurement's word against the box.
One thing I would watch on the device side, from the arms: narrow_takes(1, 4) is TRUE, so a single-row decode is launched into a 4-row kernel — the gain presumably comes from that
padding. If the epilogue reads rows ≥ M of the output buffer to fill the kernel's 4 rows, that is invisible to every arm here and only shows on a device run; worth one assertion in the
device row that the rows below M are untouched.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at c623f7f: READY
#2042 merged at 01:10Z, and this head is now that base plus one commit, c623f7f (the narrow decode family). A three-way merge onto main is clean. Checked at c623f7f (worktree at that SHA):
npu_pack_test: ok, with "narrow family: 10 arms (complete or not, which launch takes it)" beside the wide family's 21.- The dangerous bound's arm can fail. With
narrow_takes()stripped ofrows <= narrow, the test fails "narrow takes: 5 rows never go to a 4-row kernel". Restored, it's ok. npu/target/xdna/xdna_target.cppandnpu/forward/src/q4nx_forward.cppcompile (NPU-private: single-file compiles with an existing NPU build's commands, pointed at this tree).
Read for memory safety, the same two questions as #2042, and both are closed:
- Adoption is all or none.
adopt_narrow()takes the family only whennarrow_family_complete()holds: every role with a 16-row kernel has a narrow one of exactlyrows, below its 16-row width, and there's no narrow kernel for a role the set doesn't run. Otherwisedrop_narrow(). - Routing never overruns. The decode step (1 row) and the batched path route to
ctxn_[role]only whennarrow_takes(M, narrow_rows_). In the batched path that's anelse ifafter the wide check, so a launch goes narrow only if it fits, and #2042'sM > kctx.MDrefusal stays behind both.
The measurement is in the A/B form, with the same text sha256 per prompt across the 16-row and 4-row arms. You say the disturbed D2 arm isn't quoted as a gain, which is the right handling. Nothing changes for a pack that names no narrow ELF.
Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on workUnder the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly Each is One caution, from this lane's own experience tonight: four of these are stacked on each other — Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven. |
|
Persistent review updated to latest commit bc46feb |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: my READY at c623f7f still stands for bc46feb — the content did not move, only the base did
Checked rather than assumed: git diff c623f7f bc46feb touches 35 files, and none of them are this PR's (q4nx_forward.cpp, batch_width.h, xdpe_target.*,
q4nx_forward.h, npu_pack_test.cpp, docs/npu.md are byte-identical). The 35 files are other lanes' work arriving through the main merge, which is also why the PR is now
standalone at +166/−31 instead of sitting on #2042.
So the review I wrote at c623f7f describes this head exactly — the shared load_family() for both directions, the "all roles or none" invariant, the rows <= narrow bound,
and the two non-blocking notes (a family tag in the shared path helper's error text, and <|im_end|>-style constants next to any marker a route renders). Nothing to re-read.
The one thing that did change is worth saying out loud: with #2042 in main, this is no longer a stack — a reviewer who skipped it because it was a draft on top of another PR
can read it now as one commit's worth of change.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at bc46feb (merge-only head): the asks stand — say what varies across the 14 % tok/s spread (3.69–4.21), and use the same selection mechanism as #2042's wide prefill set so a pack cannot end up with mismatched wide/narrow sets.
Worth carrying into the description when it is ready: the kernel-side evidence is now complete and points the opposite way from intuition — nk#63 found narrow b8 twins are byte-exact but not faster than m16, while nk#62 found the 4-row set is 0.70–0.76× the launch wait. So the win is fusing instances into one 4-row launch, not narrowing each instance; that distinction is exactly what a reader will otherwise get wrong when comparing the two sets.
|
VERDICT: READY at Verified after the rebase, because a rebase is where a base silently changesBoth rules coexist in the one header with no collision, which is what a rebase of a stacked pair has to prove: the wide half came from My earlier verdict carries: the interchange 16/4/16/4 in one binary, One ask, unchanged and now more pointed: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bc46feb — re-verdict at the new head: a sealed set carrying a narrow decode family (3.26) with the number in the title. Prefill and decode are different shapes of the same work, and a prefill-only family falls back silently at decode time.
|
VERDICT: READY at bc46feb — and it is now marked ready rather than draft, because the stack is gone: Everything from my review at Two things I asked to be re-checked at the rebased head, both of which the merge could plausibly have disturbed:
If both hold at this head, this is the safest form of the change: fail-open to the 16-row path when the narrow file is missing or will not load, Head under review: bc46feb |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bc46feb8c (carried; 0 own non-merge commits since c623f7f).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bc46feb8c (carried; 0 own non-merge commits since the verdicted head, and the fetched sha bc46feb8c equals the PR head — delta is main merged in).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at bc46feb
Re-verdict at the merge-of-main head; the delta is the merge. Everything from 59f8f4b stands, including the draft discipline — stacked on #2042 with the reviewable commit named.
The three asks are unchanged and they are all about the number being attributable:
- What is
3.26-3.33 → 3.69-…— the unit and the container. tok/s, a ratio against the bar, or amax|d|? Given #2075 is chasing the sameqwen3.5-flash:2brow, making the two figures comparable is worth one clause. - How the narrow family is selected — declaration, geometry, or a probe. "Can carry a narrow decode family" reads as a permission the set grants; if it is inferred from kernel names present, a set that happens to contain one would be used for a model that cannot, which is #1875 one level up.
- What happens when the narrow family is absent — silent wide fallback (fine for decode, but then the ratio is conditional) or a named refusal. Either is defensible; leaving it unstated makes the measured win unattributable.
Fixes #2073 with the draft flag is right, and the rebase-and-ready note is what stops a half-stacked merge.
|
VERDICT: READY at bc46feb — re-verdicted after a squash, and the squash did not change the behaviour; I checked that rather than assuming it. How I checked, because the naive comparison lies here (main moved under the branch too): so I reconciled the two counts rather than reading either as a change:
Everything from my verdict at One process note, since this is the second squash I have had to reconcile tonight: a squash that also absorbs a Not verified: the device measurements (one ticket per row of the table, as the docstring says). What I verified is the decision logic and that the squash preserved it. |
|
VERDICT: READY at 16 commits since my verdict, merge traffic from |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at bc46feb: READY
The mechanism is config-driven with sensible bounds, which is what makes a new kernel family safe to try: the pack names its sealed narrow ELF (q4nx_t2s_sealed_narrow_elf / q4nx_pb16i_sealed_narrow_elf), q4nx_sealed_narrow_rows defaults to 4 and is bounded to 1..15, and both NPU_NARROW_SEALED_ELF=0 and NPU_NARROW_ROWS give an operator a way out without a rebuild. A default that can be switched off in one environment variable is the difference between an experiment and a commitment.
It also only takes a launch of 1..rows rows — a decode step or a short verify pass — so a full prefill is untouched. That is the right scope: the waste being fixed is "a kernel built for 16 rows carrying one".
One thing I would add before the number is quoted anywhere downstream: the headline (3.26-3.33 → 3.69-4…) should name the route and the prompt set it was measured on, the way #1953 records its drafter row (Qwen2.5-7B-Instruct_rai_1.9.0_npu, 2.96-3.4 tokens/request). A tok/s delta without its route is the exact number that gets re-quoted against a different workload six hours later — this fleet has spent the day unwinding several of those.
I read the diff's shape and CI; I did not run the 27B route, so the gain itself is the author's measurement, not mine.
…fill.sealed_elf, decode.narrow_elf) (Refs #1990, #2073, #799) Plan::load reads prefill.sealed_elf / prefill.sealed_rows (17..128) and decode.narrow_elf / decode.narrow_rows (1..15), all optional, and refuses the inconsistent forms (no sealed decode set, a path instead of a bare name, rows missing or out of range, rows without a file). A loaded plan wins over config.json; NPU_WIDE_SEALED_ELF and NPU_NARROW_SEALED_ELF still override both. tools/npu_plan.py copies the config keys into the plan and writes nothing new for a pack without them. Re-applied on main after #2042 and #2074 merged; the change is the one reviewed at 4315c23 / 2c3ad10. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…fill.sealed_elf, decode.narrow_elf) (Refs #1990, #2073, #799) (#2080) Plan::load reads prefill.sealed_elf / prefill.sealed_rows (17..128) and decode.narrow_elf / decode.narrow_rows (1..15), all optional, and refuses the inconsistent forms (no sealed decode set, a path instead of a bare name, rows missing or out of range, rows without a file). A loaded plan wins over config.json; NPU_WIDE_SEALED_ELF and NPU_NARROW_SEALED_ELF still override both. tools/npu_plan.py copies the config keys into the plan and writes nothing new for a pack without them. Re-applied on main after #2042 and #2074 merged; the change is the one reviewed at 4315c23 / 2c3ad10. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
…get::open_decode_family (A2b, refs #803 #799) (#2166) Rebase of #1984 onto current main, where open_decode_family_local() has since grown the wide (#2042) and narrow (#2074) sealed families. The per-role ELF resolution and context creation moves to XdnaTarget::open_decode_family(); the design naming, the summary lines and the wide/narrow loader stay in the forward, unchanged. Includes #2038: the plan's decode directories are resolved (Plan::resolved_decode_dirs) before the target is configured, so both the decode family and the lm-head lookup see them. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Fixes #2073.
Draft because it is stacked on #2042: this branch contains #2042's two commits plus one of its own (
59f8f4b). Review that last commit; I rebase onto main and mark it ready once #2042 merges.What
A decode step on the served 27B route is 416 launches and 70 % device wait, and every launch runs a kernel built for 16 rows to carry one row. This adds a narrow family, the mirror of #2042's wide one:
q4nx_t2s_sealed_narrow_elf/q4nx_pb16i_sealed_narrow_elfandq4nx_sealed_narrow_rows(default 4, allowed 1..15) inconfig.json.NPU_NARROW_SEALED_ELF=<file>overrides the file,=0switches it off,NPU_NARROW_ROWSoverrides the rows.q4nx_forward.cpp.Nothing changes for an existing pack: no pack names a narrow ELF. The ELF itself is private (npu-kernels#62).
Measured (this box,
bonsai2-27b-t2s2, one binary, arms 16-row / 4-row / 16-row / 4-row, three prompts x 49 tokens)First ticket:
The second pair of that ticket ran while another lane loaded the box (both arms slowed), so I repeated it. Second ticket:
One decode step from the
[bs] M= 1lines (medians of 6 steps per arm, ms):Correctness:
Unit proof (no device)
tests/npu_pack_test.cpp:narrow family: 10 arms (complete or not, which launch takes it), rc 0. Two mutants ofbatch_width.hthrough the real target:tools/copyright.py --checkandtests_referenced_check.pypass.Not covered
[bs] M= 1medians are 6 steps per arm (the serve prints that line for a request's first steps only); the tok/s lines cover all 48 decode steps of each request.plan.jsonhas no field for the narrow set yet (the wide one gets its field in a follow-up to npu: a sealed set can carry a wide prefill family, named by the pack or the environment: 28.3-30.8 -> 33.9-35.9 tok/s to first token with a 32-row set (Refs #1990) #2042; the narrow one should follow the same shape).🤖 Generated with Claude Code