Skip to content

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

Merged
bong-water-water-bong merged 4 commits into
mainfrom
cl-npuperf/1990-wide-rows
Nov 11, 2026

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #1990 (the wider-M half). Nothing changes for an existing pack: a wide set is used only when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set.

What

A sealed pb16i/t2s set is built for 16 rows per launch, so a prefill chunk is 16 rows. NPU_WIDE_SEALED_ELF=<file> names a second sealed ELF with the same instances built for NPU_WIDE_ROWS rows (default 32, allowed 17..128).

  • When it loads for every role that has a 16-row kernel, the chunk width becomes that row count and a launch of more than 16 rows takes the wide kernel.
  • Decode steps and chunks of up to 16 rows stay on the 16-row family.
  • All roles or none: a set missing a role is not adopted and the log says why.
  • A launch carrying more rows than its kernel holds is refused by name instead of running past the buffers.
  • The three rules (wide_family_complete, chunk_rows, wide_takes) are in npu/forward/include/batch_width.h.

The wide ELF itself is private (npu-kernels#60, merged) and no pack ships one yet.

Measured (this box, bonsai2-27b-t2s2, one npu-hold ticket, one binary, arms default / wide / default / wide)

D1 long1: wall=18.933s prompt_tokens=536 prefill=28.31 tok/s
D1 long2: wall=17.693s prompt_tokens=510 prefill=28.82 tok/s
D1 long3: wall=17.132s prompt_tokens=511 prefill=29.83 tok/s
W1 long1: wall=15.050s prompt_tokens=536 prefill=35.61 tok/s
W1 long2: wall=14.244s prompt_tokens=510 prefill=35.80 tok/s
W1 long3: wall=14.219s prompt_tokens=511 prefill=35.94 tok/s
D2 long1: wall=17.550s prompt_tokens=536 prefill=30.54 tok/s
D2 long2: wall=16.668s prompt_tokens=510 prefill=30.60 tok/s
D2 long3: wall=16.602s prompt_tokens=511 prefill=30.78 tok/s
W2 long1: wall=15.331s prompt_tokens=536 prefill=34.96 tok/s
W2 long2: wall=14.392s prompt_tokens=510 prefill=35.44 tok/s
W2 long3: wall=14.486s prompt_tokens=511 prefill=35.27 tok/s
amdxdna ERROR count: 2561 -> 2561

Block medians from the [bs] lines (ms):

arm rows blocks wall pack ql wait host
D1 16 103 534.2 55.3 48.8 212.0 196.3
W1 32 50 871.0 109.8 64.7 311.3 355.0
D2 16 103 509.3 51.3 44.4 210.5 181.1
W2 32 50 881.5 110.7 67.1 312.1 360.9

That is 30.0-31.4 rows/s on the default path and 36.3-36.7 rows/s with the wide set. An earlier ticket on the pre-rebase build gave the same direction (25.1-27.6 against 29.7-32.5 tok/s).

Correctness, same ticket:

[forward] wide family: 8 role(s) at M=32 from /dev/shm/cl-npuperf-SEALED-T2S-m32.elf (chunks of 17..32 rows)
text8 sha256=d819d7c8ae824fb6 in D1, W1, D2, W2, WC, WN   (' river crossing at mile 49 was')
WC (NPU_BATCH_CHECK=1 on the wide path): 9 [batchcheck] lines, 9 MATCH (M=32 x4, M=4 x2, M=1 x3)
WN (NPU_WIDE_SEALED_ELF=/dev/shm/no-such.elf):
[forward] wide family NOT adopted (no such file: /dev/shm/no-such.elf); chunks stay at 16 rows

Unit proof (no device)

tests/npu_pack_test.cpp gains 14 arms: wide family: 14 arms (complete or not, chunk width, which launch takes it), rc 0. Three mutants of batch_width.h, each built through the real target:

mutant 1 (wide_takes: rows >= narrow)        rc=1  FAIL wide takes: 1 and 16 rows stay on the decode family
mutant 2 (complete: a missing role allowed)  rc=1  FAIL wide family: one decode role without a wide kernel -> not complete
mutant 3 (chunk_rows: wide > 0 wins)         rc=1  FAIL chunk rows: a 'wide' value below the decode width never narrows the chunk

tools/copyright.py --check, tests_referenced_check.py and ci_targets_check.py --check pass.

Second commit (f1aa6e4): the pack can name the wide set

  • config.json: q4nx_t2s_sealed_wide_elf / q4nx_pb16i_sealed_wide_elf (a file in the kernels directory) and q4nx_sealed_wide_rows (default 32).
  • NPU_WIDE_SEALED_ELF=<file> overrides the pack's file; NPU_WIDE_SEALED_ELF=0 switches a pack's wide family off.
  • A named file that is missing or does not load is logged and chunks stay at 16 rows; the serve does not stop.
  • The choice is wide_elf_path() in batch_width.h, 7 more arms (wide family: 21 arms, rc 0).

Measured through the pack key, on a scratch pack that links the served pack's files and adds the key and the ELF (the served pack was not touched). One ticket, one binary, arms off (NPU_WIDE_SEALED_ELF=0) / on / off / on:

D1 long1: wall=18.688s prompt_tokens=536 prefill=28.68 tok/s
D1 long2: wall=17.730s prompt_tokens=510 prefill=28.76 tok/s
D1 long3: wall=17.707s prompt_tokens=511 prefill=28.86 tok/s
K1 long1: wall=15.690s prompt_tokens=536 prefill=34.16 tok/s
K1 long2: wall=15.030s prompt_tokens=510 prefill=33.93 tok/s
K1 long3: wall=14.952s prompt_tokens=511 prefill=34.18 tok/s
D2 long1: wall=18.586s prompt_tokens=536 prefill=28.84 tok/s
D2 long2: wall=17.587s prompt_tokens=510 prefill=29.00 tok/s
D2 long3: wall=17.826s prompt_tokens=511 prefill=28.67 tok/s
K2 long1: wall=15.673s prompt_tokens=536 prefill=34.20 tok/s
K2 long2: wall=14.880s prompt_tokens=510 prefill=34.27 tok/s
K2 long3: wall=14.866s prompt_tokens=511 prefill=34.37 tok/s
amdxdna ERROR count: 2561 -> 2561
[forward] wide family: 8 role(s) at M=32 from .../pack/kernels-m16/SEALED-T2S-m32.elf (chunks of 17..32 rows)
block medians: 16 rows 541.2, 541.4 ms; 32 rows 914.0, 908.3 ms
NPU_BATCH_CHECK=1 through the pack key: 9 [batchcheck] lines, 9 MATCH
text8 sha256=d819d7c8ae824fb6 with the family off and on (a separate 31 s ticket)

Not covered

🤖 Generated with Claude Code

…F, off by default): 28.3-30.8 -> 35.0-35.9 tok/s to first token with a 32-row set (Refs #1990)

A sealed pb16i/t2s set is built for 16 rows per launch, so a prefill chunk is 16 rows. NPU_WIDE_SEALED_ELF names a
second sealed ELF with the same instances built for NPU_WIDE_ROWS rows (default 32). When it loads for every role
that has a 16-row kernel, the chunk width becomes that row count and launches of more than 16 rows take the wide
kernel; decode and chunks of up to 16 rows stay on the 16-row family. All roles or none, and a launch carrying more
rows than its kernel holds is refused by name.

The three rules (wide_family_complete, chunk_rows, wide_takes) live in batch_width.h and are pinned without a device
by 14 new arms in tests/npu_pack_test.cpp.

Experimental: no pack ships a wide ELF and the plan has no key for one, so it is an environment switch.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at 6080e47

…lf); the environment overrides or switches it off (Refs #1990)

config.json keys q4nx_t2s_sealed_wide_elf / q4nx_pb16i_sealed_wide_elf (a file in the kernels dir) and
q4nx_sealed_wide_rows (default 32). NPU_WIDE_SEALED_ELF=<file> overrides, =0 switches the pack's family off.
A named file that is missing or does not load is logged and the pass stays at 16 rows.

wide_elf_path() holds the choice and has 7 arms in tests/npu_pack_test.cpp. Measured through the pack key on
bonsai2-27b-t2s2: first token 28.7-29.0 tok/s off, 33.9-34.4 tok/s on, same text, oracle MATCH.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong bong-water-water-bong changed the title npu: a sealed set can carry a wide prefill family (off by default): 28.3-30.8 -> 35.0-35.9 tok/s to first token with a 32-row set (Refs #1990) 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) Nov 10, 2026
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at f1aa6e4

Supersedes my note at 6080e47 (no verdict was posted there). The new commit lets a pack name the wide set in config.json; the body has its measurement and what is not covered.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f1aa6e4 — A sealed set can carry a wide prefill family named by the pack itself (+44 batch_width.h, docs +29): naming the family in the pack rather than inferring it keeps the loader honest about which kernel set it is picking, and it is the mechanism #2047 and #1903 need.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at f1aa6e4: READY

Checked at f1aa6e4. A three-way merge onto main is clean, and so is a merge with my #2047, which edits the same npu_gemm_rows.

  • npu_pack_test: ok, including "wide family: 21 arms (complete or not, chunk width, which launch takes it, which file)".
  • The dangerous-direction arm can fail. With wide_takes() stripped of its rows <= wide bound, the test fails "wide takes: 33 rows are not routed to a 32-row kernel". Restored, it's ok.
  • npu/target/xdna/xdna_target.cpp and npu/forward/src/q4nx_forward.cpp compile at this head (NPU-private: single-file compiles with an existing NPU build's commands, pointed at this tree).

Read for the two ways this could corrupt memory, and neither is open:

  • A chunk wider than some role's buffers. adopt_wide() takes the family only when wide_family_complete() holds: every role with a 16-row context has a wide one of exactly rows, and no wide kernel exists for a role the set doesn't run. Otherwise drop_wide() clears every wide context and max_rows() stays at the narrow minimum. chunk_rows() only widens when wide_rows_ > narrow.
  • A launch past its kernel. npu_gemm_rows routes to ctxw_[role] only when wide_takes(M, 16, wide_rows_), and the new M > kctx.MD check refuses by name before any launch.

Decode and chunks of 16 rows or fewer never take the wide family, so an existing pack's path is unchanged unless the pack or NPU_WIDE_SEALED_ELF names a set. The measurement is in the form #2005/#2008 used: two default/wide pairs in one ticket, both moving the same way, plus text sha256 equal across all arms and 9/9 [batchcheck] MATCH on the wide path. That's what a geometry change needs. Not run by me: the device ticket. No pack here ships a wide ELF, and the private one is in your /dev/shm.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at f1aa6e4: READY — a wider prefill family behind two explicit opt-ins, with the default path unchanged

The scoping statement is the important part and it is stated first: "Nothing changes for an existing pack: a wide set is used only when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set." A sealed pb16i/t2s set is built for 16 rows per launch, so prefill chunks at 16 rows — and a wide set is the credible fix for the prefill slope, but it changes which ELF runs, so gating it on an explicit name in the pack or an explicit env var is the only acceptable default. No existing measurement moves.

Two asks, in order, both about the failure modes of an env-var-gated ELF:

  1. Validate the wide set before use — a named file that is not the right ELF (wrong t2s set, wrong batch width, truncated) must refuse by name at load, not fail inside a launch. 1bit comfy: run the checked binary; fetch the submodule when missing #66 in gpu-kernels established the pattern for kernels ("abort at registration, naming the kernel id and code object"), and the same applies to a sealed set: the ELF should be opened and its design asserted against the pack's geometry before any launch.
  2. NPU_WIDE_SEALED_ELF must be visible in the run's log and in any measurement's provenance. This is the serve: refuse --npu-opt on a model-generic container, where the per-op forward cannot use it #1991 lesson exactly — --npu-opt was accepted then bypassed, and W6 produced identical numbers that read as "no gain" rather than "wrong route". An env var that selects a different ELF is even easier to lose track of: one line at startup (wide sealed set: <file> rows=<N>) makes every row attributable.

Three smaller ones:
3. Say what happens when the pack's config.json names a wide set that the loaded kernels cannot run — refuse, or fall back to 16 with a warning? Refusing is the correct answer (a silent fallback makes a 3× difference into a rounding error nobody notices), and the preference between pack-declared and env-var when both are present should be stated.
4. batch_width_min() interaction (#1903): a wide set widens the minimum-fold, so the sealed pack's reported width must become the wide set's width — worth one assertion, since a stale 16 would route a 128-row chunk through the 16-row path and quietly cost what this PR gains.
5. The M=16 decode family moved to the target in #1984 — worth confirming this lands after it, or the merge order puts a wide-prefill selector into a forward that is about to hand the sealed-set ownership to the target.

And the payoff framing is right: refs #1990's wider-M half, nothing claimed for existing packs — so this can land without waiting on a measurement, and its row arrives when cl-npuperf's kernels and a wide set exist together.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at f1aa6e4 — the admissibility test is the interesting part, and it is five arms that each forbid a different way a wide family can be wrong.

The gate, verbatim

inline bool wide_family_complete(const int* narrow_widths, const int* wide_widths, int n, int wide) {
    for (int i = 0; i < n; ++i) {
        if (narrow_widths[i] <= 0) {
            if (wide_widths[i] > 0) return false;        // a wide kernel for a role the set does not run
            continue;
        }
        if (wide_widths[i] != wide || wide <= narrow_widths[i]) return false;
        ++roles;
    }

Three separate conditions, and each is a real failure: every decode role must have one (a hole means a chunk of 32 rows would fall back mid-launch), it must be the same row count for every role (mixed widths would need two different launch shapes at once), and "wide" must be strictly wider than the narrow family (w16 labelled wide is not wider).

And the arms are one per way it can fail — which is what makes this a decision table rather than a happy path:

check( wide_family_complete(n16, w32,   4, 32), "every decode role has a 32-row kernel -> complete")
check(!wide_family_complete(n16, hole,  4, 32), "one decode role without a wide kernel -> not complete")
check(!wide_family_complete(n16, mixed, 4, 32), "a role built for another row count -> not complete")
check(!wide_family_complete(n16, extra, 4, 32), "a wide kernel for a role with no decode op -> not complete")
check(!wide_family_complete(n16, w16,   4, 16), "'wide' must be wider than the decode family")

Four negatives to one positive. extra is my favourite: a wide kernel for a role the narrow set does not run is evidence the two sets are not the same family, and a test that only checked "no holes" would accept it.

The opt-in is opt-in

"Nothing changes for an existing pack: a wide set is used only when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set."

A new fast path that activates only on an explicit signal is the right shape for this box: #1990's wider-M half cannot silently change numbers for every existing pack, and adopt_wide(rows) failing leaves wide_rows_ = 0 — i.e. the fallback is the previous behaviour, the same property I liked in #2026's padded_head_rows returning NV.

One ask: NPU_WIDE_SEALED_ELF names a second sealed ELF "with the same instance" — if a user points it at an ELF for a different design set, the geometry check above catches width mismatches but not a same-width ELF of the wrong model. One printed line at adopt time naming the wide ELF's path and row count would make a wrong pointer diagnosable from the log, which is the #2041 lesson.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f1aa6e4d — and the three helpers that carry the risk are the three that decide it correctly.

wide_family_complete() is the guard I would have written before allowing this at all: "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 engine#1372's corruption direction, stated as the reason rather than as a comment. Refusing a wide kernel for a role the set does not run is the same invariant from the other side, and wide <= narrow_widths[i] → false keeps the two families ordered.

wide_takes(rows, narrow, wide) gets the asymmetry right: wide only when the chunk does not fit narrow and does fit wide, so a decode step can never take a 32-row launch (which costs more whatever it carries), and a chunk past wide is the caller's refusal, not a silent clamp.

The measurement is the shape I trust: two tickets, arms off/on/off/on, and the arithmetic cross-checks — two 16-row blocks (534 + 509 ms) against one 32-row block (871 ms) is 30–31 rows/s against 35–37, which is the claim, and the 8-token greedy text is identical off and on. NPU_BATCH_CHECK=1 reporting MATCH on every chunk it checks (32-, 4- and 1-row) is the row-independence evidence, and it is the same oracle I asked for on #1959/#1963.

One thing I want written down, because it is the invariant a reader cannot check from the helpers: adopting the wide family must size every role's activation BO for wide rows, not for the narrow width it was sized for before. wide_family_complete() proves the kernels exist at that width; it cannot prove the buffers were resized. One sentence in docs/npu.md (which this PR extends by 29 lines) saying "adopting the wide set re-sizes the BOs to wide; it is refused if any role lacks the kernel" closes the gap between the guard and the thing the guard is for.

Two smaller notes:

  1. Say which narrow the call site passes to wide_takes(). With my batch_width_min() the narrow answer is the minimum over loaded roles (#1903), and wide_family_complete() requires wide > every narrow width — so passing the minimum is right and consistent. But a future caller passing one role's width would still pass the guard while violating its intent; naming "the minimum" in the comment makes the two helpers obviously a pair.
  2. "Only 32 rows has been measured" is exactly the right disclosure, and it is the reason I am not asking for the 17..128 range here: NPU_WIDE_ROWS accepts a range the measurement covers one point of. If a 64-row set is ever built, the two-block-against-one arithmetic is what should be re-checked first, because the crossover moves with the block cost.

Nothing else: this is the wider-M half of #1990 done the way I would want it — a family that is optional, complete-or-nothing, measured off/on/off/on, and text-identical.

…ows the decode family into open_decode_family_local()

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at ee11811

The branch had gone conflicting: main moved the decode-family block out of init_impl into open_decode_family_local() (#803 A2b). Main is merged in and the wide-family block moved with it, unchanged apart from indentation; sealed_dir_ is set at the same line as before.

Re-checked on the merged build, one npu-hold ticket:

D1 (wide off): 27.55 / 29.80 / 29.53 tok/s to first token
W1 (wide on):  24.50 / 35.50 / 35.28 tok/s   (the 24.50 is one loaded request; the box had other work)
[forward] wide family: 8 role(s) at M=32 ... (chunks of 17..32 rows)
NPU_BATCH_CHECK=1: 9 [batchcheck] lines, 9 MATCH;  text8 sha256=d819d7c8ae824fb6 in all three arms
amdxdna ERROR count: 2561 -> 2561

npu_pack_test rc 0, tools/copyright.py --check rc 0. The numbers in the body are from the earlier head and are not re-claimed here.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at ee11811d4 (carried; 0 own commits since the verdicted commit — delta is main merged in).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ee11811d

The rules are pure functions in a header, so I compiled them and probed the boundaries rather than reading the table.

chunk_rows(16,17)=17  (16,32)=32  (16,128)=128

wide= 17:  rows=16 ->0  rows=17 ->1  rows=32 ->0  rows=33 ->0  rows=128->0  rows=129->0
wide= 32:  rows=16 ->0  rows=17 ->1  rows=32 ->1  rows=33 ->0  rows=128->0  rows=129->0
wide=128:  rows=16 ->0  rows=17 ->1  rows=32 ->1  rows=33 ->1  rows=128->1  rows=129->0

Every window is exactly the declared one: 16 rows never takes the wide kernel (the narrow one is correct and cheaper), the top of each range is inclusive, and 129 is never taken even with a 128-row set — so a chunk can never ask a kernel for more rows than it holds, which is the failure that would matter. chunk_rows returning the wide count is what makes the block medians in the table meaningful (two 16-row blocks at 30-31 rows/s against one 32-row block at 35-37).

The naming rules are ordered the way the docs say, checked rather than assumed: NPU_WIDE_SEALED_ELF=0 disables (empty path), any other value overrides the pack, and a relative pack name is joined against the kernels directory while an absolute one is left alone.

One trap worth a sentence in the docs, not a code change: NPU_WIDE_SEALED_ELF=1 means a file named 1, not "enable" — the probe returns [1]. It fails visibly ([forward] wide family NOT adopted (...) and chunks stay at 16 rows), so it is a misunderstanding rather than a silent mismeasurement, but "set it to 0 to disable, to a path to enable" would save the next person the detour.

What makes the numbers credible: the greedy text is identical with the family off and on, and NPU_BATCH_CHECK=1 reports MATCH on every chunk of the wide path — the same forced/off pair I asked for elsewhere today. A 32-row kernel walking 32 baked rows for a 17-row chunk is a real waste (the reason #1963 splits wide chunks), so the win here is that the waste ceiling drops from 128 to 32 while launches halve; the table is consistent with that rather than with something else happening.

Composes with #1963 as expected: with narrow=16, wide=32 the split rule sees a ratio of 2, which never splits (launches × narrow < 0.9 × wide is unreachable for ratio 2), so a wide set and the split decision do not fight — the wide launch keeps the chunk.

Not verified: the tok/s figures and the NPU_BATCH_CHECK run (no NPU ticket here); no pack ships a wide set yet, so the default path is unchanged by construction.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at ee11811 — Re-verdict at the new head: a sealed set naming its wide prefill family from the pack — the mechanism #2047 and #2042's follow-ups need, kept in the pack rather than inferred.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at ee11811

The opt-in design is exactly right for a change that swaps sealed kernel sets: "Nothing changes for an existing pack: a wide set is used only when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set." A wider-M prefill set is a different arithmetic schedule; enabling it by default for unmeasured packs would be a silent correctness risk, and both opt-in paths are explicit and inspectable.

And "named by the pack" is the better of the two, because it makes the choice a property of the artifact rather than of the person running it — the same principle as the container's own dims taking precedence over a config beside it (F1's slice 2, #2070). An env var is right for experiments; a config.json key is right for anything a measurement certifies.

Two asks:

  1. state what happens when both are present and disagree — pack names set A, env names set B. Silently preferring one is the kind of decision that gets discovered by a confusing benchmark;
  2. say what a wrong wide set does (intended for a different hidden/IM): #1990's own reason for existing is that a 17-row chunk costs 6.51× a 16-row one because rows > 16 selects M=128 — so a wide set applied to a pack whose geometry it was not built for should refuse by name, not compute.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at ee11811: READY

The new head merges main, and the wide-family block moved with main's reorganisation: it now follows the decode-family load ("the wide-family block follows the decode family"). Its own diff changed only in indentation and position. I read it in its new place:

  • it runs only when a path resolves (wide_elf_path), the set is sealed pb16i/t2s and made > 0 decode contexts exist;
  • NPU_WIDE_ROWS outside 17..128 and a missing file are refused by name before any context is built;
  • each role with a 16-row context gets a wide one copying KD/ND/blocked_b/pb16i/t2s/planes, with MD = rows;
  • any failure stops the loop, and adopt_wide() still gates adoption on wide_family_complete();
  • otherwise drop_wide() and the log line say why the pass stays at 16 rows.

Same behaviour as at f1aa6e4, where I passed it.

At ee11811:

  • npu/target/xdna/xdna_target.cpp and npu/forward/src/q4nx_forward.cpp compile (NPU-private: single-file compiles with an existing NPU build's commands, pointed at this tree). This was the real risk of the move, because the block now uses dev in a different function.
  • npu_pack_test: ok, wide family 21 arms.
  • A three-way merge onto current main is clean.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ee11811 — re-issued for the new head. The delta since my f1aa6e4 verdict is main merging in (the only non-merge commits are #2044, #2051, #2054), so wide_family_complete() and its five arms are unchanged, and my verdict carries.

My one ask is still open, and I want to be precise about why it is small but not cosmetic: adopt_wide() is at xdna_target.cpp:356 and the file's only fprintf is the host-GEMM M/K/N line at :126. So a run that adopts a wide family prints nothing about it — and the failure it would hide is a pointer at the wrong ELF, which the width checks cannot catch (a same-width ELF of the wrong model is admissible by arithmetic). One line at adopt time — [forward] wide prefill family: <path> rows=<N> — converts that from "the numbers look odd" into a log line, which is exactly the #2041 lesson (a banner that cost four runs to learn).

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1990 - Partially compliant

Compliant requirements:

  • Implement a wider-M launch geometry for the served route, measured in tok/s
  • Measure prefill tok/s on the served route, before and after under npu-hold
  • A sealed pb16i/t2s set is built for 16 rows per launch, so a prefill chunk is 16 rows
  • A second sealed ELF with the same instances built for NPU_WIDE_ROWS rows (default 32, allowed 17..128) is used when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set
  • When it loads for every role that has a 16-row kernel, the chunk width becomes that row count and a launch of more than 16 rows takes the wide kernel
  • Decode steps and chunks of up to 16 rows stay on the 16-row family
  • All roles or none: a set missing a role is not adopted and the log says why
  • A launch carrying more rows than its kernel holds is refused by name instead of running past the buffers
  • The three rules (wide_family_complete, chunk_rows, wide_takes) are in npu/forward/include/batch_width.h
  • The wide ELF itself is private (npu-kernels#60, merged) and no pack ships one yet

Non-compliant requirements:

  • The PR does not include a pack that ships a wide ELF yet, as noted in the description

Requires further human verification:

  • The measured performance improvements (33.9-35.9 tok/s to first token with a 32-row set) need to be verified on the actual hardware
  • The claim that "no pack ships one yet" needs confirmation that no existing pack has been updated to include the wide ELF configuration
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Wide family adoption logic

The logic for adopting a wide family in open_decode_family_local may not correctly handle cases where a pack's wide ELF is missing or fails to load. While the code logs why adoption fails, it does not prevent the system from falling back to the 16-row family in a way that could lead to inconsistent behavior if not all roles are properly handled.

const std::string wide_path =
    onebit::npu::wide_elf_path(getenv("NPU_WIDE_SEALED_ELF"), wide_sealed_, sealed_dir_);
if (const char* wide_elf = wide_path.c_str();
    !wide_path.empty() && pb16i_ && !pb16i_sealed_.empty() && made > 0) {
    const char* wr = getenv("NPU_WIDE_ROWS");
    const int rows = wr && *wr ? atoi(wr) : wide_sealed_rows_;
    int wide = 0;
    std::string why;
    if (rows <= Q4NX_DESIGN_M_DECODE || rows > Q4NX_DESIGN_M_PREFILL) why = "NPU_WIDE_ROWS must be 17..128";
    else if (!file_exists(wide_elf)) why = std::string("no such file: ") + wide_elf;
    for (int i = 0; why.empty() && i < ndesigns_; i++) {
        I8Ctx* c16 = xd_->ctx16(i);
        if (!c16) continue;
        const char* inst = pb16i_instance(designs16_[i].role);
        I8Ctx* cw = inst ? xd_->make_ctx_wide(i) : nullptr;
        if (!cw) { why = std::string("no wide context for role ") + role_tag(designs16_[i].role); break; }
        cw->MD = rows;
        cw->KD = c16->KD;
        cw->ND = c16->ND;
        cw->blocked_b = c16->blocked_b;
        cw->pb16i = c16->pb16i;
        cw->t2s = c16->t2s;
        cw->pb16i_planes = c16->pb16i_planes;
        if (!cw->init_elf(*dev, wide_elf, rows, c16->KD, c16->ND, 1, inst)) {
            why = std::string("init_elf failed for instance ") + inst;
            break;
        }
        wide++;
    }
    if (why.empty() && wide > 0 && !xd_->adopt_wide(rows)) why = "a role with a decode context has no wide one";
    if (why.empty() && wide > 0) {
        fprintf(stderr, "  [forward] wide family: %d role(s) at M=%d from %s (chunks of 17..%d rows)\n",
                wide, rows, wide_elf, rows);
    } else {
        xd_->drop_wide();
        fprintf(stderr, "  [forward] wide family NOT adopted (%s); chunks stay at %d rows\n",
                why.empty() ? "no role has a decode context" : why.c_str(), Q4NX_DESIGN_M_DECODE);
    }
}
Chunk width calculation

The max_rows() function in XdnaTarget now uses onebit::npu::chunk_rows to determine the chunk width, which depends on the wide family being adopted. If the wide family adoption fails or is not properly initialized, this could lead to incorrect chunk width calculations and potentially cause launches to exceed buffer limits.

if (any16) {
    // engine#1990: with a wide family adopted for every role, a chunk may be as wide as that family.
    return onebit::npu::chunk_rows(onebit::npu::batch_width_min(widths16, n, 1), wide_rows_);
}
Launch routing logic

The rows_for function now checks for wide family adoption before deciding which context to use for a launch. If the wide family is not properly adopted or initialized, this could cause launches to be routed incorrectly, potentially leading to buffer overruns or incorrect kernel selection.

if (pb16i_ && ctxw_[role] && onebit::npu::wide_takes(want, Q4NX_DESIGN_M_DECODE, wide_rows_))
    return ctxw_[role]->MD;   // engine#1990

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 0c329d0: READY

Another merge of main (it brings in #2043 and others). The PR's own diff is identical to ee11811, which I passed, and a three-way merge onto current main is clean. At 0c329d0 (worktree at that SHA), npu/target/xdna/xdna_target.cpp and npu/forward/src/q4nx_forward.cpp compile (NPU-private: single-file compiles with an existing NPU build's commands, pointed at this tree), and npu_pack_test is ok with its 21 wide-family arms.

@bong-water-water-bong
bong-water-water-bong merged commit b2f17e2 into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-npuperf/1990-wide-rows branch November 11, 2026 01:10
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 0c329d0

The measurement is the part that makes this a change rather than a knob: two A/B pairs (default / wide / default / wide) on one binary under one
npu-hold ticket, with the same packs and prompts, so the 19-24 % prefill gain cannot be drift — 28.31-29.83 tok/s on the 16-row path against
35.61-35.94 with the wide set, and the second pair reproduces it (30.54-30.78 vs 34.96-35.27) after interleaving back to default. Interleaving is what a
bench needs to be believable when the box is shared, and the amdxdna ERROR count: 2561 -> 2561 line is the right guard: the wide path adds no device
errors.

The block table is the honest companion to the row rates: the wide arm halves the block count (103 → 50, as expected at 32 rows) while per-block wall
rises (534 → 871 ms), so the win is not "the kernel got faster" — it is fewer launches and less fixed cost. Anyone reading only the tok/s row would
mis-infer the mechanism, and the pack/ql/wait/host columns show where the time actually sits.

Three safety properties are the ones I would have asked for, and they are in the arm set rather than implied:

  • all roles or none — a set missing a role is not adopted and the log says why, which is the failure mode of a partial wide set (some projections
    wide, some 16-row, silently mixing shapes);
  • more rows than the kernel holds is refused by name rather than running past the buffers;
  • decode and chunks of ≤16 rows stay on the 16-row family, so the change cannot regress the path that works today.

And the scoping is right: nothing changes for an existing pack (a wide set is used only when config.json names one or NPU_WIDE_SEALED_ELF is set), the
ELF is private (npu-kernels#60, merged), and no pack ships one yet — so this is the engine half waiting on a container, exactly as #1990 describes.

Head under review: 0c329d0

bong-water-water-bong pushed a commit that referenced this pull request Nov 11, 2026
…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 top of the narrow family after #2042 merged; the change is the one reviewed at 4315c23.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 0c329d0 — built and ran: npu_pack_test ok, with wide family: 21 arms (complete or not, chunk width, which launch takes it, which file), and the arms are the right ones:

every decode role has a 32-row kernel -> complete
one decode role without a wide kernel -> not complete
a role built for another row count     -> not complete
a wide kernel for a role with no decode one -> refused
'wide' must be wider than the decode family
no role at all is not a family
chunk rows: no wide family keeps the decode width / an adopted wide family widens the chunk

"Complete" being a per-role all-or-nothing test is the decision that makes this safe, and the four negative arms are what pin it. A wide family adopted when most roles have a 32-row kernel would produce a chunk whose width some role silently cannot run — the failure would be a wrong answer or a refusal deep in a launch, not a load-time error. Requiring completeness before widening the chunk is the only version of this feature that cannot produce that, and 'wide' must be wider than the decode family closes the degenerate case where the two files are the same.

The default-off posture is stated first and is correct: "Nothing changes for an existing pack: a wide set is used only when the pack's config.json names one or NPU_WIDE_SEALED_ELF is set." A change that can alter the chunk width of every existing pack by loading a file is exactly the kind that should require opting in.

One ask: the row-count range is documented as allowed 17..128, and the arms cover 16 (refused), 32 (adopted) and a mismatched role. Worth an arm at 17 and at 128 — the two ends of the documented range — because a validator written as > 16 && < 129 and one written as >= 17 && <= 128 behave identically in the middle and differently at the boundary, and the boundary is what the docs promise.

Second ask, smaller: since a wide ELF changes the chunk width (and therefore what a prefill measurement means), one line in docs/npu.md saying which prefill rows were measured with it — so a speed row quoted later cannot be read as belonging to the 16-row path. The PR already documents the mechanism; the measurement caveat is the part a reader will need in three weeks.

Scope of my check: built at this head, ran npu_pack_test (all arms green, including the 21 wide-family ones). No device, so "a launch of more than 16 rows takes the wide kernel" on real kernels is the PR's claim; the decision logic — which is where this could go wrong — is what the arms pin.

bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
…-4.21 tok/s decode with a 4-row set (Fixes #2073) (#2074)

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: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong pushed a commit that referenced this pull request Nov 11, 2026
… the merge exposed on a pack with a wide family

Conflicts: the batch_width.h include and the wide-family test arms from #2042 are kept beside this branch's.

Fixes, all found on bonsai2-27b-t2s2 with the 32-row sealed family of #2042:

1. A walked chunk stepped through its rows by a_k, the design's real width, which is 0 on a design that does not
   pad: every block after the first re-read the first block's rows. NPU_BATCH_CHECK=1 npu-run on 32 ids with the split
   forced reported identical=16 differing=16 first row=16; the serve generated different text. The stride is now
   row_stride(a_k, K) (batch_split.h), in the split walk and in the older block walk. After: identical=32 forced,
   default and off.
2. One start-up launch was taken for the wide launch's cost (4.17 ms against 0.51 ms). The wide cost counts only
   after three timed launches (kSplitWideSamples, split_wide_cost()).
3. With a wide sealed family adopted the measured rule stands down: a narrow launch is mostly timed carrying one
   row, so its cost understates a full one (0.50 against 1.23 ms still split). NPU_PREFILL_SPLIT=1 still forces it.

tests/npu_pack_test.cpp: batch split 11 -> 19 arms. docs/npu.md: the three points under the #1959 section.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong pushed a commit that referenced this pull request Nov 11, 2026
…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>
bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
…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>
bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant