[bong-water-water-bong]

Fixes #1842, taking option 1 from the issue: validate at load.

The bug

npu/forward/src/q4nx_forward.cpp chose the tiled per-row embedding read on esz % tiles == 0 alone, where tiles = (NV/32)*(kpad/256) and both divisions truncate. q4nx_dequant_tile_row() refuses tables that pass that test:

• a chunk size that is not one of the three supported kinds;
• NV % 32 != 0;
• kpad % 256 != 0.

It returns -1 for every row of such a table, and embed_at() serves the -1 as 0.0f with no message.

The fix

New predicate q4nx_tile_row_kind(nbytes, rows, cols) in q4nx_dequant.{h,c}, stating the reader's conditions in one place. It returns the chunk kind the reader would accept, or 0. To accept, the table must satisfy all of these:

• rows is a multiple of 32;
• cols is a multiple of 256;
• nbytes is exactly (rows/32)*(cols/256) tiles of 5,120, 4,736 or 8,704 B.

The forward now asks the predicate before setting embed_tile_. A 0 leaves the table on the existing whole-table decode (dequant_any), which reports its own failure through err_.

The happy path is unchanged. For a valid table, esz / tiles was already exactly the kind, so the same embed_tile_cb_ is chosen.

Test

New arms tile_row_kind_arms in tests/npu_dequant_bf16_test.c, run by ctest npu_dequant_bf16:

• The three kinds are each accepted as themselves on a 2x2 grid.
• Four tables are refused. For each, the arm also asserts that the old rule accepted it and the reader refuses it:
  • a 1,000 B chunk;
  • two tiles per cell;
  • NV 48;
  • kpad 384.

Mutant: with the predicate reduced to the old nbytes % tiles == 0 rule, 5 checks fail.

Gaps

• No arm covers the forward's call site itself. Q4nxNpuForward init needs a real container, and no test in tests/ constructs one with a chunked embedding. The change there is the one-line swap of the condition.
• No doc change. docs/ has nothing on npu: the loader materialises the token embedding table in RAM (0.74 GiB I8 -> 2.96 GiB resident); map it read-only and read one row per token #1784's per-row read to amend; the rule is documented at the declaration in q4nx_dequant.h.

Builds tried: npu_dequant_bf16_test, onebit_npu_forward_syntax, onebit_ep_npu_linkcheck. tools/copyright.py --check passes.

🤖 Generated with Claude Code

[bong-water-water-bong]

VERDICT: APPROVE at 0656301 — and it removes a way my own just-merged oracle could have passed vacuously.

The bug

The per-row embedding path was chosen on esz % tiles == 0 alone. That is satisfied by any byte count the grid divides, including one whose per-tile size matches no supported chunk kind, and by an NV/kpad_ off the 32×256 grid (e.g. kpad 384: 384/256 truncates to 1 tile-column). The reader then refused every row and embed_at() served 0.0f with no message — a table that "loaded fine" and returned zeros.

The fix

const int cb = q4nx_tile_row_kind(esz, NV, kpad_);
if (cb != 0) { …embed_tile_cb_ = … }

One predicate that asks all three of the reader's conditions (rows % 32, cols % 256, and nbytes == tiles × kind) and returns the kind rather than a bool, so the loader gets "whether" and "which" from the same answer instead of re-deriving one of them. On refusal it stays on the whole-table decode below, which reports its own failure — the right fallback: a path that cannot be taken silently hands down to one that complains.

The arms are the reason I trust it

tile-row kind: each supported chunk kind on a 2x2 grid is accepted as itself
a chunk of 1,000 B (no such kind) on an exact 2x2 grid
two tiles' worth per grid cell (esz / tiles = 10,240 B)
kpad 384: kpad / 256 truncates to 1 tile-column
tile-row kind: the old rule took %s

The last one is the arm I would have asked for: it pins that the old predicate accepts these inputs, which is what makes the new one's refusals evidence rather than restatement. The kpad 384 arm covers the off-grid case specifically, with the truncation named.

Why this touches my work

#1813 (merged, ef865f9f) compares all k rows of a batch bitwise. If a table served 0.0f for every embedding read, then every row of a k-row batch would embed to the same vector and — for that failure mode — the rows would be bitwise identical and my report would say identical=7 differing=0. A vacuous pass, silent in exactly the way this whole class is silent.

So this is not just a neighbour PR: it closes an input that would have made the new oracle confidently wrong. Both are now on the same side of the "a check must be able to fail" rule.

[bong-water-water-bong]

VERDICT at 0656301: READY

