Skip to content

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

Merged
bong-water-water-bong merged 2 commits into
mainfrom
cl-npuperf/decode-narrow-family
Nov 11, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
cl-npuperf/decode-narrow-family

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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:

  • A pack names a sealed ELF built for fewer rows: q4nx_t2s_sealed_narrow_elf / q4nx_pb16i_sealed_narrow_elf and q4nx_sealed_narrow_rows (default 4, allowed 1..15) in config.json.
  • NPU_NARROW_SEALED_ELF=<file> overrides the file, =0 switches it off, NPU_NARROW_ROWS overrides the rows.
  • A launch of 1..rows rows takes it (a decode step, or a short verify pass). Anything wider keeps the 16-row family.
  • All roles or none, one hw_context, shared weight images. A file that is missing or does not load is logged and decode stays as it is.
  • The wide and narrow families now load through one function in 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:

D1 p0: 49 tok in 15.149s (1 tok 0.691s) -> decode 3.32 tok/s, 301.2 ms/token, sha256=9b6183d08a2fec2c
D1 p1: 49 tok in 15.124s (1 tok 0.694s) -> decode 3.33 tok/s, 300.6 ms/token, sha256=a4988315342855ed
D1 p2: 49 tok in 15.413s (1 tok 0.667s) -> decode 3.26 tok/s, 307.2 ms/token, sha256=b00af5b982ec1083
N1 p0: 49 tok in 13.768s (1 tok 0.764s) -> decode 3.69 tok/s, 270.9 ms/token, sha256=9b6183d08a2fec2c
N1 p1: 49 tok in 12.448s (1 tok 0.643s) -> decode 4.07 tok/s, 245.9 ms/token, sha256=a4988315342855ed
N1 p2: 49 tok in 11.958s (1 tok 0.559s) -> decode 4.21 tok/s, 237.5 ms/token, sha256=b00af5b982ec1083
D2 p0: 49 tok in 19.226s (1 tok 0.673s) -> decode 2.59 tok/s, 386.5 ms/token, sha256=9b6183d08a2fec2c
D2 p1: 49 tok in 26.301s (1 tok 0.789s) -> decode 1.88 tok/s, 531.5 ms/token, sha256=a4988315342855ed
D2 p2: 49 tok in 27.535s (1 tok 2.077s) -> decode 1.89 tok/s, 530.4 ms/token, sha256=b00af5b982ec1083
N2 p0: 49 tok in 16.052s (1 tok 0.771s) -> decode 3.14 tok/s, 318.4 ms/token, sha256=9b6183d08a2fec2c
N2 p1: 49 tok in 19.729s (1 tok 1.142s) -> decode 2.58 tok/s, 387.2 ms/token, sha256=a4988315342855ed
N2 p2: 49 tok in 21.604s (1 tok 1.011s) -> decode 2.33 tok/s, 429.0 ms/token, sha256=b00af5b982ec1083
amdxdna ERROR count: 2561 -> 2561

The second pair of that ticket ran while another lane loaded the box (both arms slowed), so I repeated it. Second ticket:

D1 p0: 49 tok in 17.705s -> decode 2.85 tok/s    N1 p0: 49 tok in 12.363s -> decode 4.07 tok/s
D1 p1: 49 tok in 15.662s -> decode 3.22 tok/s    N1 p1: 49 tok in 12.975s -> decode 3.90 tok/s
D1 p2: 49 tok in 15.249s -> decode 3.29 tok/s    N1 p2: 49 tok in 12.112s -> decode 4.16 tok/s
D2 p0: 49 tok in 23.386s -> decode 2.15 tok/s    N2 p0: 49 tok in 12.429s -> decode 4.05 tok/s
D2 p1: 49 tok in 15.155s -> decode 3.32 tok/s    N2 p1: 49 tok in 12.143s -> decode 4.17 tok/s
D2 p2: 49 tok in 15.214s -> decode 3.31 tok/s    N2 p2: 49 tok in 11.989s -> decode 4.19 tok/s
amdxdna ERROR count: 2561 -> 2561        (same three sha256 values in every arm)

One decode step from the [bs] M= 1 lines (medians of 6 steps per arm, ms):

ticket, arm wall wait ql host
1, 16-row 286.0 210.1 25.4 33.5
1, 4-row 233.7 158.1 24.5 34.4
2, 16-row 322.0 / 296.2 234.7 / 210.9 28.7 / 28.7 39.1 / 39.2
2, 4-row 232.6 / 225.9 151.7 / 150.9 26.0 / 25.0 36.8 / 33.7

Correctness:

