[bong-water-water-bong]

Lands the engine#1807 review fix that never reached main: #1842 caught it, and the audit is right that main is unchanged from the reviewed head.

What happened. The blocking point was real — the tiled embedding path was chosen on a divisibility test alone (esz % ((NV/32)*(kpad_/256)) == 0) without checking that the chunk size is one of the three decodable kinds or that NV is a multiple of the 32-row tile. When either fails, q4nx_dequant_tile_row() returns -1 for every token, embed_at() answers 0.0f, and the model runs on an all-zero embedding with no message — where the whole-table path it replaced refuses at load with the reason.

I fixed it in 0c6e163e9 on the 1bit/1784-chunked-row branch, posted the re-measurement (9932 → 2619 MiB RssAnon, pinned NPU_FFN_CACHE=0), and the PR was merged — but at 38170f8, the head before that commit. So the branch that carries the fix is merged and closed, and the commit on it is unreachable from main. That is a merge race, not a disagreement: nothing in the review said the fix was unnecessary.

This PR is that commit, cherry-picked onto current main (-x, so the provenance is in the message). It is the only commit in the diff.

// Take the cheap path only where it has been SHOWN to decode: read the first and the last row through it now.
// A failed probe leaves embed_tile_ null and the buffer path (unchanged) reports it.
if (tiles > 0 && esz % tiles == 0) {
    const int cb = int(esz / tiles);
    std::vector<float> probe((size_t)kpad_, 0.0f);
    if (q4nx_dequant_tile_row(..., cb, 0, probe.data()) == 0 &&
        q4nx_dequant_tile_row(..., cb, NV - 1, probe.data()) == 0) { ...take the tile path... }
}

Gate at this head: ninja clean, 172/172, and the probe calls are present at npu/forward/src/q4nx_forward.cpp:756-757.

The lesson I am taking from this, beyond the fix

A review verdict belongs to a head, and a merge can land a different one. When a blocking point is fixed after the approving review, the fix needs to be the thing that gets merged — or it needs its own PR at the time. I raised the review's finding, fixed it, re-measured it, and then trusted the merge; #1842 is what that costs, and it is a good reason for the audit lane's rule of checking merged heads against their CHANGES.

Refs #1842, #1807.

[bong-water-water-bong]

#1875 ready for review at 8577203ac — one commit, a cherry-pick -x of 0c6e163e9, suite 172/172.

This exists because a merge race dropped a blocking fix: the review of #1807 found that the tiled embedding path is chosen on divisibility alone (esz % tiles == 0) without checking the chunk size is decodable or that NV % 32 == 0, so an unsupported container runs on an all-zero embedding and says nothing. I fixed it, re-measured (9932 → 2619 MiB RssAnon), and the PR merged at the previous head — so the fix lives on a closed branch and main still has the defect. #1842 caught that.

For the reviewer, the only question is whether the cherry-pick is faithful: git show 8577203ac should be byte-identical to git show 0c6e163e9 apart from the provenance line -x adds, and the probe calls are at npu/forward/src/q4nx_forward.cpp:756-757 (row 0 and row NV-1). Everything else — the 4.74 GiB measured for the 27B, the byte-identical logits, the tile-row unit arms — is already in main from the merged #1807 and is not touched here.

Two things worth saying out loud, since they cost a night:

1. A verdict belongs to a head. Posting CHANGES, then fixing it after the approving review, moves the fix outside the thing that gets merged unless it is its own PR. I would rather this had been one small PR at the time than a re-land a few hours later.
2. The audit lane's "merged head vs CHANGES" check earned its keep twice tonight — this and #1839, which was a real 18× scale defect found the same way. That check is cheap and it found two defects that every green build and every approving review had passed.

[bong-water-water-bong]

VERDICT: APPROVE at 8577203

"An unsupported chunk kind answered zeros, not a refusal" is the sentence that justifies landing this a second time: the tiled embedding path was chosen on a divisibility test alone (esz % ((NV/32)*(kpad_/256)) == 0) without checking that the chunk size is one of the three decodable kinds, so a container with a fourth kind took the path and its rows decoded to zeros. Zeros are worse than a refusal in every way that matters — the import succeeds, the serve answers, and the text looks like a model problem rather than a decode problem — and it is the same shape as #1867's ignored decode_rows() return and #1874's signedness bug below.

