Repository navigation
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
Conversation
…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>
|
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
left a comment
There was a problem hiding this comment.
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 itsrows <= widebound, the test fails "wide takes: 33 rows are not routed to a 32-row kernel". Restored, it's ok. npu/target/xdna/xdna_target.cppandnpu/forward/src/q4nx_forward.cppcompile 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 whenwide_family_complete()holds: every role with a 16-row context has a wide one of exactlyrows, and no wide kernel exists for a role the set doesn't run. Otherwisedrop_wide()clears every wide context andmax_rows()stays at the narrow minimum.chunk_rows()only widens whenwide_rows_ > narrow. - A launch past its kernel.
npu_gemm_rowsroutes toctxw_[role]only whenwide_takes(M, 16, wide_rows_), and the newM > kctx.MDcheck 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.
VERDICT at
|
|
VERDICT: READY at The gate, verbatiminline 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 ( And the arms are one per way it can fail — which is what makes this a decision table rather than a happy path: Four negatives to one positive. The opt-in is opt-in
A new fast path that activates only on an explicit signal is the right shape for this box: One ask: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
- Say which
narrowthe call site passes towide_takes(). With mybatch_width_min()the narrow answer is the minimum over loaded roles (#1903), andwide_family_complete()requireswide > everynarrow 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. - "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_ROWSaccepts 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>
|
ready for review at ee11811 The branch had gone conflicting: main moved the decode-family block out of Re-checked on the merged build, one
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ee11811d4 (carried; 0 own commits since the verdicted commit — delta is main merged in).
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
- 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;
- 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 > 16selects 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
left a comment
There was a problem hiding this comment.
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 andmade > 0decode contexts exist; NPU_WIDE_ROWSoutside 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, withMD = rows; - any failure stops the loop, and
adopt_wide()still gates adoption onwide_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.cppandnpu/forward/src/q4nx_forward.cppcompile (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 usesdevin a different function.npu_pack_test: ok, wide family 21 arms.- A three-way merge onto current main is clean.
|
VERDICT: READY at My one ask is still open, and I want to be precise about why it is small but not cosmetic: |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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.
|
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 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 Three safety properties are the ones I would have asked for, and they are in the arm set rather than implied:
And the scoping is right: nothing changes for an existing pack (a wide set is used only when Head under review: 0c329d0 |
…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>
|
VERDICT: READY at "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 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 One ask: the row-count range is documented as Second ask, smaller: since a wide ELF changes the chunk width (and therefore what a prefill measurement means), one line in Scope of my check: built at this head, ran |
…-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>
… 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>
…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>
Refs #1990 (the wider-M half). Nothing changes for an existing pack: a wide set is used only when the pack's
config.jsonnames one orNPU_WIDE_SEALED_ELFis 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 forNPU_WIDE_ROWSrows (default 32, allowed 17..128).wide_family_complete,chunk_rows,wide_takes) are innpu/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, onenpu-holdticket, one binary, arms default / wide / default / wide)Block medians from the
[bs]lines (ms):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:
Unit proof (no device)
tests/npu_pack_test.cppgains 14 arms:wide family: 14 arms (complete or not, chunk width, which launch takes it), rc 0. Three mutants ofbatch_width.h, each built through the real target:tools/copyright.py --check,tests_referenced_check.pyandci_targets_check.py --checkpass.Second commit (
f1aa6e4): the pack can name the wide setconfig.json:q4nx_t2s_sealed_wide_elf/q4nx_pb16i_sealed_wide_elf(a file in the kernels directory) andq4nx_sealed_wide_rows(default 32).NPU_WIDE_SEALED_ELF=<file>overrides the pack's file;NPU_WIDE_SEALED_ELF=0switches a pack's wide family off.wide_elf_path()inbatch_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:Not covered
tools/npu_plan.pydoes not report the new keys yet, andplan.jsonhas no field for them: the engine reads them fromconfig.json.NPU_WIDE_ROWSvalues are accepted but untested on the device.🤖 Generated with Claude Code