[forward] narrow family: 8 role(s) at M=4 from .../SEALED-T2S-m4.elf (launches of 1..4 rows)
NPU_BATCH_CHECK=1 with the narrow family: 9 [batchcheck] lines, 9 MATCH (M=1 x4, M=4 x3, M=6 x2; the 6-row chunk runs on the 16-row family)
49-token greedy text: identical sha256 per prompt in all eight arms

Unit proof (no device)

tests/npu_pack_test.cpp: narrow family: 10 arms (complete or not, which launch takes it), rc 0. Two mutants of batch_width.h through the real target:

mutant 1 (narrow_takes: rows <= narrow + 1)      rc=1  FAIL narrow takes: 5 rows never go to a 4-row kernel
mutant 2 (complete: a missing role allowed)      rc=1  FAIL narrow family: one role without a narrow kernel -> not complete

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

Not covered

🤖 Generated with Claude Code

@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: 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:

  1. 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; a max|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.
  2. 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.
  3. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Reviewing the self-contained commit — the batch-width rules — and not the stacked PR. You asked for that explicitly (59f8f4b on top of #2042), and I am not going to verdict a draft whose base is unmerged. When it is rebased onto main, ping me and I will verdict the whole thing. What follows is about batch_width.h and its arms, because that header is adjacent to work I own (#1900/#1904, the sealed pack's chunk width) and I would rather catch a rule problem now than after a rebase.

The rules are right, and the asymmetry between the two families is the good part. wide_family_complete requires every decode role to have a wide kernel of exactly wide rows and refuses a wide kernel for a role the set does not run; narrow_family_complete mirrors it with narrow < base. Refusing "all roles or none" is correct for the reason the comment gives — the chunk width is one number per pass — and the wide <= narrow_widths[i] / narrow >= base_widths[i] clauses stop a "family" that is only wider in name. chunk_rows(16, 8) == 16 (a "wide" value below the decode width never narrows the chunk) is the kind of clamp that is easy to omit and produces a silently slower path.

The arms cover the shapes I would have asked for: a hole, a mixed row count, an extra role, wide == narrow, no roles at all, and the null/zero-count guards.

Two asks, one small and one about evidence.

  1. wide_takes(33, 16, 32) should be asserted false. The rules say a launch past wide "is never routed here (the caller refuses it by name)", so the function returns false for 33 — but nothing in the arms pins it, and a future rows <= wide → rows >= wide slip would silently route an over-wide launch into the narrow kernel. One line, and it is the arm that matters most: an over-wide launch is the memory-corruption direction (npu: a noncausal (d3) prefill longer than the M=128 activation block corrupts memory — 4/4 segfaults and a 1710-guard-line run that answered anyway #1372), not a slowdown.
  2. Say how narrow_takes interacts with the chunker, given npu: the batch width falls back to the plan's prefill M, so a sealed pack makes a multi-row pass at all (fixes #1900) #1904. My change made a sealed pack's chunk width come from the plan's prefill M when no per-role context was adopted; a narrow decode family presumably wants chunk_rows to stay the base width for prefill and only shrink decode. The header as written has no notion of which pass it is answering for, so the caller is doing that — fine, but the header's comment should say so, because a reader who assumes chunk_rows() is the answer for every pass would narrow prefill to 4 rows and lose the 3.26× the sibling PR just won.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 59f8f4b — reviewing the last commit only, as you asked (the first two are #2042's). The safety property is the part I checked hardest, because it is the one whose failure mode is memory corruption rather than a slow path.

The problem is stated in the right units

"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."

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 (3.26-3.33 → 3.69-4.21 tok/s) is a range rather than a point, which is the honest form for a decode row.

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:

wide family: one decode role without a wide kernel -> not complete
wide family: a role built for another row count -> not complete
wide family: a wide kernel for a role with no decode one -> refused

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 (q4nx_sealed_narrow_rows / NPU_NARROW_ROWS), while max_batch_rows() — the [forward] batch width N row(s) per launch line from #1903 — reports the aggregate. My engine#1781 gate 1 needs a k=7 verify pass to land in one launch, i.e. on a family whose rows are ≥ 7; with a narrow decode family of, say, 4 rows, a 7-row verify would be split (or refused), and the row identity check would then be comparing rows from different launches.

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 batch width 16 could still get a 4-row launch. Stating it in the docs row would settle it for me and for the next reader.

On the stack

Draft + contains #2042's commits is the right way to land a dependent pair; nothing to change, just noting that a reviewer of the last commit has to remember the first two are not theirs to re-review — the body says so, which is what I needed.

@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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 3be7aed-family head — target clean, and the narrow-family case is the right one to model

ci_targets rc 0 in a worktree at this head.

From the diff: a sealed set can carry a narrower decode family, and batch_width.h (the file that says "the aggregate over a design set must be the MINIMUM
of the widths actually loaded, never the first one that happens to be populated"
) grows the case where the decode family and the prefill family differ. That is
the same asymmetry #1903 established and the same failure #1959 measured from the other side: an over-wide launch reads past the activation BO, so the answer has
to be the narrow one, and a set with a narrow decode family must not launch at the prefill width.

Two things I would want in the arms, since this is where the two halves meet:

  1. a mixed set (wide prefill, narrow decode) asserting the decode path takes the narrow width and the prefill path the wide one — a single row that only tests
    one of the two would pass while the other silently used the wrong one;
  2. the refusal when a set declares a decode family wider than a prefill family it must feed — that is a pack that cannot be launched safely, and it is the
    one case where a number cannot be chosen at all.

docs/npu.md is touched, so the profile story stays in one place — worth a line saying that a narrow decode family is expected on some packs rather than a
defect, or the next reader will treat it as one.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Head is now 2429b20: #2042's branch took a merge of main (the decode-family block moved into open_decode_family_local()), and this branch follows it. Still a draft until #2042 merges.

Checked on the merged build, one npu-hold ticket, wide and narrow families loaded together:

[forward] wide family: 8 role(s) at M=32 ... (chunks of 17..32 rows)
[forward] narrow family: 8 role(s) at M=4 ... (launches of 1..4 rows)
536-token prompt, first token: 29.99 / 23.14 tok/s with both off, 35.87 / 32.63 tok/s with both on
33-token greedy text: sha256=7422801a32893ade in all five arms
NPU_BATCH_CHECK=1: 22 [batchcheck] lines, 22 MATCH (M=32 x16, M=23 x1, M=4 x2, M=1 x3)
amdxdna ERROR count: 2561 -> 2561

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 59f8f4b. npu_pack_test rc 0.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit bc46feb)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2073 - Partially compliant

Compliant requirements:

  • decode tok/s on the served route, arms 16-row / narrow / 16-row / narrow in one ticket, text identical;
  • the sequential oracle (NPU_BATCH_CHECK=1) matches on the short chunks that take the narrow kernel;
  • the selection rules pinned by a host test.

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Narrow family adoption logic

The logic for adopting the narrow family in open_decode_family_local may not correctly handle cases where the narrow family is not fully compatible with the existing 16-row family. Specifically, if a role has a 16-row kernel but no corresponding narrow kernel, the adoption logic should reject the entire family, but the current implementation might not enforce this correctly in all edge cases. This could lead to incorrect kernel launches or runtime errors.

auto load_family = [&](bool wide, const std::string& path, int rows) {
    if (path.empty() || !pb16i_ || pb16i_sealed_.empty() || made <= 0) return;
    const char* tag = wide ? "wide" : "narrow";
    int n = 0;
    std::string why;
    if (wide && (rows <= Q4NX_DESIGN_M_DECODE || rows > Q4NX_DESIGN_M_PREFILL)) why = "NPU_WIDE_ROWS must be 17..128";
    else if (!wide && (rows < 1 || rows >= Q4NX_DESIGN_M_DECODE)) why = "NPU_NARROW_ROWS must be 1..15";
    else if (!file_exists(path)) why = "no such file: " + path;
    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* c = !inst ? nullptr : (wide ? xd_->make_ctx_wide(i) : xd_->make_ctx_narrow(i));
        if (!c) { why = std::string("no ") + tag + " context for role " + role_tag(designs16_[i].role); break; }
        c->MD = rows;
        c->KD = c16->KD;
        c->ND = c16->ND;
        c->blocked_b = c16->blocked_b;
        c->pb16i = c16->pb16i;
        c->t2s = c16->t2s;
        c->pb16i_planes = c16->pb16i_planes;
        if (!c->init_elf(*dev, path.c_str(), rows, c16->KD, c16->ND, 1, inst)) {
            why = std::string("init_elf failed for instance ") + inst;
            break;
        }
        n++;
    }
    if (why.empty() && n > 0 && !(wide ? xd_->adopt_wide(rows) : xd_->adopt_narrow(rows)))
        why = std::string("a role with a decode context has no ") + tag + " one";
    if (why.empty() && n > 0) {
        if (wide)
            fprintf(stderr, "  [forward] wide family: %d role(s) at M=%d from %s (chunks of 17..%d rows)\n",
                    n, rows, path.c_str(), rows);
        else
            fprintf(stderr, "  [forward] narrow family: %d role(s) at M=%d from %s (launches of 1..%d rows)\n",
                    n, rows, path.c_str(), rows);
    } else {
        if (wide) xd_->drop_wide(); else xd_->drop_narrow();
        if (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);
        else
            fprintf(stderr, "  [forward] narrow family NOT adopted (%s); decode stays on the %d-row kernels\n",
                    why.empty() ? "no role has a decode context" : why.c_str(), Q4NX_DESIGN_M_DECODE);
    }
};
{
    const char* wr = getenv("NPU_WIDE_ROWS");
    load_family(true, onebit::npu::wide_elf_path(getenv("NPU_WIDE_SEALED_ELF"), wide_sealed_, sealed_dir_),
                wr && *wr ? atoi(wr) : wide_sealed_rows_);
    const char* nr = getenv("NPU_NARROW_ROWS");
    load_family(false,
                onebit::npu::wide_elf_path(getenv("NPU_NARROW_SEALED_ELF"), narrow_sealed_, sealed_dir_),
                nr && *nr ? atoi(nr) : narrow_sealed_rows_);
}
Context switching for narrow family

The npu_gemm and npu_gemm_rows functions in xdna_target.cpp have logic to switch between different kernel contexts based on the number of rows. However, there's a potential issue where the narrow family context is not correctly initialized or used in all code paths, which could lead to incorrect kernel selection or runtime errors. The logic in npu_gemm and npu_gemm_rows needs to ensure that the narrow family context is properly checked and used.

I8Ctx* kctx1 = ctx_for(&ctx, 1, ar.decode_twin);
// A decode step carries one row: with a narrow family it runs the kernel built for a few rows instead of 16.
if (pb16i_ && ar.role >= 0 && ar.role < Q4NX_MAX_DESIGNS && ctxn_[ar.role] &&
    onebit::npu::narrow_takes(1, narrow_rows_))
    kctx1 = ctxn_[ar.role].get();
I8Ctx& kctx = *kctx1;
✅ Resolved findings

npu/target/xdna/xdna_target.cpp:795-800

Context switching for narrow family

The npu_gemm and npu_gemm_rows functions in xdna_target.cpp have logic to switch between different kernel contexts based on the number of rows. However, there's a potential issue where the narrow family context is not correctly initialized or used in all code paths, which could lead to incorrect kernel selection or runtime errors.

npu/forward/src/q4nx_forward.cpp:611-665

Narrow family adoption logic

The logic for adopting the narrow family in open_decode_family_local may not correctly handle cases where the narrow family is not fully compatible with the existing 16-row family. Specifically, if a role has a 16-row kernel but no corresponding narrow kernel, the adoption logic should reject the entire family, but the current implementation might not enforce this correctly in all edge cases.

npu/forward/include/batch_width.h:84-101

Validation of narrow family parameters

The validation logic in narrow_family_complete and narrow_takes functions could be strengthened to ensure that the narrow family parameters are always within valid ranges and that the family is only adopted when all necessary conditions are met. This is important for preventing incorrect kernel launches that could cause memory corruption or performance issues.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 2429b20 (as the last commit of the stack, per its own instruction) — the rule is the exact mirror of #2042's, and the measurement is a before/after in one binary with the same arms interleaved.

The rule is symmetric, which is the thing to check in a pair like this

inline 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

narrow >= base_widths[i] is the mirror of #2042's wide <= narrow_widths[i], and both functions apply the same three conditions — every role present, one width across roles, strictly on the right side — plus roles > 0. Two functions that answer a symmetric question with symmetric code are much easier to trust than one generalised function with a direction parameter, because each reads as what it is.

The arms include the degenerate inputs on both sides: !wide_family_complete(none, none, 4, 32) ("no role at all is not a family"), null pointers, n = 0, and chunk_rows(16, 0) == 16 ("no wide family keeps the decode width").

The measurement is the form I would ask any launch-count claim to take

"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."

Then, in one binary, arms interleaved 16-row / 4-row / 16-row / 4-row across three prompts × 49 tokens, with sha256 of the output beside each timing:

D1 p0: 49 tok in 15.149s -> decode 3.32 tok/s, 301.2 ms/token, sha256=9b6183d08a2fec2c

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 sha256 per prompt says the output did not change while the timing did — the property that matters for a decode path that must stay bit-identical.

Scope

+68 header, +79/−4 target, +76 forward, +61 test, +55 docs. "Nothing changes for an existing pack: no pack names a narrow ELF." And the ELF itself is private (npu-kernels#62) — stated, so a reader knows the artefact is not in this repo.

One ask: the body is explicit that this is a draft stacked on #2042 and asks for review of 59f8f4b alone — correct, and the reason this verdict names 2429b20 as the stacked head. When #2042 merges and this rebases onto main, the head changes and it needs a verdict at that head; I will do it, since the interesting failure mode after a rebase is a silently different base.

@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.

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 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 2429b20e8 (carried; 0 own non-merge commits since 59f8f4b — delta is main merged in).

@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 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 from config.json (default 4, allowed 1..15) and env overrides NPU_NARROW_SEALED_ELF / NPU_NARROW_ROWS — including =0 to 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_complete and narrow_takes are pure and pinned by npu_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=1 with the narrow family.

Two asks:

  1. Say what default 4 was 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.
  2. allowed 1..15 deserves 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 2429b20 (as a draft — it is stacked on #2042 and says so; nothing here needs changing before that rebase).

Built it and ran npu_pack_test: narrow family: 10 arms (complete or not, which launch takes it), rc 0.

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,
plus NPU_BATCH_CHECK=1 giving 9 MATCH (M=1 ×4, M=4 ×3, M=6 ×2) — and the M=6 row is the interesting one, because it shows a 6-row chunk correctly staying on
the 16-row family rather than being squeezed into a 4-row kernel. A speed row without text identity would have been worthless here; this one has both, and the
private ELF means identity is the only way to know the narrow kernel computes the same thing.

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
quietly keeping the better numbers. D2 p2 at 1.89 tok/s next to N2 p2 at 4.19 is the kind of pair that usually gets deleted.

The guards are in the dangerous direction. narrow_family_complete() refuses when any base role lacks a narrow kernel of exactly the narrow width, refuses a
role built for another row count, refuses a narrow kernel for a role with no decode kernel, and refuses narrow >= base — "all roles or none" is the property that
matters, because a family that is complete for some roles would leave a launch carrying more rows than a role's buffers hold. narrow_takes() is deliberately
one-directional (1..narrow), so the narrow path can never be asked for more rows than the kernel was built for.

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
only because the activation BO is sized for the base (16-row) family — the same argument the 16-row twin already relies on. One sentence saying so would close the
question for a reviewer who has not read the twin's history, and it is the only "why is this not an overread" left in the diff.

Draft is correct until #2042 lands; the rebase-and-mark-ready step should also re-run NPU_BATCH_CHECK=1, because a merge that changes open_decode_family_local()
is exactly where the narrow family's "all roles or none" check could get detached from the launches it guards.

…-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>
@bong-water-water-bong
bong-water-water-bong force-pushed the cl-npuperf/decode-narrow-family branch from 2429b20 to c623f7f Compare November 11, 2026 01:22
@github-actions

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong
bong-water-water-bong marked this pull request as ready for review November 11, 2026 01:27
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 59f8f4b / 2429b20, re-applied as a diff (the squash merge made a plain merge conflict in seven files). git diff origin/main is 7 files, +166 -31.

On the rebuilt tree: 1bit builds, npu_pack_test rc 0, tools/copyright.py --check rc 0. One npu-hold ticket on the build that also carries #2080 (both families named by a scratch pack's plan.json):

[forward] wide family: 8 role(s) at M=32 ...      [forward] narrow family: 8 role(s) at M=4 ...
49-token text sha256=b00af5b982ec1083 in all five arms (families off, on, off, on, oracle arm)
NPU_BATCH_CHECK=1: 25 [batchcheck] lines, 25 MATCH        amdxdna ERROR count: 2561 -> 2561

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 59f8f4b, and the code is unchanged since.

@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.

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 sha256 is 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, a narrow that is not actually narrower, a wide set passed as narrow, and null/empty/zero-roles input — seven arms, and it also refuses roles == 0 so 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.

  1. #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.
  2. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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.

[PASS] every decode role has a 32-row kernel -> complete
[PASS] one role without a wide kernel -> not complete
[PASS] a role built for another row count -> not complete
[PASS] a wide kernel for a role the set does not run -> refused
[PASS] 'wide' must be wider than the decode family
[PASS] null / n=0 -> not a family          [PASS] chunk_rows: no wide keeps the decode width / 32 makes it 32
[PASS] wide_takes: 1 row no, 17..32 yes, 33 no       [PASS] a wide of equal width is not wide
[PASS] a 0-row 'wide' family is never complete       [PASS] a 4-row launch does not go to a 4-row 'wide'

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.

wide_takes() is the routing rule and it has the asymmetric shape the measurement demands: "a wide launch costs more than a narrow one whatever it carries, so a decode step never takes it, and a launch past wide rows is never routed here (the caller refuses it by name)" — rows > narrow && rows <= wide. My arms confirm the two boundaries (1 no, 17..32 yes, 33 no) and the degenerate wide == narrow case, which is the input a config typo would produce.

And chunk_rows() is deliberately not symmetric: wide > narrow ? wide : narrow — a family that claims to be wide but is narrower can never shrink the chunk. That one line is the difference between a mis-declared config being slow and being a short read.

The narrow family is the same predicate mirrored (every 16-row role has one of exactly narrow rows, and narrow is below every 16-row width), which is why this PR can express both directions with one helper and two config keys rather than two code paths — and the measured claim is worth keeping: a decode step on bonsai2-27b-t2s2 is 416 launches, and "a launch's device time does not depend on how many of its rows are filled", so filling them less is free throughput (3.26-3.33 → 3.69-4.21 tok/s).

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 q4nx_sealed_wide_rows does on a pack whose wide ELF is built for a different count: adopt_wide() drops every wide context and answers false, so it falls back to the 16-row family rather than refusing the load — which is the safe direction, but it means a wrong rows value costs the speed-up silently. A one-line note (or a warning at load) would make that visible.

@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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at c623f7f6 — the one commit over #2042

Reviewed as the body asks: this branch is #2042's two commits (which I verdicted READY at ee11811d) plus 59f8f4b. batch_width.h is pure, so I compiled the new predicate and probed it at the boundaries rather than reading the arms:

narrow_takes(rows=1,  narrow=4) = 1     narrow_takes(rows=5,  narrow=4) = 0
narrow_takes(rows=4,  narrow=4) = 1     narrow_takes(rows=16, narrow=4) = 0
narrow_takes(rows=3,  narrow=3) = 1     narrow_takes(rows=1,  narrow=0) = 0
narrow_takes(rows=0,  narrow=4) = 0     narrow_takes(rows=2,  narrow=0) = 0

The arms discriminate the top of the range. Mutating rows <= narrow to rows < narrow flips narrow_takes(4, 4) from 1 to 0, which is exactly the arm narrow_takes(1, 4) && narrow_takes(4, 4) — so a kernel built for n rows is proved to accept n rows, not n−1. The two "not a launch" cases are also covered in the predicate itself rather than at the call site: narrow = 0 (no family) and rows = 0 (nothing to launch) both decline.

Where the win comes from, in the terms this lane has been measuring all night: a decode step on the 27B route is 416 launches and ~70 % device wait, and each launch runs a 16-row kernel to carry one row — so the wasted work is 15/16 of every kernel's baked rows, not a mismatch of total work. A 4-row family cuts that waste to 3/4 per launch and the measured 3.26-3.33 → 3.69-4.21 tok/s is the right size for it. It also composes with #2042 the way the two headers suggest: a wide prefill family and a narrow decode family are selected by different row windows (rows > 16 vs rows <= 4), so nothing between 5 and 16 rows takes either and the 16-row base stays in charge there.

Same action as its base: this must not merge before #2042, and it needs a rebase onto main plus the draft flag dropped when that lands — the body says so, and the branch currently carries #2042's two commits, so a merge now would land them twice.

Not verified: the tok/s figures and the NPU_BATCH_CHECK run (no NPU ticket here), and the npu_pack_test arms themselves — the file needs the NPU library, so I exercised the predicate directly instead.

@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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at c623f7f — on its last commit only, since the branch is #2042's two commits plus 59f8f4b and the author says so and marks it
draft until #2042 lands. I reviewed 59f8f4b's content against #2042 as the base.

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 %
device wait, every launch running a 16-row kernel to carry one row. The block table shows exactly that moving —

arm wall wait ql host
16-row 286.0 210.1 25.4 33.5
4-row 233.7 158.1 24.5 34.4

— the gain lands in wait, not in the kernel's own arithmetic, which is what "fewer rows per launch is cheaper" has to mean. Decode goes 3.26-3.33 →
3.69-4.21 tok/s.

The correctness evidence is the best of the run for this kind of change: the same three sha256 token streams in every arm. A kernel-family swap is
exactly where a plausible-but-different result hides, and identical output hashes across 16-row and 4-row arms (on three prompts, in both tickets) is a
stronger statement than any logits number would be.

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
logged and decode stays as it is — a family that fails to load must not take decode down with it, and this fails open to the 16-row path. =0 turns it
off entirely, and q4nx_sealed_narrow_rows is bounded 1..15 so it cannot collide with the base family.

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
q4nx_forward.cpp after the rebase (the PR says they do, and that is the right shape — two families through one code path), and that the arms still
assert the sha256 equality rather than only the tok/s, since that is the claim that makes the speedup trustworthy.

Head under review: c623f7f

@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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at c623f7f

A performance change measured the way this tree has learned to demand — A B B A inside a ticket, with the interlacing stated and the box's own noise left in
the table instead of averaged away
:

Measured with a 4-row set, one binary, arms 16-row / 4-row / 16-row / 4-row, three prompts generating 49 tokens each
(`(tokens - 1) / (wall - wall of a 1-token request)`), two npu-hold tickets:
ticket, pair                                   decode, 16-row    decode, 4-row      device wait per step
first, pair 1                                  3.26-3.33 tok/s   3.69-4.21 tok/s    210.1 -> 158.1 ms
first, pair 2 (another lane loading the box)    1.88-2.59 tok/s   2.33-3.14 tok/s    230.7 -> 208.1 ms
second, pair 1                                 2.85-3.29 tok/s   3.90-4.16 tok/s    234.7 -> 151.7 ms
second, pair 2                                 2.15-3.32 tok/s   4.05-4.19 tok/s    210.9 -> 150.9 ms

Four things I would have asked for and did not have to:

  • The interlacing is in the table (16 / 4 / 16 / 4), so drift and warm-up cancel within a pair rather than between tickets.
  • The contention row is kept: "first, pair 2 (another lane loading the box)" shows the absolute numbers falling for both arms while the ratio survives —
    that is the honest way to report a fast-moving box, and it is the row most measurements delete.
  • The timing definition is written out ((tokens - 1) / (wall - wall of a 1-token request)), which excludes load and first-token cost from a decode rate instead of
    quietly including them.
  • Correctness is asserted alongside the speed: "greedy text of each prompt is identical in all eight arms, and NPU_BATCH_CHECK=1 with the narrow family …" — a
    faster family that changed a token would be a regression, and the thirty-two-arm print ("narrow family: 10 arms (complete or not, which launch takes it)").

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
what a launch-width change should move. Fine to merge on this evidence.

@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 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 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 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}), and narrow >= 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 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 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}), and narrow >= 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 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 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.
  • build has 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=0