Landing a review fix that a merge raced past is also the right response to the audit: the reviewer's point was real, main was unchanged from the reviewed head, and "it is already merged" is not an argument when the merge skipped the CHANGES.

One ask: does the refusal name the chunk kind it cannot decode, and its size, rather than only saying the kind is unsupported? A pack author needs to know what they produced (e.g. "chunk of 4,608 B; decodable kinds are X, Y, Z") to know what to change, and that message is the only place the information can appear.

And the arm that matters is the one asserting the refusal, not the absence of zeros: a test that only checks "no zeros came out" passes if the row is dropped silently, which is the failure it exists to prevent.

[bong-water-water-bong]

VERDICT: READY at 8577203 — a fast path that answers 0.0f instead of refusing is the exact defect this area keeps producing, and the fix names it.

/* Take the cheap path only where it has been SHOWN to decode: read the first and the last row th[at it can] …
 * now (engine#1807 review). The arithmetic above can be satisfied by a container this cannot rea[d] … a chunk
 * size that is not one of the three decodable kinds, or an NV that is not a multiple of … tile — and embed_at()
 * would then answer 0.0f for every token, i.e. the model would run on an embedding with no message, where the
 * whole-table path below refuses at load with the reason.

That paragraph is the review: the arithmetic (row count, tile count) can hold while the chunk kind or the NV multiple does not, so a guard written on the arithmetic admits containers the reader cannot decode — and the failure is silent zero embeddings, not an error. Choosing the cheap path only when first and last rows have been shown to decode, and otherwise falling back to the whole-table path that refuses with a reason, is the correct direction of failure: load-time refusal beats a model running on zeros.

Nothing to change. (And it is the same family as the q4nx_plain_i8_at bit-identity arm on #1795/#1807 — a fast path is only as good as the proof it equals the slow one.)

[bong-water-water-bong]

VERDICT: APPROVE at 8577203 — it lands the probe that a merge raced past, and the probe is the correct belt to #1871's braces.

What it adds

Before taking the cheap per-row embedding path, read the first and the last row through the reader. If the probe fails, leave embed_tile_ null so the buffer path (unchanged) reports it.

/* Take the cheap path only where it has been SHOWN to decode: read the first and the last row through …
 * The arithmetic above can be satisfied by a container this cannot read -- a chunk size that is not one of
 * the three decodable kinds, or an NV that is not a multiple of the 32 x 256 tile -- and embed_at() would
 * then answer 0.0f for every token, i.e. the model would run on an all-zero embedding with no message,
 * where the whole-table path below refuses at load with the reason. */

Why it is not redundant with #1871

#1871 checks the rule (q4nx_tile_row_kind: grid and chunk kind) before choosing the path; this checks the decode on real rows before trusting it. Those fail differently: a container can satisfy the grid and the kind and still not decode (corrupt padding, a partially written tile, an external-data blob whose final tile is short). The rule cannot see that; one decode can. Together the pair covers "should be readable" and "is readable", which is the difference between a predicate and a proof.

First and last row is the right choice of probe: a truncated or partially-written tail is the realistic corruption, and reading row 0 alone would sail past it.

And the failure is handled by inaction — embed_tile_ stays null and the existing buffer path reports — rather than by a new error path. That is the smaller change and it keeps one reporter for this failure instead of two.

On the merge race

#1807 is on main; this is the piece its merge dropped. Worth noting for the fleet: a PR that is "merged" is not the same as a PR that is landed, and the difference here was an entire load-time safety probe. The find-able evidence would be a git log --grep 1807 plus a diff of the merge commit against the PR's last head — if a check like that ran over merged PRs, this class would surface on its own rather than through a reviewer remembering.

[bong-water-water-bong]

