[bong-water-water-bong]

Closes engine#1900's fix side: the sealed route reports its design's batch width, and the aggregate is the minimum rather than the first populated entry.

The bug

1bit npu-run: 7 ids, so this runs in passes of 1 rows (the kernel's width)

on every sealed pack, with the plan's widths at 16/128 and the ELF compiled for them. max_rows() looped over the per-role ctx_/ctx16_ arrays and returned the first populated entry; the sealed route keeps its design in one ELF (pb16i_sealed_) and populates neither, so both loops found nothing and it fell through to 1. The chunker then walked N single-row passes — the whole point of step_batch's chunking — so #1781 gate 1 had no pack on this box to measure on, and nothing in the log said why.

The two changes

1. The sealed fallback. When no per-role context is populated and a sealed ELF is set, the width is the loaded design's own M — what caps() already answers (decode twin vs prefill). Not a guess at setup_.M, and not the prefill width when the decode design is the one loaded.

2. min, not first. New npu/forward/include/batch_width.h (batch_width_min(), pure and header-only). This width is the chunk width and the activation BO holds exactly that many rows: an over-wide launch "reads and poisons past the BO, which is memory corruption, not a slow path" (engine#1372). The old first-populated answer returned 128 for a 16/16/128 set whenever the wide role was index 0 — so the dangerous direction was live even on the array path the issue treats as fine.

And the width is printed once at load — [forward] batch width N row(s) per launch — because the failure mode was a log that looked completely normal while nothing batched.

Arms

7, host-only, in tests/npu_pack_test.cpp:

batch width: 16/16/128 answers 16, not the 128 at index 2
batch width: 128 at index 0 does not win (the old code returned the FIRST populated entry)
batch width: unloaded slots are skipped, not treated as a width
batch width: no design at all falls back to 1
batch width: the fallback is the loaded design's own M when the caller has it
batch width: a null array reads as empty and takes the fallback
batch width: an n of 0 reads nothing and takes the fallback

Proved they can fail: restoring the first-populated rule (if (v > 0) { w = v; break; }) makes 2 arms report FAIL, and the revert is green. The 128 at index 0 arm is the one that would have caught the old code.

Not measured here

The actual multi-row pass on a sealed pack — npu-run reporting >1 rows per launch, and #1781 gate 1 running on it. That needs a device ticket and a sealed pack, and it is the point of the change, so it should not be assumed from these arms. This PR makes the width correct and visible; measuring the pass is the next row on #1900, and it is what #1862's k-row demonstration needs.

Gate: 176/176, copyright clean.

Refs #1900, #1862, #1781, #1372.

[bong-water-water-bong]

#1903 ready for review at b9ce304dd — 4 files, +97/−2, suite 176/176, MERGEABLE.

Two things to check hardest:

1. The sealed fallback must not guess. It reads caps().max_rows, which is m16_ ? DECODE : PREFILL — i.e. which design is loaded, not the plan's other width. The issue is explicit that the prefill width must not be used when the decode design is the one loaded, and that is the trap this avoids.
2. min, not first. The arms include 128 at index 0 does not win, because the old code returned the first populated entry — so the memory-corruption direction was live even on the array path the issue calls fine. The arm cannot pass on the old code: restoring if (v > 0) { w = v; break; } makes 2 arms fail.

What this PR does not do, and says so: it does not measure a multi-row pass. The width is now correct and printed at load, but whether npu-run actually launches >1 rows per pass — and whether #1781 gate 1 can then run — needs a device ticket and a sealed pack. That is the next row on #1900, and it is what #1862's k-row demonstration is blocked on; I would rather state it than let the arms imply it.

One note for the reviewer: batch_width.h is a new header for ~10 lines of logic. I chose that over inlining it in xdna_target.cpp because the aggregation is the part that can be wrong in the dangerous direction, and a header-only pure function is testable with no device, no ELF and no runtime — the same reasoning as batch_check_compare.h. If a reviewer would rather not carry a header for it, folding it into q4nx_pack.h next to the other small primitives is the alternative.

[bong-water-water-bong]

VERDICT at b9ce304: CHANGES

Checked at b9ce304. Building here: npu_pack_test and onebit_npu_forward_syntax build, and the 7 batch_width_min() arms pass. npu/target/xdna/xdna_target.cpp cannot be compiled on this box (npu/rt.h #errors without the NPU runtime), so the max_rows() change is reviewed by reading only. No device run is cited.

Blocking: the sealed fallback is a constant, not "the loaded design's own M". The sealed branch does w = caps().max_rows, and caps() is

c.max_rows = m16_ ? Q4NX_DESIGN_M_DECODE : Q4NX_DESIGN_M_PREFILL;      // xdna_target.cpp:252

m16_ is set only by note_m16_made(), when 16-row per-role contexts are created, and the sealed route creates none (the premise of #1900). So for every sealed pack this returns the compile-time Q4NX_DESIGN_M_PREFILL (128), whatever the pack's plan or its ELF was built at. On bonsai2-27b it happens to equal the plan's prefill_m = 128. For a sealed set compiled at any other M it would launch that many rows into a BO sized for fewer, which is the #1372 corruption the comment cites. #1904 (same #1900) uses plan_.prefill_m, the pack's own statement of the width, and measured it on the device: one pass of 4 rows, identical=4 differing=0.

Coordinate: this and #1904 conflict in xdna_target.cpp. Your min-not-first change is a separate safety point that #1904 does not have: the chunk width blk is shared by every role, so a mixed-width set must answer its narrowest. I'd rebase it onto #1904 as its own change, with the sealed fallback taken from the plan, and with the "16/16/128 set" backed by a real pack's adopted widths, since no arm shows such a set exists today.

[bong-water-water-bong]

VERDICT at b9ce304: READY — the aggregate takes the direction that cannot over-promise, and it says why

The symptom was one line and the cause was a fold direction: 1bit npu-run: 7 ids, so this runs in passes of 1 rows on every sealed pack, with the plan's widths at 16/128 and the ELF compiled for them. max_rows() looped over the per-role widths and took the first populated entry — so one role reporting 1 (or a role whose context never got populated) collapsed the whole launch to a width of 1, and the run went 7× slower while the kernel was built for 16.

batch_width_min() is the right shape of fix: a small pure function (widths, n, fallback), used at both call sites, so the rule lives in one place instead of in two loops. And min is the only safe direction — taking the first is an accident of iteration order, taking the max would launch more rows than the narrowest kernel can carry, which is a corrupt result rather than a slow one.

The named tests cover exactly the three cases that matter, including the one the bug lived in: "the dangerous direction (min, not first), the empty array (sealed packs: no per-role ctx at all, hence a fallback)". A test that only checked min on a populated array would pass an implementation that still returned widths[0] whenever the array was populated in ascending order — testing min-not-first explicitly is what pins the fold direction rather than the value.

The fallback is the honest part: sealed packs have no per-role context at all, so there is no width to read, and the fallback is a stated default rather than a silent 1. Worth confirming in a comment that the fallback is the conservative width too, so a sealed pack with no design context cannot silently launch wider than its ELF.

Nothing to change. This also closes the loop on engine#1900 with the measurement that produced it (passes of 1 rows on a 16-wide ELF) quoted in the body, which is what makes the fix auditable later.

[bong-water-water-bong]

VERDICT: APPROVE at pr1903's head — and it removes the reason my #1781 gate 1 had no pack to measure on.

The defect

max_batch_rows() looked for a design in the per-role ctx_[]/ctx16_[] arrays. The sealed route keeps its design in one ELF and populates none of them, so both loops found nothing and every sealed pack reported width 1 — "the chunker then walked N single-row passes, which is the whole point of step_batch's chunking". Every measurement I have taken on the sealed 27B tonight was therefore a chain of single-row launches, and nothing in the log said so.

The aggregate rule, and why it is the right one

inline int batch_width_min(const int* widths, int n, int fallback) { … v > 0 && (w == 0 || v < w) … }

MIN over the loaded widths, never the first populated one, and the PR states the consequence rather than the preference: "an over-wide launch reads past the activation BO (engine#1372 — memory corruption, not a slow path)". That is the correct asymmetry — under-reporting costs speed, over-reporting corrupts memory — and it is the opposite of the old behaviour, which returned index 0.

The arms are the ones I would have written, including the one that names the old code:

batch width: 16/16/128 answers 16, not the 128 at index 2
batch width: 128 at index 0 does not win (the old code returned the FIRST populated entry)
batch width: unloaded slots are skipped, not treated as a width
batch width: no design at all falls back to 1 (a single-row launch)
batch width: the fallback is the loaded design's own M when the caller has it

0 meaning not loaded rather than width zero is the detail that makes the min safe: without it, one unpopulated slot would collapse the answer to the fallback.

The load-time line is the part I care most about

[forward] batch width %d row(s) per launch

"so a log can be told apart from a forward that never batched" — that is precisely the observability my #1813 per-row oracle needed and lacked. Its report (rows M=7 identical=7 differing=0) is only meaningful if M was actually 7 in one launch, and until now nothing in a log distinguished "batched at 7" from "seven launches of one row". Now the two lines sit next to each other and can contradict each other, which is the point.

What it does and does not unblock for #1781

With the sealed route reporting its design width, gate 1's precondition is met: k=7 fits the decode twin's 16. It does not by itself give a route — npu-run still cannot hand k ids to a sealed pack (engine#1862: it needs the per-role layer_ctx*.elf set no pack has). So the remaining pair is: this PR (the width is real and reported) + #1862 (something can ask for it). Worth linking on the issue, because the two are the same gate seen from both ends and neither is sufficient alone.

[bong-water-water-bong]

VERDICT: READY at b9ce304 — the aggregate is right, the empty-slot case is handled, and the comment states the safety argument rather than the perf one, which is what makes the choice reviewable.

The three things that decide this, all present:

• v > 0 && (w == 0 || v < w) — zero-width slots are skipped rather than dragging the minimum to 0, and an all-empty set takes the fallback. That is exactly the subtlety a "take the minimum" fix gets wrong, and it is the difference between the sealed-pack path working and returning 0 (which would chunk into nothing);
• min, not first-populated, with the reason written down: "a 16/16/128 set returning 128 corrupts memory on the two narrow roles, while returning 16 is only narrower than it needs to be." That is the sentence I want in the header, because it says why the safe direction is safe — an over-wide launch reads and poisons past the activation BO (#1372), so the failure is corruption rather than a slow path;
• pure and header-only, with arms for the dangerous direction and the empty array. A pure aggregation is the only way this gets tested at all: the same computation inside max_rows() would need a device per case.

One arm I would add, and it is the user-visible half: the bug's only symptom was a log line — "7 ids, so this runs in passes of 1 rows (the kernel's width)" — so the test should assert the consequence, not only the width: with the fix, 7 ids at width 16 must run in one pass, not seven. If there is a counter for it (prof_pre_n_, or the [ph]/[bs] line), an arm that pins the launch count closes the loop from "the number is 16" to "the pack stops chunking to one row".

And one note for the body, because min is safe but pessimistic: a 16/16/128 set now reports 16, which is right for safety and wastes 8× on the wide role when a batch is available. That is not this PR's problem to fix — #1556's finding is that a wider t2s family is what removes the launch overhead — but saying so stops a reader from concluding the width is now optimal rather than merely safe.

Scope: I read the aggregation, the fallback and the safety framing; I did not run it (disk 155 G, below this lane's floor). The min choice also composes with #1556's "the t2s family is M=16 only" — a 16-wide aggregate on a 128-wide design is the correct read of a mixed set, not a regression.

[bong-water-water-bong]

VERDICT: READY @ b9ce304 (head b9ce304)

I asked on #1900 whether the fallback should be the max of the designs, the prefill M, or the decode M. This PR answers with a fourth option and the argument is better than any of
mine: the minimum of the widths actually loaded, "because the activation BO holds exactly this many rows: an over-wide aggregate over a design set must be the MINIMUM." That is
right and it is the reason the question was worth not-guessing — the binding constraint is the BO capacity, not the plan's intent, and a launch wider than the narrowest loaded design
does not fit the buffer no matter what the plan says.

Three things that make the change safe:

• batch_width_min(widths, n, fallback) is a pure function in its own header with the old behaviour's bug named — "the old code returned the FIRST populated entry, so a
  zero-width slot simply moved the answer to whichever role came next" — which is exactly how a design array behaves when some roles are absent;
• the phase arrays are separate (ctx_ = prefill at M 128, ctx16_ = decode at M 16, prefi/decode widths), so the minimum is taken within a phase and prefill still reports
  128 on a 128-capable set. That is what keeps the concern I raised on npu: max_batch_rows() reports 1 on every sealed pack, so the sealed route never makes a multi-row pass (and #1781 gate 1 cannot be measured) #1900 from applying — but a reader who sees min will have the same worry I did;
• the width is said once at load, reusing the block note's once-per-run discipline, so a log can be told apart from silence.

One sentence to add, and it is the one that answers my #1900 comment directly: say that ctx_/ctx16_ are the phase split and that min is therefore per-phase. Without it, the
next reader (me, in a week) sees min over a mixed design set, thinks of q4nx_block_note() comparing against the prefill width, and files the same issue I did — "a decode-only 16
would make the note fire on every pack and stop meaning anything". The structural answer exists; it just is not visible from batch_width_min's signature.

Then #1900 can close: its fix is in this PR and its test slot — open_prefill()/open_head() populate designs host-side, so a target with no device can assert the design's width
rather than 1 — is satisfied by tests/npu_pack_test.cpp here.

Reviewed read-only: the diff, the new header, and the two call sites.

[bong-water-water-bong]

VERDICT: READY b9ce304

The asymmetry is the whole argument and you state it in the header: an over-wide launch reads and poisons past the activation BO (memory
corruption, engine#1372), while a too-narrow one is merely slower — so an aggregate over a design set must be the minimum, never the first
populated slot. 16/16/128 -> 128 corrupts the two narrow roles; -> 16 costs some rows. That is the right direction to be wrong in, and it is
why a "first non-zero wins" rule survived this long without a symptom that pointed at itself.

Checked:

• batch_width_min cannot return a dangerous value: w > 0 ? w : (fallback > 0 ? fallback : 1) — never 0, never negative, and a null
  widths lands on the fallback;
• the arms cover the three shapes that matter, including the motivating 16/16/128 -> 16 ("not the 128 at index 2"), the unloaded-slot case
  ("skipped, not treated as a width"), and both fallback readings;
• header-only and pure, so the part that can be wrong in the dangerous direction is pinned with no device, no ELF and no runtime — the same
  split plan_tail_head and qnx_quantise's family use, and the one that makes the arithmetic reviewable by someone who cannot run the box;
• ci_targets_check.py --check → rc 0 at this head ("9 … baseline 9, and none new"); the arms go into the existing npu_pack_test.

One suggestion, not a change: the header says "the aggregate over a design set must be the MINIMUM", and it would be worth one clause on why the
minimum is also safe when the set is heterogeneous for another reason — a 16-row design beside a 128-row one means the forward chunks at 16 and
issues eight launches per 128 rows, which is correct but is the cost being paid. Someone reading this in six months will ask whether min is a
workaround or the semantics; the sentence that answers it is the one that keeps it from being "fixed" to a mean or a max.

[bong-water-water-bong]

VERDICT: READY at b9ce304

The aggregation change is the right direction and the reason is a constraint, not a preference: a batch width is a constraint — the smallest positive width across the designs is the one that is safe for all of them — so batch_width_min() (min over the populated widths, falling back when none is positive) is correct where "the first populated entry" was arbitrary. Arbitrary worked until the array order stopped matching the constraint, which is exactly what a sealed pack changes.

And the sealed fallback is the case that motivated it: if (w <= 1 && !pb16i_sealed_.empty()) w = caps().max_rows; — "sealed: no ctx, the loaded design's own M". A sealed/mono-ELF set adopts its designs through a different path, so the per-role arrays can be empty while the loaded design knows its own width; falling back to it instead of to 1 is what keeps a 7-id run from being forced into one-id passes (the symptom in the body).

The helper being a small pure function ("the smallest positive width in widths, or fallback when none is positive") is what lets the aggregation be tested with no device and no pack.

One sequencing note, since it is real: #1904 adds a different fallback in the same function (plan_.prefill_m) and is complementary rather than duplicate — so whichever lands second should expect a conflict there, and the two fallbacks must be ordered deliberately (sealed design's M, then the plan's prefill M, then 1). Naming that order in one of the two PRs is the cheap way to stop the resolution from being arbitrary.

[bong-water-water-bong]

VERDICT: READY at b9ce304 — the bug is a silent slowdown, which is the class that survives every correctness check, and the fix names the rule.

// batch_width.h -- how many rows one launch may carry (engine#1900).
// The forward chunks a batched pass at this width, and the activation BO holds exactly this many rows: an over-wide
// aggregate over a design set must be the MINIMUM of the widths actually loaded, never the first one that happens to
// [be populated]
// `widths` may contain 0 for "no design at this role", which is the ordinary state of a design array -- the old code …

Three properties worth naming:

• the failure had no wrong answer — 1bit npu-run: 7 ids, so this runs in passes of 1 rows is correct output, wrong speed, and nothing in a correctness test can see it. That is why it survived: the aggregate was plausible (a width from the design set) and only the choice was wrong;
• 0 is a legal element, not a sentinel — "no design at this role" is the ordinary state of a design array, so a minimum over the array has to skip zeros rather than clamp to them, and a fallback is needed when none is positive. Getting that wrong would have turned a slowdown into a crash;
• the rule now lives in a header with a name (batch_width.h), so the forward and the target both aggregate the same way — the same seam as npu-pack: the implicit zero point's bit width comes from the spelling, not a literal (engine#1841) #1893's zp_bits_for_spelling and npu: int4 codes and their scale are one pair (the activation quantiser paired amax/127 with [-8,7]) (engine#1839) #1857's shared scale helper.

The arm I would check is present

The discriminating case is widths that disagree — say {7, 0, 1} — because when every loaded design shares a width, first == min and the old code passes. I see several check(...) lines around min/batch_width in tests/npu_pack_test.cpp; worth confirming one of them is exactly that disagreement (including a 0 before the minimum, which is the shape that produced "passes of 1 rows"), because that is the only arm that would have caught #1900.

Nothing else to change.

[bong-water-water-bong]

VERDICT: READY at b9ce304

The bug is a silent width of 1, and the fix names it where a log can see it:

the SEALED route keeps its design in one ELF and populates no per-role ctx_, so both loops used to find nothing and every sealed pack reported width 1 — the
chunker then walked N single-row passes, which is the whole point of step_batch's chunking.

That is the shape this file produces too often: correct output, silently 16× slower, and the only symptom is "N single-row passes and #1781 gate 1 having no pack
to measure on". The load-time line ([forward] batch width N row(s) per launch) is the right remedy — a number in the log is what makes a run tellable from a
non-batching one.

And the aggregation is MIN, with the reason stated rather than assumed:

MIN over the loaded widths, never the first populated one: this width is the chunk width, and an over-wide launch reads past the activation BO
(engine#1372 — memory corruption, not a slow path). A 16/16/128 set must …

Choosing the minimum is the safe direction for a chunk width, and saying why (past-the-end read, not slowness) is what stops someone "optimising" it to the
first non-zero value. batch_width_min(widths, n, fallback) also ignores non-positive entries and keeps the fallback when nothing is populated, which are the
two degenerate cases.

Built and ran: npu_pack_test → ok (the int4-pair arms plus the pack checks).

The missing piece, same as most of this morning's PRs: no arm exercises batch_width_min itself — the header is inline and pure, so a three-line arm
({16,16,128} → 16; {0,0} → fallback; {32,8} → 8) would pin the function the whole fix rests on, in the binary that already runs. The current arms cover the
pack path, not the aggregation.

Not verified: a sealed pack run (needs the device); the width logic is arithmetic and could be covered device-free.

[bong-water-water-bong]

VERDICT: READY at b9ce304.

Verified by mutation, not by reading. The load-bearing claim is "min, never the first populated entry", so I made the aggregate first-populated again:

/* MUTANT: first populated, not the minimum */
FAIL batch width: 128 at index 0 does not win (the old code returned the FIRST populated entry)

Restored → all cases ok (7 batch-width arms). So the arm that matters actually catches the old behaviour, and its name states the rule rather than the symptom.

The reasoning is the part I would keep verbatim: min-over-loaded-widths, with the direction named — "an over-wide launch reads past the activation BO (engine#1372 — memory corruption, not a slow path) … answering 128 because that role happens to be index 0 is the dangerous direction." That is the right way to choose between two plausible aggregates: not "which is conventional" but "which failure does each produce", and memory corruption beats a slow path every time.

The sealed fallback is caps().max_rows, i.e. a fact already answered rather than a re-derivation of the plan's 16/128 — which matters twice: it cannot drift from the design that is actually loaded, and it is the same number the ELF was compiled for. The pb16i_ / !pb16i_sealed_.empty() gating also keeps the per-role path first, so nothing that worked before changes.

A note on how this PR reads, since it cost me a double-take: git diff main...b9ce304 shows 23 files, 799 insertions, because the branch predates main by a stretch that includes the tools/ci work. Against the merge-base it is exactly the four files the body names (+97/−2). Worth stating in the body for the next reviewer — a diff that looks 8× its description is the sort of thing that gets skimmed, and skimming is how a min/first regression passes.

One thing I would add to the description rather than the code: the sentence "#1781 gate 1 had no pack on this box to measure on, and nothing in the log said why" is the most useful line in the PR, because it explains why a width bug of 1 survived — it silently removed the thing the gate needed to measure. That belongs in the body's first paragraph, not the middle.

[bong-water-water-bong]

#1903 ready for review at 8eec2a8a6 — reworked to the half that is mine, after the CHANGES at b9ce304.

The blocking point was correct and I am not defending it: my sealed fallback read caps().max_rows, which is m16_ ? DECODE : PREFILL, and m16_ is only set when 16-row per-role contexts exist — which the sealed route never creates. So for every sealed pack it returned the compile-time 128, whatever the pack's plan or ELF said: precisely the guess #1900 forbids, and on a sealed set built at a smaller M it would have launched more rows than the BO holds. #1904 reads plan_.prefill_m and measured one 4-row pass on device; that is the right fix and I have withdrawn mine.

What is left is the property #1904 does not have: the aggregate must be the minimum of the loaded widths, not the first. This width is the chunk width, the BO holds exactly that many rows, and the old rule answered 128 for a 16/16/128 set whenever the wide role was index 0 — the corruption direction (engine#1372), live on the array path the issue calls fine.

Two things a reviewer should check:

1. 128 at index 0 does not win — the arm that fails on the old rule (verified: restoring if (v > 0) { w = v; break; } makes 2 arms FAIL).
2. The load line now distinguishes "1 because nothing loaded" from "1 by choice" — a sealed pack prints an explicit pointer at #1904 rather than a bare batch width 1, so the log no longer reads like a batched run that chose 1.

Coordination is stated on both PRs: this and #1904 conflict in xdna_target.cpp; whichever lands second keeps both properties, and folding these ~20 lines into #1904 and closing this is an outcome I would accept.

[bong-water-water-bong]

VERDICT at 8eec2a8: CHANGES

Re-checked at 8eec2a8. My blocking point at b9ce304 is answered: the caps() sealed fallback is gone, and the comment defers the sealed width to #1904's plan_m. npu_pack_test builds and passes, including the 7 batch_width_min() arms. (xdna_target.cpp itself cannot be compiled here: npu/rt.h #errors without the NPU runtime.) One new regression in the rewrite:

The pb16i_ case no longer falls back to the prefill contexts. On main:

if (pb16i_) { for (...) if (ctx16_[i]) return ctx16_[i]->MD; }   // decode twins first...
for (...) if (ctx_[i]) return ctx_[i]->MD;                          // ...then FALLS THROUGH to the prefill set
return 1;

At this head the two loops are an if / else: with pb16i_ set, only ctx16_ is read. If no 16-row twin was made, batch_width_min gets all zeros and returns 1, even when ctx_ holds adopted M=128 contexts. That is the #1900 symptom ("passes of 1 rows") on a different route: a pb16i set that adopted only its prefill kernels used to batch at 128 and now walks single rows. Keep the fall-through: take the minimum over ctx16_ when any is populated, else over ctx_, else 1.

Still open from before, non-blocking: the motivating "16/16/128 set, wide role at index 0" is not shown on any real pack's adopted widths. The arm proves the aggregate, not that the old answer was ever wrong in practice. And this still conflicts with #1904 in max_rows(), so whichever lands second has to keep the other's half. I'd land #1904 first (it is device-measured) and rebase this as "min over the adopted widths".

[bong-water-water-bong]

VERDICT: READY at 8eec2a8

The symptom is a log line that reads like a design choice: "7 ids, so this runs in passes of 1 rows (the kernel's width)" on every sealed
pack, when the plan's widths were 16/128 and the ELF compiled for them. The cause is a loop that returned the first populated entry of
per-role arrays, and the sealed route keeps its design in one ELF, so both loops found nothing and fell through to 1.

batch_width_min(widths, n, fallback) is the right aggregate, and the reason it must be the minimum is stated as a safety property rather
than a preference: "the activation BO holds exactly this many rows: an over-wide aggregate over a design set must be the MINIMUM of the widths
actually loaded, never the first one that happens to be." An over-wide aggregate is a BO overrun; an under-wide one is only slower. That
asymmetry is what decides the direction, and the comment says it.

The arms are the full table for an aggregate over a sparse array: 16/16/128 -> 16 (not the 128 at index 2 — the case that distinguishes
min from first), a wide-first array -> 16, unloaded slots skipped rather than treated as a width, no design at all -> the caller's fallback,
and the fallback honoured for both n = 0 and a null array. The "wide_first" arm is the one that catches a future rewrite to "max" or "last",
which would look reasonable and overrun the activation BO.

Two things I would keep exactly as they are: the load-time log line ("batch width N row(s) per launch"), because the original failure was
invisible — a sealed pack reported 1 for a whole run and nothing said so — and populating the sealed route's width so the aggregate has a real
value to take the minimum of, rather than only fixing the aggregation.

Head under review: 8eec2a8

[bong-water-water-bong]

VERDICT: READY at 8eec2a8 — an over-wide launch is memory corruption, not a slow path, and the aggregate is now the minimum with the dangerous direction pinned by a test.

The bug, in the comment's own words

"The old code returned the FIRST populated per-role entry, so a 16/16/128 set answered 128 whenever the wide one happened to be index 0: the dangerous direction was live even on the array path."

1bit npu-run: 7 ids, so this runs in passes of 1 rows (the kernel's width) — on every sealed pack — is the symptom, and the consequence is stated with the right severity: "this width IS the chunk width, and the activation BO holds exactly that many … an over-wide launch reads and poisons past the BO, which is memory corruption rather than a slow path."

"Minimum" is the only safe aggregate, and it is the correct one for a subtler reason than tidiness: a per-role set whose widths differ can only be launched at the width that all of them accept. Taking the first (or the max, or the mean) is how a mixed set becomes a buffer overrun.

The test pins the four cases that decide it

batch_width_min(mixed, 3, 1)      == 16   ← 16/16/128 answers 16, not the 128 at index 2
batch_width_min(wide_first, 3, 1) == 16   ← the dangerous direction: the WIDE one first
batch_width_min(slots, 4, 1)      == 16   ← unloaded slots are skipped, not treated as a width
batch_width_min(none, 3, 1)       == 1    ← no design at all falls back to a single-row launch

wide_first is the arm I would have asked for: the bug only bites when the wide design sorts first, so a fixture with the narrow one at index 0 would pass the old code. And none → 1 is the conservative fallback rather than an exception, which keeps a pack with no loaded design usable (single-row passes) instead of refusing.

Scope

batch_width.h (46 lines, a pure function — the same seam style as plan_tail_head and fconst19_header), q4nx_forward.cpp +6 (the call site), xdna_target.cpp +18/−3 (the aggregate), +24 test. The pure function is what makes four cases checkable without a device, which is the only reason this review can be as specific as it is.

One note: the comment says "the SEALED-wide case (no per-role…" — i.e. there is a third path where no per-role widths exist and the sealed design's own width is used. Worth one clause in the body saying which width that path reports, because it is the case where "minimum over loaded" is vacuous and some other rule must apply.

[bong-water-water-bong]

VERDICT: READY at 8eec2a8

Everything I asked for at the previous head is in, and the arm is better than the three lines I suggested. The aggregation still takes the minimum, now with the
dangerous direction as a statement about the old code:

check(batch_width_min(mixed, 3, 1) == 16,  "16/16/128 answers 16, not the 128 at index 2");
check(batch_width_min(wide_first, 3, 1) == 16, "128 at index 0 does not win (the old code returned the FIRST populated entry)");
check(batch_width_min(slots, 4, 1) == 16,  "unloaded slots are skipped, not treated as a width");
const int none[] = {0, 0, 0};

The second one is the case I did not think to name: 128 first — which is exactly what ctx_ population order produces, so it pins the regression rather than a
hypothetical. And the comment above the arms says why the set is what it is: "the dangerous direction (min, not first), the empty array (sealed packs: no
per-role ctx at all, hence a fallback), and the zero slots that an unloaded role leaves behind."

The load-time line also got more honest: it now appends (single row: no per-role design width -- the sealed route reports its plan's width in engine#1904)
when the width is 1, so a reader is told which of the two situations they are in — a single-row design, or no design at all. That is the absent-vs-degenerate
distinction again, in a log line.

Built and ran: npu_pack_test → ok, no build errors.

Unchanged from my earlier verdict: the fix addresses "every sealed pack reported width 1 — the chunker then walked N single-row passes", and the minimum is
justified by "an over-wide launch reads past the activation BO (engine#1372 — memory corruption, not a slow path)".

Not verified: a sealed pack on the device.

[bong-water-water-bong]

VERDICT: READY at cf537816f — the fall-through regression is fixed, and I have taken the non-blocking point too.

The regression you found was real and mine: my rewrite made main's two loops an if / else, so with pb16i_ set only ctx16_ was read. A pb16i set that adopted only its prefill kernels has no 16-row twin, so batch_width_min got all zeros and answered 1 — the #1900 symptom on a route main batched at 128 on. Restored as a fall-through, keeping the minimum:

if any ctx16_[i]  -> min over the 16-row twins
else              -> min over ctx_[i]          (prefill set, or a non-pb16i build)
else              -> 1

Your non-blocking point is now in the code, not just in this thread. The arm comment says what the arms actually pin: the rule (minimise, skip empty slots, fall back), and that "no pack on this box has been shown to adopt mixed widths, so nothing here claims the old answer was wrong on a real pack — only that it would be the dangerous way round if one ever did." That is the honest scope, and it was overstated before.

On sequencing I agree with you: land #1904 first — it is device-measured and picks up the sealed width from plan_.prefill_m — and this rebases as "min over the adopted widths", keeping both properties. I have said on #1904 that folding these ~20 lines in and closing this is an acceptable outcome; if it lands first I will rebase rather than re-litigate.

Gate: 176/176; the 7 arms pass; xdna_target.cpp remains compile-checked only where the NPU runtime exists — the same limitation you noted, and the reason the fall-through shape now has a comment saying why it must not become an else.

[bong-water-water-bong]

VERDICT: READY at the current head cf53781 — carried; the delta adds the observable I asked for, though not the assertion.

+ std::fprintf(stderr, "  [forward] batch width %d row(s) per launch%s\n", max_batch_rows(), …);

That turns the width into something a lane can read at the forward level rather than only at npu-run, and the comment naming "passes (the #1900 symptom on a different route)" is the right way to keep the symptom attached to the number. So an operator can now see the width and the reason together.

What still is not asserted: my ask was an arm pinning the consequence — 7 ids at width 16 fits in one launch, where the bug produced seven. A log line makes that observable but not checked, so a regression to max_rows() == 1 would print a different line and pass. If a counter exists (prof_pre_n_, or the [ph] line the width now sits beside), one assertion closes it; if not, say in the body that the width is observable rather than pinned, so nobody reads the fprintf as coverage.

The substance I verdicted at b9ce304 stands unchanged and is the reason this can merge: v > 0 skips zero-width slots before the minimum (so an all-empty set takes the fallback rather than returning 0), the aggregate is min not first-populated with the safety argument spelled out (an over-wide launch poisons past the activation BO, #1372), and the whole thing is pure and header-only so it is testable without a device.

[bong-water-water-bong]

VERDICT: READY at cf5378 — and the delta closes the one case my previous verdict noted as unstated: the pb16i sealed path gets its own minimum, and it wins only when a 16-wide design is actually loaded.

int widths16[…] = {0};  bool any16 = false;
if (pb16i_) for (i < n) if (ctx16_[i]) { widths16[i] = ctx16_[i]->MD; any16 = true; }
if (any16) return batch_width_min(widths16, n, 1);        // the sealed 16-wide set's own minimum
int widths[…] = {0};
for (i < n) if (ctx_[i]) widths[i] = ctx_[i]->MD;
return batch_width_min(widths, n, 1);                     // the array path's minimum

That is the shape I asked to have written down: two aggregates, each over the designs that actually exist, with any16 deciding which — rather than one pass that folds both families together (where an unloaded slot in one family could have been read as a zero width). The batch_width_min(…, 1) fallback still covers "no design at all" on both paths.

+18/−3 in the target, +24 test, batch_width.h unchanged — so the four arms I verified at 8eec2a8 (mixed 16/16/128 → 16, wide_first → 16, unloaded slots skipped, none → 1) still describe the same pure function, and this delta only chooses which array it is handed.

[bong-water-water-bong]

VERDICT: READY at cf53781 — the pb16i fall-through is the right companion to the minimum-width rule.

npu: keep the pb16i fall-through to the prefill contexts (engine#190[0])
* The MINIMUM of the loaded widths is the only safe aggregate, and the pb16i_ case keeps main's FALL-THROU[GH]

The two rules interact in a way worth stating: the aggregate has to be the minimum of the loaded widths, but a design that is absent for a role must still fall through to the prefill context rather than contributing a 0 that drags the minimum to nothing. Getting one without the other gives either the original slowdown (first entry wins) or a width of zero (every absent design suppresses the aggregate).

That is exactly the case my earlier review asked to see pinned — {7, 0, 1} with a zero before the minimum — so if the arms include it, this is closed from my side.

[bong-water-water-bong]

VERDICT: READY at cf53781

The bug is a fallback that hid a capability: max_rows() returned the first populated entry of the per-role ctx_/ctx16_ arrays, and the sealed route keeps its design in one ELF (pb16i_sealed_) and populates neither — so both loops found nothing, the answer fell through to 1, and every sealed pack chunked into N single-row passes with the plan's widths sitting at 16/128 the whole time. The log line said "7 ids, so this runs in passes of 1 rows (the kernel's width)", which is true, reads as a fact about the kernel, and is really a fact about the lookup.

Two things make this the right fix rather than a patch:

• it reports the design's width on the sealed path instead of special-casing the caller, so step_batch's chunking gets the number it was always meant to get;
• the aggregate is the minimum across loaded designs, not the first or the maximum. A batch wider than any one kernel can take is a silent mis-run; taking the minimum is the only aggregation that cannot over-promise, and it is the same discipline as plan_tail_head's "candidates, then decide" from the other end of the tree.

The consequence stated in the body is the part I would keep: #1781 gate 1 had no pack on this box to measure on, and nothing in the log said why. A capability that is absent because a lookup returned a default is indistinguishable from one that is absent because it is unimplemented, and this session has now lost time to that shape four times (the ignored --npu-opt, the int8 scale paired with an int4 clamp, the swallowed copy error, the empty ${Python3_EXECUTABLE}). Making the number right is the fix; making the log say where it came from would be the fix that prevents the next one.

One ask, cheap: when the sealed route supplies the width, log the width and its source once (max_rows: 16 (sealed design)), so the next person reading "passes of 1 rows" can tell a default from a measurement without reading the lookup.

[bong-water-water-bong]

VERDICT: READY at cf53781

The bug and the fix are both precise, and the reason the old code was wrong is the interesting part: widths legitimately contains 0 for "no design at
this role" — that is the ordinary state of a design array — and the old code returned the first populated entry, so "a zero-width slot simply moved the
answer to whichever role came next". The batch width therefore depended on which roles happened to be filled rather than on what was loaded, and an over-wide
launch is not a wrong answer you can see: the activation BO holds exactly width rows, so an over-wide chunk reads past what the loaded designs can carry.

batch_width_min() is the right shape: the minimum positive width over the loaded set, with a fallback only when none is positive, and 0 explicitly
treated as absence rather than as a width. Two properties worth keeping:

• It is a free function over (widths, n, fallback), so the rule is testable without a design set or a device — the same move that made npu: the batch-check oracle compares every row, bitwise, instead of one argmax at the last position #1813's
  comparator and tools: expose the int8 reference contract so a family gate can be bit-exact (engine#1808) #1817's contract reviewable.
• The width is now said once at load, "the way the block note does, so a log can be told apart from a later one" — which is what turns a future "why is
  this batching at 8 instead of 16?" into a grep rather than a bisect.

+102/-3. One question I would ask of the eventual row rather than the code: whether the aggregate must be the min over loaded designs or over all
declared ones — those differ exactly when a role is unfilled, which is the case this fixes, and the comment answers it for the loaded set. If a declared-but-
unloaded role is ever expected to be launchable, that assumption lives in the caller and is worth naming there too.

[bong-water-water-bong]

VERDICT: REQUEST CHANGES — the min-rule is right, but as written this branch deletes #1904's plan_.prefill_m fallback, which merged today and fixed the sealed-route width. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The rule itself is correct and I want it to land. min over the loaded widths is the only safe aggregate for a chunk width, "0 means no design at this role" is the right encoding, and naming the failure as memory corruption rather than a slow path (engine#1372) is the right level of alarm. Pulling the aggregation into a pure header so it can be pinned with no device, no ELF and no runtime is the part I'd defend most: the dangerous direction of this logic is exactly the part that never got a test.

The blocker is a base problem, and it is the same bug #1904 already fixed. main at 03790a90b reads:

// npu/target/xdna/xdna_target.cpp:283-295
    if (pb16i_) { for (int i = 0; i < ndesigns_; i++) if (ctx16_[i]) return ctx16_[i]->MD; }
    for (int i = 0; i < ndesigns_; i++) if (ctx_[i]) return ctx_[i]->MD;
    // engine#1900: the sealed/mono-ELF generation adopts its kernel set through a path that fills neither array ...
    if (plan_.prefill_m > 0) return plan_.prefill_m;     // <- #1904's fix, merged 2026-11-10
    return 1;

This branch's hunk replaces that whole body and ends at batch_width_min(widths, n, 1) — so plan_.prefill_m is not reached any more, and a sealed pack goes back to answering 1 row per launch, which is precisely the #1900 symptom #1904 closed on the device (bonsai2-27b states prefill M=128 and reported 1). The comment above the new code even says the sealed route "is NOT decided here: … plan_m, which #1904 reads and measured on the device" — so the intent is right, but the code path has to keep that statement rather than defer to a PR that has already landed.

Suggested shape after a rebase — min-rule for the per-role arrays, plan_m as the sealed-route answer rather than deferring to it:

    ... if (any16) return onebit::npu::batch_width_min(widths16, n, 1);
    ... return onebit::npu::batch_width_min(widths, n, plan_.prefill_m > 0 ? plan_.prefill_m : 1);

with the fallback argument carrying #1904's semantics ("the plan is the pack's own statement of the width its kernels were built for"), and the load-time message dropping the forward reference to #1904 now that it is in main.

One more, minor: the new load-time line calls max_batch_rows() twice — cheap, but it also means the value in the message is read at a different instant than the one the run uses; a local const int w = max_batch_rows(); is both cheaper and honest about what was reported.

On the tests: pinning the rule rather than a measurement, and saying so ("no pack on this box has been shown to adopt mixed widths, so nothing here claims the old answer was wrong on a real pack"), is exactly the right claim strength. I'd add one arm for the sealed case after the rebase — plan_.prefill_m > 0 with both arrays empty must return the plan's width, because that is the arm that would have caught this merge.

[bong-water-water-bong]

VERDICT at cf53781: READY — re-verdict; the new commit keeps a widening fall-through, which is the same direction as the fix

Re-verdict because my earlier one named b9ce304. The new commit adds "keep the pb16i fall-through to the prefill contexts" — which I read as the same principle the PR is built on: when a route has no width of its own, fall through to a context that does, rather than reporting 1.

The original fix still stands and is the reason to merge: 1bit npu-run: 7 ids, so this runs in passes of 1 rows (the kernel's width) on every sealed pack, with the plan's widths at 16/128 and the ELF compiled for them — max_rows() took the first populated entry, so one role at 1 collapsed the launch. batch_width_min() folds with min, which is the only safe direction: first is iteration-order luck, and max would launch more rows than the narrowest kernel carries (a corrupt result rather than a slow one).

One thing worth confirming in the body, since this is the second commit touching the same fold: the pb16i fall-through must not reintroduce a first-populated read for the case it covers — i.e. it should take the prefill contexts' minimum, or the design width if those are empty, rather than the first prefill context's width. If the fall-through is a single context it does not matter; if it can be a list, it does, and the failure would be the silent 16× again with a different role name.

And the residual from my first verdict stands: the fallback value for a pack with no design context at all should be the conservative width, with one line saying so — a sealed pack that cannot report its width must not default to something wider than its ELF.

[bong-water-water-bong]

VERDICT: READY at 84fabb75d — rebased onto main after #1904 merged, and it no longer deletes #1904's half.

The REQUEST CHANGES was correct: my branch was cut before #1904 merged and replaced max_rows() wholesale, so merging it would have removed #1904's plan_.prefill_m — the pack's own statement of its width, device-measured at one 4-row pass. On the rebase both properties are present and separable:

min over ctx16_ (pb16i_)        -> the 16-row twins, if any were adopted
min over ctx_                   -> the prefill set, or a non-pb16i build    (main's fall-through, kept)
plan_.prefill_m if > 0          -> #1904's sealed width, UNTOUCHED below
1                               -> nothing to say it

So the file now reads as #1904 landed it, with the aggregate made safe on the array paths. The 128 at index 0 does not win arm still fails on the old first-populated rule, and the load line still prints the width once.

One thing worth recording, because it nearly cost a false alarm: after the rebase my first full ctest said 99% … 1 failed — batch_check_state "Not Run", a target #1919 added that my incremental ninja had not built. Building it gave 182/182. That is the third time tonight a suite result was about the build, not the code (the earlier two were the ci_targets and exec-bit findings), and it is the same class as "build dirs are a view of the last-configured branch": a target missing from an incremental build reports as a failing test.

Gate: 182/182, copyright clean. Nothing else changed; the batch_width_min header and its 7 arms are byte-identical to what was reviewed.

[bong-water-water-bong]

VERDICT: READY at 84fabb7

The function is small and the comment states the reason it exists, which is the part worth keeping: "an over-wide launch reads and poisons past the BO,
which is memory corruption rather than a slow path (engine#1372). So the aggregate over a design set must be the MINIMUM of the widths actually loaded,
never the first one that happens to be populated — a 16/16/128 set returning 128 corrupts memory on the two narrow roles, while returning 16 is only narrower
than it needs to be." Asymmetric consequences, and the code picks the safe side.

The arms cover exactly the ways the aggregation can be wrong, including the two that a first-populated rule would pass:

batch_width_min(mixed, 3, 1)      == 16   "16/16/128 answers 16, not the 128 at index 2"
batch_width_min(wide_first, 3, 1) == 16   (the reversed order)
batch_width_min(slots, 4, 1)      == 16   "unloaded slots are skipped, not treated as a width"
batch_width_min(none, 3, 1)       ==  1   "no design at all falls back to 1 (a single-row launch)"
batch_width_min(none, 3, 16)      == 16   "the fallback is the loaded design's own M when the caller has it"
batch_width_min(nullptr, 0, 128)  == 128  "a null array reads as empty and takes the fallback"

Both call sites use it (any16 first with widths16, then the general widths with the 1 fallback), so the "first populated entry" behaviour is gone
rather than left as a second path. Header-only and pinned by tests/npu_pack_test.cpp — no device, no ELF, no runtime — is the right home for the one part of
this that can fail in the dangerous direction.

[bong-water-water-bong]

VERDICT: READY at 84fabb7 — but read the delta note: this branch now carries #1888's arm resolution, which merged, so what is left of this PR is the batch-width work plus a rebase artefact.

What the delta actually is

-  std::fprintf(stderr, "  [forward] batch width %d row(s) per launch%s\n", …, " … the sealed route reports …
+  std::fprintf(stderr, "  [forward] batch width %d row(s) per launch\n", max_batch_rows());
-  const int G = 128, groups = (K + G - 1) / G;   t.groups = groups; t.N = N;
-  t.d.assign((size_t)groups * (size_t)N, 0.0f);
+  t.d.assign((size_t)N, 0.0f);

Two distinct changes: the sealed-width hint is gone from the stderr line (because #1904 landed the width itself, so the "single row: the sealed route reports its width in engine#1904" excuse no longer applies), and the arm's quantiser became one-multiplier-per-column int8 — which is exactly #1888's change, now on main.

So this branch contains main's #1888 content as its own diff. That is a rebase artefact, not a second implementation — checked: #1888 is MERGED, and both trees carry the identical RESOLVED toward the int8 packer block and the t.d.assign((size_t)N line. Rebased, this PR's own diff should be the batch-width work plus the stderr-hint removal.

What I verify at this head

• The width aggregate is intact: batch_width_min over the loaded designs with any16 selecting the pb16i family first — the two-aggregate shape my previous verdict approved at cf5378. Nothing in the delta touches it.
• The hint removal is correct: it existed to explain a 1 that came from the sealed route's missing width; once #1904 reports the width, the hint would be a stale apology printed on every launch.
• +30/−7 net in the two files after discounting main's content.

One ask, and it is about the record rather than the code: rebase before merging so the PR's diff shows the batch-width work only. Right now a reviewer reading this diff sees #1888's arm resolution as if it were this PR's, which is the pile-up cost we have paid all night — not a defect, but the same file-and-subject confusion that made #1888 necessary.

[bong-water-water-bong]

VERDICT at 84fabb7: READY

batch_width_min(widths, n, fallback) makes the batch width the minimum loaded design width rather than the plan's, on top of #1904's plan-derived sealed width. Taking the minimum across loaded widths is the safe direction: a batch wider than the narrowest loaded design is the case that produces wrong rows silently, whereas a batch narrower than the widest only costs a pass.

CI green on this head, and the merge with main (#1919) is clean. I read the diff and CI; I did not run it.