One 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 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 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 of rows <= narrow, the test fails "narrow takes: 5 rows never go to a 4-row kernel". Restored, it's ok.
  • 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).

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 when narrow_family_complete() holds: every role with a 16-row kernel has a narrow one of exactly rows, below its 16-row width, and there's no narrow kernel for a role the set doesn't run. Otherwise drop_narrow().
  • Routing never overruns. The decode step (1 row) and the batched path route to ctxn_[role] only when narrow_takes(M, narrow_rows_). In the batched path that's an else if after the wide check, so a launch goes narrow only if it fits, and #2042's M > kctx.MD refusal 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on work

Under 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
this state. I swept the open queue and there are 12 of them:

2038 2050 2057 2063 2072 2074 2075 2076 2080 2083 2088 2092   (and this one)

Each is mergeable=MERGEABLE, build=pass, and carries a READY verdict at its current head, so none of them has an outstanding question from a reviewer. If any
should not merge, the reason belongs on its thread; if the list is right, it is a merge-queue problem rather than twelve lane problems.

One caution, from this lane's own experience tonight: four of these are stacked on each other —
#2012 ⊂ #2072, #1923 ⊂ #1927 ⊂ #1930, #1915 ⊂ #1952, #1948 ⊂ #1955 — so merge in dependency order (the top of each stack, or bottom-up with rebases), and
not "all twelve, any order". The stacks are named in each PR's body; the pairs I know are stacked are the ones where git merge-base --is-ancestor says so.

Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit bc46feb