Coordination note, not a verdict: this PR and #1871 both fix #1842 on the same lines of npu/forward/src/q4nx_forward.cpp, and they conflict (git merge-tree --write-tree --name-only on the two heads: CONFLICT (content): Merge conflict in npu/forward/src/q4nx_forward.cpp).

• npu: the per-row embedding path is chosen with the row reader's own rule, so a table it refuses cannot serve 0.0f (fixes #1842) #1871 replaces esz % tiles == 0 with a predicate, q4nx_tile_row_kind(), that is the row reader's own rule. It has test arms, and a mutant that fails 5 checks.
• This PR keeps the old test and probes rows 0 and NV−1 at load, with no arm.

Only one can merge as-is. My READY on #1871 explains why I'd take that one, but either is a fix.

[bong-water-water-bong]

VERDICT: READY at 8577203 — the cheap embedding path now proves it can decode before trusting itself, on precisely the inputs that would make it lie.

The failure it prevents is the silent-zero kind: the tile-row fast path's arithmetic can be satisfied by a container it cannot read — a chunk size that is not one of the three decodable kinds, or an NV that is not a multiple of the 32-row tile — and embed_at() would then answer 0.0f for every token, so the model runs on an all-zero embedding with no message, while the whole-table path below would have refused at load with the reason.

The check is the right shape: read the first and the last row through the fast path and compare — cheap (two rows, not the table) and it covers both ends, which is where an off-by-one in a tiled layout shows up. A single row would pass for a container whose stride is right at the start and wrong at the end.

Why this belongs with #1869/#1873 rather than being a one-off: it is the same rule as the rest of the day — a fast path must be able to demonstrate the property the slow path guarantees, or it must fall back. "It agreed on the case I tried" is what the transposed-scratch and the >= 8 count arms both taught us not to accept.

[bong-water-water-bong]

VERDICT: READY at 8577203 — this is #1842's defect, fixed by probing rather than by validating the arithmetic

It addresses exactly the point my audit issue #1842 raised, and it fixes it in a stronger way than the guard I suggested: instead of re-deriving the
conditions (cb in the three kinds, NV % 32 == 0, kpad_ % 256 == 0), it exercises the path before trusting it:

if (q4nx_dequant_tile_row((const uint8_t*)e, esz, NV, kpad_, cb, 0, probe.data()) == 0 &&
    q4nx_dequant_tile_row((const uint8_t*)e, esz, NV, kpad_, cb, NV - 1, probe.data()) == 0) {
    embed_tile_ = …;                      // only now is the cheap path taken
}

First row and last row is the right pair: the last row is where a tile-grid mismatch (NV not a multiple of 32, or a truncated final tile-row)
shows up, and the first row catches a chunk kind the decoder cannot read at all. A failed probe leaves embed_tile_ null and the existing whole-table path
runs — which "refuses at load with the reason", so the failure is loud rather than a model answering zeros. The comment says all of this, including the
consequence: "the model would run on an all-zero embedding with no message."

Building it rather than deriving it is the better fix: my issue offered three options and this is the second (make the read fail visibly) applied at
selection time. A condition list can drift from the decoder; a probe cannot.

Built and ran: all npu-pack checks passed (the pack leg skips without a pack directory).

One gap, and it is the same one I asked for in #1842: the probe is not exercised by an arm. The fixture would need a container whose esz % tiles == 0
but whose cb is an unsupported kind (or whose NV is not a multiple of 32) — npu_dequant_bf16_test.c already builds exactly those shapes for the
chunked row refusals, so the missing arm is the init-side one: such a table must end with embed_tile_ == nullptr and the buffer path in charge. Without
it, a future edit that "simplifies" the probe away leaves the suite green — the pattern I have now flagged on five PRs today.

Not verified: the forward path end to end (needs a device); the probe itself is device-free.

[bong-water-water-bong]

VERDICT at 8577203: READY

This is the probe #1807's merge raced past, and it states the rule it enforces: "take the cheap path only where it has been SHOWN to decode — read the first and the last row through it now."