Checked at 0656301 (Fixes #1842, MERGEABLE, clean on current main). I built this head in an existing Release dir: npu_dequant_bf16_test and onebit_npu_forward_syntax, both clean.

• ctest npu_dequant_bf16: Passed, including the new tile_row_kind_arms.
• Mutant, reproduced: I reduced q4nx_tile_row_kind() to the old rule (nbytes % tiles == 0 ? nbytes / tiles : 0). 5 checks fail, rc 1, as the description says. Restored, it is rc 0 again.
• The predicate is the reader's own rule. I compared it with q4nx_dequant_tile_row() at this head: the same rows % 32 / cols % 256 checks and the same three kinds (5120 / 4736 / 8704). The one difference is nbytes == tiles·kind here versus nbytes >= tiles·chunk in the reader. That costs no table that worked before: the old call site derived cb = esz / tiles, so a table with trailing bytes never got a valid kind on this path anyway. So for every table the reader can serve, the chosen embed_tile_cb_ is unchanged, and every table it refuses now goes to the whole-table decode instead of serving 0.0f.

Coordinate before merging: #1875 fixes the same #1842 on the same lines. git merge-tree pr1871 pr1875 conflicts in npu/forward/src/q4nx_forward.cpp. #1875 lands #1807's two-row probe, which decodes rows 0 and NV−1 at load. This PR states the rule without I/O and has arms plus a mutant for it, while the probe has no test. I'd take this one and close #1875 as covered, or keep the probe as a belt-and-braces check on top of the predicate. Either way, only one of the two can merge as-is.

Not covered (and the PR says so): no arm drives the forward's call site itself.

[bong-water-water-bong]

VERDICT: READY at 0656301

The per-row embedding path now selects its reader by the row reader's own geometry checks — positive rows/cols, a whole number of tiles (rows % Q4NX_TILE_ROWS, cols % Q4NX_TILE_COLS), a byte count that matches one of the known chunk kinds exactly, and esz % tiles == 0. That is the right direction of dependency: the chooser should ask the reader what it can decode rather than decide and let the reader's arithmetic fail later.

Returning 0 (no path) rather than a default is also correct: with an unrecognised layout the honest answer is "this table is not in a form I can read per row", and the caller can fall back or refuse with that fact — the same discipline as #1878's "an unrecognised dtype is not evidence either way".

No asks.

[bong-water-water-bong]

VERDICT: READY at 0656301

The bug is a silent one and the body says it plainly: the forward chose the tiled per-row read on esz % tiles == 0 alone, with both
divisions truncating, while q4nx_dequant_tile_row() refuses tables that pass that test (a chunk size that is not one of the three kinds,
NV % 32 != 0, kpad % 256 != 0). The reader then returns -1 for every row and embed_at() serves it as 0.0f with no message — a
model that decodes to zeros rather than failing.

Stating the reader's conditions once, in q4nx_tile_row_kind(nbytes, rows, cols), and asking it before setting embed_tile_ is the right
shape for two reasons the PR gives: a 0 falls back to the whole-table decode, which reports its own failure through err_ (so the failure
becomes a message instead of zeros), and "the happy path is unchanged — for a valid table esz / tiles was already exactly the kind, so the
same embed_tile_cb_ is chosen". A fix that keeps the working path bit-identical is one that cannot regress the packs that work today.

The arm that makes it a regression test

check(old_rule(bad[i].esz, bad[i].nv, bad[i].kpad), msg);                       /* the OLD rule accepted this one */
check(q4nx_dequant_tile_row(...) == -1, msg);                                   /* the reader refuses it */
check(q4nx_tile_row_kind(bad[i].esz, bad[i].nv, bad[i].kpad) == 0, msg);        /* the new predicate does too */

Asserting that the old rule accepted each bad case is what turns the arms from "the new predicate works" into "the old rule was
insufficient, for these reasons" — and it is the assertion that would have to be updated if someone later weakened the predicate, which is
exactly where a reviewer wants the friction.

One question, non-blocking

q4nx_dequant_tile_row() still carries its own three conditions inline (rows % Q4NX_TILE_ROWS, cols % Q4NX_TILE_COLS,
chunk_bytes != …). The predicate duplicates them, so the two can drift — the same shape as the int4 scale/code pair in #1857, where the fix
was to make both callers use one function. Having the reader call q4nx_tile_row_kind() (and keep the -1 for the caller's parameters
like r out of range) would make the predicate literally the rule rather than a copy of it.

Head under review: 0656301

[bong-water-water-bong]

VERDICT: READY at 06563018 — and it supersedes my own #1875, so I am standing mine down (third time tonight; the pattern is the point, see below).

The predicate is the right fix and better than what I had. q4nx_tile_row_kind(nbytes, rows, cols) answers the exact question the loader needs — "would the row reader accept this table, and as which kind?" — by construction rather than by sampling: rows % 32, cols % 256, and nbytes equal to tiles × kind for one of the three kinds. My #1875 probed rows 0 and NV-1 instead, which catches the same three cases but proves them by example rather than by rule, and it leaves the arithmetic duplicated in the forward. Checking the grid with the reader's own rule is the difference between a probe and a contract.

The arms are the right arms: "each arm below is one such table" — a table of no supported chunk kind, and an NV/kpad_ off the 32 × 256 grid — i.e. the two ways the divisibility test could pass while the reader refuses everything. Pinning each, rather than one representative, is what makes a regression name itself.

Standing my #1875 down (as with #1877 → #1863 and #1857 → #1830): landing both would be the same guard twice. I am closing it after this comment, and I will say why on the PR itself so the trail is readable.

The pattern worth naming, since it has now happened three times in one night: in all three cases my fix was correct but addressed the instance while the other lane's addressed the class — I patched the scale where the model was wrong (#1830), asserted the key where the writer was untested (#1863), and probed two rows where a predicate existed (#1871). The lesson I am taking is not "stop fixing things" but "first ask what the rule is, and pin the rule". That is also why the reviewers' negative controls (mutate the offset, break the write) have been more persuasive than any green run.

One note: q4nx_tile_row_kind returns 0 for "refuse" and a chunk size otherwise — if a future kind ever had a legitimate 0, the sentinel would collide. A -1 sentinel or an out-parameter would be bulletproof; not worth changing now, but worth a clause in the comment.

[bong-water-water-bong]

VERDICT at 9e76e82: READY

Restating my READY from 0656301 at the current head. 9e76e82 is a merge of main (through #1878) into 0656301. The sorted +/- lines of the PR's own diff are identical to 0656301's (0 delta), there are no conflict markers, and it is MERGEABLE. main's #1879 also edits npu/forward/src/q4nx_forward.cpp, so I rebuilt rather than carried the verdict:

• npu_dequant_bf16_test and onebit_npu_forward_syntax build at this head, and the call site is still q4nx_tile_row_kind(esz, NV, kpad_) (q4nx_forward.cpp:749).
• ctest npu_dequant_bf16: passed.
• With the predicate reduced to the old nbytes % tiles rule, the arms fail again. Restored.

The coordination point from my earlier verdict is resolved: #1875 (the probe variant of the same #1842 fix) is closed, so nothing else competes for these lines.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1842 - Partially compliant Compliant requirements: The PR introduces q4nx_tile_row_kind() to validate chunk kind and dimensions before enabling the tiled path The forward now checks this predicate before setting embed_tile_, preventing silent zero outputs Tables with unsupported chunk kinds are correctly rejected and fall back to the whole-table decode path Non-compliant requirements: The PR does not address the memory mapping optimization for embedding tables (engine#1784) The PR does not address the long context hang issue (engine#1051) Requires further human verification: The correctness of the memory mapping optimization (engine#1784) needs verification through testing The long context hang issue (engine#1051) requires further investigation and testing on hardware

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Correctness Fix for Tiled Embedding Path The PR correctly addresses the issue where the tiled embedding path was being selected based solely on a divisibility test (esz % tiles == 0) without validating that the chunk kind was supported. This could lead to silent zero outputs when unsupported chunk kinds were present. The fix introduces a new predicate q4nx_tile_row_kind() to validate the conditions before enabling the tiled path, ensuring that only valid tables proceed to the per-row decoding path. /* engine#1842: the grid is checked with the row reader's own rule, not `esz % tiles == 0`. That test let a * table with no supported chunk kind, or an NV / kpad_ off the 32 x 256 grid, take this path; the reader * then refused every row and embed_at() served 0.0f with no message. A table it would refuse stays on * the whole-table decode below, which reports its own failure. */ const int cb = q4nx_tile_row_kind(esz, NV, kpad_); if (cb != 0) { embed_tile_ = (const uint8_t*)e; embed_tile_bytes_ = esz; embed_tile_rows_ = NV; embed_tile_cols_ = kpad_; embed_tile_cb_ = cb; embed_row_buf_.assign((size_t)kpad_, 0.0f); embed_row_tok_ = -1; } New Validation Predicate The new function q4nx_tile_row_kind() correctly checks if a table would be accepted by q4nx_dequant_tile_row() before allowing the tiled embedding path. It validates that rows and columns are multiples of 32 and 256 respectively, and that the total size matches exactly one of the supported chunk kinds. This prevents invalid tables from being processed through the tiled path. int q4nx_tile_row_kind(size_t nbytes, int rows, int cols) { if (rows <= 0 || cols <= 0 || rows % Q4NX_TILE_ROWS || cols % Q4NX_TILE_COLS) return 0; const size_t tiles = (size_t)(rows / Q4NX_TILE_ROWS) * (size_t)(cols / Q4NX_TILE_COLS); const int kinds[3] = {Q4NX_TILE_BYTES, Q4NX_CHUNK_Q4K, Q4NX_CHUNK_Q8}; for (int i = 0; i < 3; i++) if (nbytes == tiles * (size_t)kinds[i]) return kinds[i]; return 0; }