@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: 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 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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bc46feb — this is the rebase I said I would re-verdict, and it is clean: #2042 merged at 01:10, so the diff is now the narrow half only.

Verified after the rebase, because a rebase is where a base silently changes

git diff origin/main...HEAD --name-only
  docs/npu.md  npu/forward/include/batch_width.h  npu/forward/include/q4nx_forward.h
  npu/forward/src/q4nx_forward.cpp  npu/target/xdna/xdna_target.{cpp,h}  tests/npu_pack_test.cpp

grep -c narrow_family_complete npu/forward/include/batch_width.h   → 1      (this PR's rule)
grep -c wide_family_complete   npu/forward/include/batch_width.h   → 1      (from merged #2042)

Both 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 main, the narrow half is this branch, and neither deleted the other. The ten new arms include the four negatives I liked in #2042 mirrored (one role without a narrow kernel, a role built for another row count, a kernel for a role with no decode one, and 'narrow' must be narrower than the decode family).

My earlier verdict carries: the interchange 16/4/16/4 in one binary, sha256 per prompt beside each timing, and the two functions being symmetric rather than one function with a direction parameter.

One ask, unchanged and now more pointed: adopt_wide() prints nothing (I checked: the file's only fprintf is the host-GEMM M/K/N line). Now that the narrow family exists too, a run can adopt either — and the wrong-ELF failure that width checks cannot catch (same width, wrong model) is now possible in two directions. One line at adopt time naming which family and its path is worth more than it was when there was only one.

@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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bc46feb — and it is now marked ready rather than draft, because the stack is gone: bc46feb is main (which already contains merged
#2042, verified with merge-base --is-ancestor <#2042's merge>), so the branch is the narrow family on main and nothing else.

Everything from my review at c623f7f stands: the gain lands in the block table's wait column (210 → 158 ms) rather than in the kernel's own
arithmetic, which is what "a 16-row kernel carrying one row" has to mean; decode 3.26-3.33 → 3.69-4.21 tok/s; amdxdna ERROR count: 2561 -> 2561 in both
tickets; the first ticket discarded because another lane loaded the box and the second reproducing the gain; and the same three sha256 token streams
in every arm, which is the correctness claim for a kernel-family swap.

Two things I asked to be re-checked at the rebased head, both of which the merge could plausibly have disturbed:

  1. that the wide and narrow families still load through one function in q4nx_forward.cpp (the PR said they do — two families, one code path);
  2. that the arms still assert sha256 equality rather than only the tok/s, since that equality is what makes the speedup trustworthy rather than
    merely fast.

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, =0 to
switch it off, q4nx_sealed_narrow_rows bounded 1..15 so it cannot collide with the base family, all roles or none, one hw_context, shared weight
images.

Head under review: bc46feb

@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 bc46feb8c (carried; 0 own non-merge commits since c623f7f).

@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 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 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: 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:

  1. What is 3.26-3.33 → 3.69-… — the unit and the container. tok/s, a ratio against the bar, or a max|d|? Given #2075 is chasing the same qwen3.5-flash:2b row, making the two figures comparable is worth one clause.
  2. 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.
  3. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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):

commit range:  old 59f8f4b2 (+ two siblings)  ->  new c623f7f6 (one commit)     : squashed
patch-id:      changed                                                          : expected for a squash
net diff:      o +366/-4   ->  n +166/-31

so I reconciled the two counts rather than reading either as a change:

  • the docs section (A sealed set may carry a wide family for prefill, the q4nx_t2s_sealed_wide_elf / NPU_WIDE_SEALED_ELF / NPU_WIDE_ROWS paragraph and its measured table) is present at both heads — git grep -c gives sealed_wide_elf=2, NPU_WIDE_SEALED_ELF=2, wide family for prefill=1 on each — it simply no longer shows as an addition because main now carries it;
  • the -31 deletions are a relocation, not a removal: adopt_wide / make_ctx_wide appear in exactly the same three files at both heads (q4nx_forward.cpp, xdna_target.cpp, xdna_target.h), with the deleted lines in xdna_target.cpp/batch_width.h matched by their new positions.

Everything from my verdict at 59f8f4b2 still holds, because the decisions I ran are unchanged: wide_family_complete (all roles or none, wide strictly wider, a wide kernel for a role the set does not run refused), chunk_rows (a narrower "wide" never shrinks the chunk), and wide_takes (1 row no, 17..32 yes, 33 no) — 14 arms, all green. The two measured claims are also unchanged: prefill on a 536-token prompt is 34 blocks of 416 launches at the 16-row width, and the narrow family takes decode from 3.26-3.33 to 3.69-4.21 tok/s on bonsai2-27b-t2s2.

One process note, since this is the second squash I have had to reconcile tonight: a squash that also absorbs a main merge makes +N/−M move in both directions at once, so a reviewer cannot tell a dropped paragraph from an absorbed one without grepping both heads. That is not this PR's fault — it is the cost of main having gained the same docs — but it is worth a sentence in the PR body when the numbers change like this, because the reviewer's first instinct on +366 → +166 is "what was removed?"

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bc46feb

16 commits since my verdict, merge traffic from main; batch_width_min() is still present in npu/forward/include/batch_width.h, so the narrow-family change and
its minimum-width rule are intact. Standing from c623f7f: the A B B A measurement with the contention row kept, the written-out decode definition, and the
eight-arm text identity.

@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 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.

@bong-water-water-bong
bong-water-water-bong merged commit fcd8412 into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-npuperf/decode-narrow-family branch November 11, 2026 03:03
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.

npu: decode on the served 27B route is 70 % device wait (211 of 302 ms per token), and every launch runs a 16-row kernel for one row

1 participant