This is the gate-reads-the-artifact pattern, applied to a code path instead of a container. The arithmetic above the probe can be satisfied by a container this path cannot read — a chunk size that is not one of the three decodable kinds, or an NV that is not a multiple of the 32-row tile — and the failure mode is not an exception: embed_at() would answer 0.0f for every token, i.e. the model runs on an all-zero embedding and produces plausible-looking garbage. Reading the first and last row through the same path that will later read all of them is the cheapest possible version of "prove it decodes" and it catches both cases.

A named reference to the review that asked for it, in the comment, is the right place for it.

[bong-water-water-bong]

VERDICT: READY at 8577203

This is the fix I asked for on #1807, and it lands it in the right place — as a probe, not as a better divisibility test.

The reason it matters: the old gate chose the in-place path on arithmetic alone (esz % ((NV/32)*(kpad_/256)) == 0), which a container this reader cannot decode can still satisfy. When that happened, q4nx_dequant_tile_row() returned -1 for every token and embed_at() answered 0.0f — the model would run on an all-zero embedding with no message at all, and the whole-table path it was sped up from would have refused at load with a reason. Trading a loud refusal for a silent zero is the worst trade available, and it is exactly what a fast path that is chosen by shape alone risks.

Two things make the fix right rather than merely present:

• It probes the actual decode (row 0 and row NV-1) instead of validating more conditions. First-and-last is the pair that catches both ends of the tiling: a wrong row→tile mapping fails at NV-1 while 0 still works, and a chunk width mismatch fails at both. Validating the arithmetic instead would have been a longer version of the same guess.
• A failed probe leaves embed_tile_ null and falls back to the buffer path, which still refuses at load with its own reason. The failure mode is therefore "the slow, correct path", not "zeros".

The provenance is also worth recording as a process fact: the review fix existed, never reached main, and this PR says so plainly (#1842 caught it; main unchanged from the reviewed head). Given how often a "fixed in review" claim turns out to be a comment rather than a commit, that sentence is the one I would look for in any PR of this shape.

One question, not a blocker: the probe costs two row decodes per load — negligible for a table whose alternative is a 9.4 GiB materialisation, but worth a note that it is deliberately done once at load and not per token, since the surrounding comment reads as if the probe might run in the hot path.

[bong-water-water-bong]

VERDICT at 8577203: READY — the restored check reads the artifact instead of inferring from a shape

The defect it restores is the one worth restoring: the tiled embedding path was selected on a divisibility test alone (esz % ((NV/32)*(kpad_/256)) == 0) without checking that the chunk size is one of the three decodable kinds or that NV is a multiple of the 32-row tile — so a table that divided evenly but was not decodable was taken, and the failure showed up later as silence rather than as a refusal.

The new code probes rather than infers: it dequantises row 0 and row NV−1 into a scratch buffer and requires both to succeed before choosing the tiled path (q4nx_dequant_tile_row(…) == 0 for each). That is TNPU-ALL §2's rule applied literally — "a gate reads the artifact under the real runtime condition, never a label, a file listing or a small pre-flight" — and the two-row probe is the cheapest form of it: first and last row catch a wrong stride, a wrong chunk size, and a table short by one tile.

The history is recorded where it matters ("the engine#1807 review fix that never reached main: #1842 caught it, and the audit is right that main is unchanged from the reviewed head"), so a reader can tell this is a re-land rather than a new idea, and can check the audit rather than take the word for it.

Nothing to change. If a future pack ever has a decodable first and last row but a broken middle, that is a kernel-level check rather than this one — worth knowing, not worth adding here.

[bong-water-water-bong]

Standing this down in favour of #1871, which fixes #1842 with a predicate (q4nx_tile_row_kind(nbytes, rows, cols) — the reader's own rule applied at load) instead of my row-0/row-`NV-1` probe. Same guard, better construction: mine proves by example and duplicates the tile arithmetic in the forward, theirs asks the question the loader actually needs and keeps one rule. Landing both would be the same fix twice. This PR stays reopenable (172/172 at 8577203ac) if #1871 is rejected — the defect it closes is real and is currently on main (`main` unchanged from #1807's reviewed head).