Skip to content

npu: read the tile-packed embedding table one row per token, so the 27B stops materialising 4.74 GiB (engine#1784) - #1807

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/1784-chunked-row
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/1784-chunked-row

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Second half of engine#1784 (owner item F4): the tile-packed embedding table is read one row per token too, so the 27B stops materialising 4.74 GiB.

The pack is not the dtype its header says

The merged first half (#1795) left one question open: every I8-embedding container here has data_size == NV*H in the header, yet none of them took the plain-int8 branch. A temporary print in the init answered it:

[F4DEBUG] etd=model.embed_tokens.weight dtype=I8 data_size=794624000 en=1271398400 NV=248320 H=5120 kpad_=5120 equal=0

NV*H = 1,271,398,400 but the tensor holds 794,624,000 bytes — and 794,624,000 / 5,120 = 155,200, exactly the row count in the header's shape. The same bytes fit two readings: 1 byte per element for 155,200×5,120, or 155,200 tiles of 5,120 B = 0.625 B per element for the 248,320×5,120 the runtime uses — i.e. a packed 4-bit table. dequant_any correctly takes the chunked branch, and decoding it whole costs 4.74 GiB of anonymous memory for a 0.74 GiB file (6.4×) on a table read one row per token.

The change

q4nx_dequant_tile_row() (public tree, declared beside q4nx_dequant_tensor_chunked): row r lives as row r % 32 inside each of cols/256 tiles, so it reads those 20 tiles (~100 KB) and nothing else. embed_at() gathers the row once per token into a kpad_-wide buffer and serves the H reads from it — the access the callers already had (k innermost), with a buffer holding one row instead of the table.

Measured on the device route

npu-hold, the pack's own full_i8_*.elf kernels, one 9-minute ticket per arm (~/wt/zc-probes/f4-dev-*, harness /tmp/f4dev.sh, both gpu-hold tickets released on exit):

RssAnon after load after one prompt step-1 logits
before (materialised) 9773 MiB 9947 MiB c93ff21631b818ae26e93583cb9f2ca8
after (one row per token) 2658 MiB 2678 MiB c93ff21631b818ae26e93583cb9f2ca8

7.15 GiB of private memory returned, and the first generated position's vocabulary-wide float32 logits are byte-identical (cmp) between the two binaries. That is the strongest equality available on this pack: the logits fold in the embedding row and the tied head at once. The text reply is empty on both arms — a property of this pack on this route, not of the change — so the gate is the logits, not the transcript, and I am not claiming a text identity I did not observe.

Test

Three chunk kinds (5,120 B tiles, Q4K, Q8) × all 64 rows: a gathered row equals the whole-table decode bit for bit; plus row-out-of-range, one byte short, a non-tile-multiple rows, and a null table refused. Host only, no device, no model. Suite 168/168.

Refs #1784.

…he 27B stops materialising 4.74 GiB (engine#1784)

The bf16 and plain-int8 tables already read in place (#1795). The containers that looked like plain int8 are not:
a temporary print in the init showed `NV=248320 H=5120` against a tensor of 794,624,000 bytes, i.e. the file holds
155,200 tiles of 5,120 B = **0.625 bytes per element**, a packed 4-bit table that `dequant_any` correctly sends down the
chunked branch. Decoded whole, it is 1,271,398,400 floats = **4.74 GiB of anonymous memory for a 0.74 GiB file**, on a
table that is read one row per token.

    [F4DEBUG] etd=model.embed_tokens.weight dtype=I8 data_size=794624000 en=1271398400 NV=248320 H=5120 kpad_=5120 equal=0

`q4nx_dequant_tile_row()` (public tree, `npu/forward/src/q4nx_dequant.c`, declared beside
`q4nx_dequant_tensor_chunked`) decodes one row: row `r` lives as row `r % 32` inside each of `cols/256` tiles, so it
reads those 20 tiles (~100 KB) and nothing else. Same grid and same tile ordering as the whole-table decoder, which the
test pins. `embed_at()` gathers the row once per token into a `kpad_`-wide buffer and serves the H reads from it, which
is the access the callers already had (`k` innermost); the buffer holds one row, not the table.

**Measured on the device route** (`npu-hold`, the pack's own `full_i8_*.elf` kernels, one 9-minute ticket per arm):

| | RssAnon after load | after one prompt | logits |
|---|---|---|---|
| before (materialised) | 9773 MiB | 9947 MiB | `c93ff216…` |
| after (row per token) | **2658 MiB** | **2678 MiB** | `c93ff216…` |

**7.15 GiB of private memory returned**, and the first generated position's vocabulary-wide float32 logits are
**byte-identical** between the two binaries (`cmp`, same md5) -- the strongest equality available here, since it folds in
the embedding row and the tied head at once. The pack's text reply is empty on both arms (a property of this pack on this
route, not of the change), which is why the gate is the logits rather than the transcript.

Test: for all three chunk kinds (5,120 B tiles, Q4K, Q8) and every one of 64 rows, a gathered row equals the whole-table
decode **bit for bit**; plus row-out-of-range, one byte short, a non-tile-multiple `rows`, and a null table. Host only, no
device. Suite 168/168.

Refs #1784. Supersedes nothing: #1795 stays as the bf16/plain-int8 half.

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

#1807 ready for review at 38170f854 — 5 files, +127/−6, suite 168/168, MERGEABLE.

Three things a reviewer should lean on, in order:

  1. The identity gate is the logits, not the transcript, and I say so because the transcript is empty. Both arms of the 27B on the device route return '' — a property of this pack on this route — so quoting "identical output" from the text would have been a comparison of two empty strings. The first generated position's vocabulary-wide float32 logits are byte-identical instead (cmp, md5 c93ff216…), which folds in the embedding row and the tied head, so one exact match covers both readers of the table.
  2. The row decode is pinned against the whole-table decoder for every row and all three chunk kinds (64 rows × 3), not against hand-written values — because the failure mode is a tile-index off-by-one that reads a neighbouring tile and still produces plausible numbers.
  3. The RSS figures are RssAnon (private), taken on the device route under npu-hold with both tickets released: 9773 → 2658 MiB after load, i.e. 7.15 GiB, which is the 4.74 GiB table plus the 1.48 GiB buffer the old code allocated and never used for this dtype. The repro is two gpu-hold lines in the body.

One thing worth a second pair of eyes: embed_tile_cb_ is derived as esz / ((NV/32)*(kpad_/256)) and accepted only when that divides exactly, with the whole-table buffer kept as the fallback otherwise. That is deliberate — an unexpected grid keeps the old behaviour instead of guessing — but it means a future pack with a different tile count silently loses the win. If that ever happens the log line says "dequantized (… GiB)" where this one says "read in place", so it is visible rather than silent.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 38170f8 — the strongest evidence this item has produced, and a real 7 GiB.

The diagnosis is the valuable part

The header says data_size == NV*H for every I8-embedding container, yet none took the plain-int8 branch — and the reason is
arithmetic, not a flag: 794,624,000 / 5,120 = 155,200 rows, i.e. 0.625 B/element over the 248,320×5,120 the runtime uses, which
is a packed 4-bit table wearing an I8 header. Printing the debug line that distinguishes "header lies" from "branch is wrong" is
exactly how that class of question should be closed.

q4nx_dequant_tile_row() following the chunked layout (row r % 32 inside each of cols/256 tiles, ~100 KB read per row) plus a
one-row gather in embed_at() is the minimum change that keeps the callers' existing k-innermost access.

The measurement, and the arithmetic a reader will try to do

RssAnon after load after a prompt step-1 logits
before 9773 MiB 9947 MiB c93ff22…
after 2658 MiB 2678 MiB c93ff22…

Byte-identical first-position logits across a 7 GiB memory change is the strongest equality this pack allows — the logits fold
in the embedding row and the tied head, so an off-by-one in the tile-row arithmetic or a wrong scale cannot survive it. And the
refusal to claim text identity ("the reply is empty on both arms, a property of this pack on this route, so the gate is the
logits") is the right call: a claim you cannot make is better named than approximated.

One note on the numbers, because a reader will try to reconcile them: the 4.74 GiB named in the diagnosis does not equal the
6.95 GiB returned (9773 − 2658 MiB). It does add up if the before arm held two materialisations — the bf16 copy (2.37 GiB)
and the f32 decode (4.74 GiB) = 7.10 GiB of buffer — which is consistent with #1795 leaving embed_ unsized-to-zero and embed_f_
built for the chunked formats. Worth one clause in the body, because otherwise the headline number looks larger than its own
mechanism, and "the saving is bigger than the thing I removed" is the shape of a measurement that moved for another reason.

The test is the right coverage

Three chunk kinds (5,120 B tiles, Q4K, Q8) × all 64 rows, gathered row vs whole-table decode bit for bit — that is what makes
the tile-row arithmetic verifiable rather than asserted — plus out-of-range, one-byte-short, non-multiple rows, and a null table
refused. Host-only, so it runs everywhere the suite does.

Head under review: 38170f8

@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 38170f8: READY

The criterion is met in the form the plan asks for: a bit-exact result against a NAMED reference, with the two places an off-by-one can hide as arms.

+/* engine#1784: one row of a CHUNKED (tiled) table, decoded without decoding the table …
+ * The row gather must reproduce exactly what q4nx_dequant_tensor_chunked() puts in that row -- all three chunk kinds,
+ * every row including the last one of a tile-row and the last row of the tensor -- because the tiles are ordered
+ * (row-tile * ntiles_across + col-tile) and an off-by-one there reads a neighbouring tile …

That comment is the review: it names the reference (q4nx_dequant_tensor_chunked()), states that all three chunk kinds must match, and calls out the two boundary rows where the
tile arithmetic changes — the last row of a tile-row and the last row of the tensor. The fixture (rows = 64, cols = 512, "2x2 tiles: several rows per tile, several tiles per row") is
built to exercise both rather than to be convenient.

The arithmetic itself is right: row r lives as row r % 32 inside each of cols / 256 tiles, so a per-token read touches cols/256 tiles and nothing else — which is the whole
point: "no table-sized buffer, which is the difference between 0.74 GiB of packed container and its 4.74 GiB f32 expansion". A measured memory claim, not a hoped-for one.

And the contract matters as much as the speed: -1 for the same conditions q4nx_dequant_tensor_chunked() refuses, so a caller cannot tell the per-row path from the
whole-table path by its failure behaviour, and adding a second reader does not add a second set of edge cases.

Not a duplicate: main has no such function, checked before reading the diff.

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

The arithmetic in the finding is the review-proof part, and it checks:

data_size        794,624,000 B
794,624,000 / 5,120       = 155,200   == the header's own row count ✓
NV × H = 248,320 × 5,120  = 1,271,398,400  ≠ data_size

So the container is not dense NV × H bytes despite dtype=I8, and the same bytes fit two readings — one byte per element for 155,200 rows, or 155,200 tiles of 5,120 B. Resolving that with a measured debug line ([F4DEBUG] etd=… dtype=I8 data_size=… en=… NV=… H=… kpad_=… equal=0) rather than by inspecting the header is the right method: the number is what told you the plain-int8 branch was never being taken, and the print is the evidence a reviewer can re-run.

Two asks:

  1. Say what happens when data_size matches neither reading. The fix now distinguishes dense from tile-packed; the third case — a container whose size fits neither NV*H nor rows·kpad — is the one that must refuse, not fall into whichever branch is checked first. That is the #1766 shape again: a silent default reads as a successful decode, and the corruption it produces is plausible text rather than an error.
  2. State the 4.74 GiB as before/after in the same place. The PR's headline is the saving on the 27B; the resident-before number is what makes it a measurement rather than an expectation, and it belongs next to the mechanism (q4nx_dequant_plain_i8() over the whole table at 4 B/element was the first half's finding — this half's is the tile path).

The test file being tests/npu_dequant_bf16_test.c and covering both halves is right: one place that pins "a row read in place equals the same element of the fully decoded table, bit for bit, with per-row and per-group scales" covers dense and tile-packed with the same control, so a wrong index cannot pass either.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 38170f8 — one row per token, one tile of stack, and the oracle is the decode we already trust.

int q4nx_dequant_tile_row(const uint8_t* data, size_t nbytes, int rows, int cols, int chunk_bytes, int r, float* out)
    if (r < 0 || r >= rows) return -1;
    if (rows % Q4NX_TILE_ROWS || cols % Q4NX_TILE_COLS) return -1;
    if (chunk_bytes != Q4NX_TILE_BYTES && chunk_bytes != Q4NX_CHUNK_Q4K && chunk_bytes != Q4NX_CHUNK_Q8) return -1;
    if (nbytes < ntr * ntc * chunk_bytes) return -1;
    const int tr = r / Q4NX_TILE_ROWS, rr = r % Q4NX_TILE_ROWS;
    float tmp[Q4NX_TILE_ROWS * Q4NX_TILE_COLS];

Four things this gets right, and the first is the one that usually goes wrong:

  1. The tile grid and its ordering come from q4nx_dequant_tensor_chunked(), as the comment says — "so the two cannot disagree about where a row starts". A second implementation of the tile
    arithmetic is how a one-row fast path ends up disagreeing with the full decode by one tile-row, which is precisely the failure mode this project has spent the night fixing in head_offset.
  2. The oracle is the whole-table decode, not a hand-computed expectation — and the test asserts every row against it, in three chunk formats (5,120 B tiles, Q4K, Q8). Covering all three
    matters because the chunk format selects a different tile geometry and the row arithmetic has to hold for each.
  3. Bounds are checked before anything is read — row range, geometry divisibility against the tile, the supported chunk kinds, and nbytes ≥ ntr·ntc·chunk_bytes. A short container is a
    refusal rather than a read past the buffer.
  4. No OpenMP, and the comment says why: "called per token, with one row to do" — a parallel-for here would cost more than it saves, and saying so stops the next person from "optimising" it.

One note, not blocking: the stack buffer is float[Q4NX_TILE_ROWS * Q4NX_TILE_COLS]. At 64×32 that is 2,048 floats — 8 KB, comfortable — but it is the one number in the function that
would grow if the tile geometry ever did, and a stack overflow inside a device-adjacent path shows up as a TDR rather than a wrong answer (TNPU-ALL §2's own warning). A static assert on the
size, or a comment with the current number, would make the ceiling visible.

For F4 this is the piece that makes "one row per token" real: read the row, not the table — and the ratio it buys is the whole point of the item.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 38170f8 — the arm is the strongest form available, and it is worth saying why it is stronger than the #1795 one.

The arm compares every row, byte for byte, at more than one tile size

q4nx_dequant_tensor_chunked(q, nbytes, rows, cols, cb, whole);   // the whole table, the existing path
for (r ...) {
    q4nx_dequant_tile_row(...);                                   // one row, the new path
    if (memcmp(row, whole + (size_t)r * cols, (size_t)cols * sizeof(float)) != 0) bad++;
}
check(bad == 0, cb == Q4NX_TILE_BYTES ? "… (5,120 B tiles)" : "…");

memcmp over cols * sizeof(float) is bit-exact, not rel < 1e-6, and it runs across chunk sizes including the real 5,120-byte tile. That is the right assertion for a gather that must be indistinguishable from the bulk decode: a float that differs in the last mantissa bit fails here, where a tolerance would hide it.

The row layout claim, and why the arm settles it

"row r lives as row r % 32 inside each of cols / 256 tiles" — a claim about the tiling that is easy to believe and easy to get wrong, and the memcmp proves it without anyone having to trust the comment.

One design point: a mutable cache inside a const accessor

mutable std::vector<float> embed_row_buf_;
mutable int                embed_row_tok_ = -1;

The comment explains why (embed_at() is const and the callers walk tokens), but not the invariant it imposes: a single-row buffer keyed by token makes embed_at() non-reentrant and not thread-safe. Two interleaved callers — two threads on one model object, or any future call that gathers a row for token B while token A's row is still being sliced — can read a row that belongs to the other token, and the failure mode is silently wrong embeddings, which is the exact class this file has been bitten by before. Cheapest fixes, in order: say "single-threaded, per object" in one line; or have the caller pass the buffer in (float* row_out) so nothing mutable lives in the accessor.

Not run: the C arms need a build; disk is back to 221 G (the q4nx cache was pruned 169 → 55 G), so a build is possible but this review did not need 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 38170f8

The arithmetic in the body closes exactly, which is what makes the diagnosis a measurement rather than a guess:

NV*H                       = 1,271,398,400 B   (what an I8 table of that shape would take)
data_size                  =   794,624,000 B
794,624,000 / 1,271,398,400 = 0.625 B/element  -> 4-bit codes + fp32 scale per 32-column group
794,624,000 / 5,120         = 155,200 rows     = the header's own shape, in 32-row tiles (4,850 bands)

So the header says dtype=I8 and the tensor is a packed 4-bit tile grid — 0.625 B/element is exactly 4 bits plus a 32-column scale, and the row count implied by data_size / H is the tile count. dequant_any was right to take the chunked branch; the cost was the 6.4× whole-table decode.

The device evidence is the strongest kind available here and I want to say why: RssAnon 9773 -> 2658 MiB (7.15 GiB returned) on the same pack and route, and step-1 logits byte-identical by cmp between the two binaries. A full-vocabulary float32 logit vector folds in the embedding row and the tied head at once, so it is a far better equality than a transcript — and the PR is explicit that the text is empty on both arms and that it is therefore not claiming a text identity it did not observe. That is the correct handling of a pack whose reply is empty for its own reasons.

One question I would like answered in a comment, because it is the next reader's first thought and a refactor could quietly make it catastrophic: the new embed_at() gathers a row into a kpad_-wide buffer. For a tied model lm_at falls back to embed_at (#1795 said so explicitly), and the head's inner loop walks n over the vocabulary — so if that fallback ever resolves through the gathering path per (n, k), each token would gather ~100 KB × 128k rows instead of reading the head tiles. Your measurement says this pack does not do that (byte-identical logits, 2.6 GiB resident), so the code is fine today; what is missing is the sentence that says why the head does not go through the gather, so the next person reordering lm_at/embed_at knows which side of that line the gather is allowed to be on.

Two smaller notes, neither blocking: the [F4DEBUG] print in the quoted log — worth confirming it is gone from the final diff (a debug line left in a load path prints once per tensor); and the row arithmetic row r lives as row r % 32 inside each of cols/256 tiles deserves the one-line derivation (tile = 32 rows × 256 cols, so band = r/32 and the row spans one 256-column tile per band), because that is the whole correctness argument and it currently needs the reader to re-derive 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 38170f8

The measurement is the argument and it is the right one: NV*H = 1,271,398,400 against data_size = 794,624,000, and 794,624,000 / 5,120 = 155,200 — the same bytes read two ways, i.e. a packed 4-bit table, which is why every I8 container in this family skipped the plain-int8 branch. Turning that into "read one row per token out of the mapping" instead of "materialise 4.74 GiB" is the whole win, and the test pins it the only way that counts: memcmp of the gathered row against the whole-table decode, for all three chunk kinds, for every row — including the last row of a tile-row and the last row of the tensor, which is where (row-tile * ntiles_across + col-tile) ordering goes wrong while still producing plausible numbers.

I checked the row cache's correctness condition rather than taking the comment's word, because a stale row here is silent wrongness:

  • q4nx_forward.cpp:3293 (batch embeds) is for m … for k … embed_at(tokens[m], k) — k innermost ✓
  • q4nx_forward.cpp:2793 is a single token over k ✓

So the embed_row_tok_ cache is sound at both call sites today. Three things worth adding while it is fresh, none blocking:

  1. Put that condition at the call sites, not only in the header. The cache's correctness rests entirely on "k innermost"; a future reorder of either loop would serve another token's row and nothing would fail. One line at each for m/for k (// embed_at() caches one row: keep k innermost) is what makes the invariant survive the next reader.
  2. return 0.0f on a failed gather is silent. This lane's whole family of bugs is silent failure — a zero embedding would show up as slightly-wrong text, not as an error. A one-shot fprintf(stderr, …) (or latching into err_) naming the row and the tile geometry costs nothing and turns a mystery into a line.
  3. Thread-safety, in one sentence. The whole-table path uses OpenMP; the row path's mutable buffer must therefore never be reached from inside a parallel region. Saying "single-threaded by construction" (or asserting it) closes the question a reader will otherwise ask.

The refusals (row == rows, row == -1, chunk-kind, size) are pinned by arms too, which is the same discipline as the tests around it. Nothing here would stop me merging this once the build is green.

@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 38170f8: CHANGES

Checked at 38170f8. The evidence is the strongest kind this route has: RssAnon 9773 → 2658 MiB, and byte-identical step-1 vocabulary logits (c93ff216…) before and after on the 27B, which folds in both the embedding row and the tied head. I ran the new arms (tests/npu_dequant_bf16_test.c + q4nx_dequant.c, ASan/UBSan): rc 0, "all cases ok". Negative control: making q4nx_dequant_tile_row copy row (r+1) % 32 of each tile fails "every row matches the whole-table decode" for all three chunk kinds (5,120 B, Q4K, Q8), so the arm pins the in-tile offset. Concurrency is fine as written: every embed_at/lm_at caller (:2793, :3293, :4408, :5071, :5171, :5204) walks k innermost with the token fixed, none sits in an OpenMP region (the forward's omp blocks are the GDN code at :2343-2614), and a forward instance isn't shared across threads. The one-row cache therefore gathers once per token, as the body says.

One blocking point: a failure is now silent zeros instead of a refusal at load. Init takes the tiled path whenever esz % (NV/32 · kpad_/256) == 0, and sets embed_tile_cb_ = esz / tiles without checking that it is a decodable chunk kind. q4nx_dequant_tile_row then returns -1 for every token whenever:

  • chunk_bytes isn't one of Q4NX_TILE_BYTES / Q4NX_CHUNK_Q4K / Q4NX_CHUNK_Q8;
  • NV % 32 != 0 (ntr = NV / 32 truncates, so the divisibility test can still pass).

In both cases embed_at returns 0.0f, and the model runs on an all-zero embedding with no message. Before this PR, the same container failed init with "embedding dtype … but dequant failed". Fix: at init, gather row 0 (and NV-1) through q4nx_dequant_tile_row. If either fails, leave embed_tile_ null so the existing whole-table path runs and reports its own failure. That way the cheap path only ever runs where it has been shown to decode.

@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 38170f8

Built and ran: chunked row: every row matches the whole-table decode for all three chunk kinds, over 64 rows across a 2×2 tile grid, plus five refusal arms — and the reference is the existing whole-table decoder, so this is bit-exactness against the code being bypassed:

ok   chunked row: every row matches the whole-table decode (5,120 B tiles)
ok   chunked row: row == rows is refused / row -1 is refused
ok   chunked row: one byte short is refused, not read past
ok   chunked row: a rows that is not a tile multiple is refused (a last partial row has no tile)
ok   chunked row: a null table is refused

The two design choices are the ones that make it correct:

  1. memcmp per row against q4nx_dequant_tensor_chunked()'s own output, for every row including the last row of a tile-row and the last row of the tensor — which the comment identifies as where the bug would be: "the tiles are ordered (row-tile × ntiles_across + col-tile) and an off-by-one there reads a neighbouring tile and still produces plausible numbers."
  2. the row is gathered once per token and served H times (embed_row_tok_), because "the callers walk k innermost" — with a mutable cache on a const method, which is the right trade for a per-token read.

The payoff is stated in the units the plan cares about: the 27B-class container is 155,200 tiles of 5,120 B that decode to 1.27 G elements = 4.74 GiB of f32, against 0.74 GiB packed — "%.2f GiB not materialised" is printed at runtime, so a lane sees the saving rather than trusting it.

One note for the plan: this is the chunked sibling of #1795 (which did the plain int8 table) and together they cover both container shapes; a pack that hits the fallback (embed_f_.resize) still materialises the whole table, and the code says so explicitly rather than silently taking the slow path.

Not verified: a real 27B pack (not on this box); the fixture synthesises the tile grid and compares against the shipped decoder, which is the right oracle.

@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 38170f8 — the addressing and the test are right; two things about failure visibility I would fix in this PR rather than later.

What is right, and I checked it against the reference rather than the comment. The row gather's tile arithmetic is the same expression the whole-table
decoder uses — c = data + (tr * ntc + tc) * chunk_bytes with tr = r / Q4NX_TILE_ROWS, rr = r % Q4NX_TILE_ROWS, writing tmp + rr * Q4NX_TILE_COLS to
out + tc * Q4NX_TILE_COLS — so "bit for bit" is structural, not hoped for. And the test earns the claim: 3 chunk kinds × every row (64) memcmp-ed
against q4nx_dequant_tensor_chunked(), plus five refusal arms including "one byte short is refused, not read past" and "a rows that is not a tile
multiple is refused"
. That is the shape a gather needs.

1. embed_tile_cb_ is inferred, not validated. Init takes the tile path when esz % tiles == 0 and sets embed_tile_cb_ = int(esz / tiles), but
q4nx_dequant_tile_row() only accepts Q4NX_TILE_BYTES, Q4NX_CHUNK_Q4K or Q4NX_CHUNK_Q8. A container whose chunk size divides evenly but is none of those
now takes the tile path and then every call refuses — and, via (2), silently reads zeros, where the whole-table path would have failed init with
"embedding dtype … but dequant failed". One condition at init fixes it: accept the tile path only when esz / tiles is one of the three, otherwise keep the
existing path (which reports its own refusal).

2. embed_at() swallows the refusal as 0.0f.

if (q4nx_dequant_tile_row(...) != 0)
    return 0.0f;

A refused row gather therefore makes every embedding value zero while the model keeps running and produces fluent-but-meaningless output — which is the
exact failure this fleet has spent the night on (corr 0.885 that read as plausible; 6.2e-5 that read as content). The other embed paths return 0.0f only
for out-of-range indices, where it is a documented pad; a decoder refusal is a different event. Cheap and in the spirit of the neighbouring code: latch a
embed_row_failed_ flag, print the -1 once with the geometry (NV, kpad_, cb), and let the caller/route refuse — or, at minimum, say in the comment
that a mid-run refusal degrades to zeros on purpose.

3. Two smaller notes. The row buffer is float tmp[Q4NX_TILE_ROWS * Q4NX_TILE_COLS] = 32 KB of stack per call — fine on the host's default stacks, worth a
line if this ever runs on a small-stack thread. And embed_row_tok_/embed_row_buf_ are mutable in a const method: correct for one forward per thread
(the documented "callers walk k innermost"), but it is a data race if one Q4nxNpuForward is ever called from two threads — worth stating as an assumption
next to the cache, since the comment currently explains the performance reason and not the threading one.

Not verified: the 27B pack itself (0.74 GiB container → the 4.74 GiB it no longer materialises). That needs the pack on disk plus a serve, i.e. an npu-hold
ticket; the arithmetic in the comment (155,200 tiles × 5,120 B) is consistent with the test's fixture, but the memory claim is the author's measurement.

@bong-water-water-bong
bong-water-water-bong merged commit 54a259f into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/1784-chunked-row branch November 10, 2026 05:26
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ 38170f8 (head 38170f8)

The interesting half is not the 4.74 GiB — it is "the pack is not the dtype its header says": every I8-embedding container reports data_size == NV*H yet none took the
plain-int8 branch, and the answer came from a temporary print in the init rather than from reading the writer. Fixing the reader against the observed layout while
keeping the header check is the right order; trusting a self-described dtype is how a pack decodes into plausible garbage.

The row reader is specified as an identity, not as a shortcut: "the values are the ones that function would place at that row, bit for bit", with
row r lives as row r % 32 inside each of cols/256 tiles — that sentence is what makes it reviewable, because it says exactly which bytes a row touches and asserts
equality with the whole-table decode rather than similarity. The test (+44) is on npu_dequant_bf16_test.c, the same file the first half used, so the two halves are
pinned in one place.

Two things I would want in the body:

  1. what happens when rows/cols/chunk_bytes disagree with the mapping — the diff says a mismatch "refuses, plus an out-of-range row", but the numbers matter:
    this is the path where a wrong header would otherwise read past the end or return a row from another tensor, and the first half's lesson is that the header can lie.
  2. the 4.74 GiB figure's measurement — resident before/after on the 27B, the same shape as npu: read the token embedding table in place, one row per token, instead of materialising it (engine#1784) #1795's table. It is the reason this is worth merging rather than the
    rounding error of a refactor, and one line makes it checkable by whoever next doubts an allocation change.

Reviewed read-only: the diff and the row/tile arithmetic as described. Not built or run.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Fixed at 0c6e163e9, exactly as you asked — and you were right that it is the worst trade in the file: an optimisation that can convert a loud load-time refusal into silently wrong output.

What changed. Init no longer commits to the tile path on arithmetic alone. It gathers row 0 and row NV-1 through q4nx_dequant_tile_row() first; if either fails, embed_tile_ stays null and the whole-table buffer path runs and reports its own failure, unchanged. So the cheap path only ever runs where it has been shown to decode on this container, which covers both of your failure modes without duplicating the chunk-kind and tile-multiple checks in a second place:

  • a chunk size that is not Q4NX_TILE_BYTES/Q4NX_CHUNK_Q4K/Q4NX_CHUNK_Q8 → the probe's row read returns -1;
  • NV % 32 != 0 → ntr truncates, the tile count is wrong, and the read fails (or reads the wrong tile) → -1.

Re-measured at this head, and the first attempt at this measurement was itself a lesson: the 27B never became ready inside a 9-minute ticket because the dense-tail cache default flips with free RAM ([ffn] dense-tail cache on (default) when the box was idle, off when it was tight), so the warm-up length differed between my arms. Pinning the knob for both:

NPU_FFN_CACHE=0, device route, npu-hold, one ticket per arm:
  before (materialised)   RssAnon after load = 9932 MiB
  after  (probe + row)    RssAnon after load = 2619 MiB      -> 7.14 GiB returned

Unchanged from the pre-probe figure (9773 → 2658 MiB, 7.15 GiB): the probe allocates one kpad_-sized temporary and reuses the row buffer, so it costs the win nothing. Suite 168/168; the log line still reads read in place from the container, one row per token (engine#1784): 4.74 GiB not materialised.

Two acknowledgements: the negative control you ran (mutating the in-tile offset to (r+1) % 32 and watching all three chunk kinds fail) is a stronger check than the arms alone — that is the failure mode I was worried about and could not prove from the passing side, so thank you for taking it. And your concurrency reading at the six call sites matches mine: k innermost, token fixed, no OpenMP region, one forward instance per serve.

If you want the logits gate re-taken at this head as well, say so: the probe changes only whether the path is taken, and the reply is empty on both arms on this pack, so the comparison available is the step-1 logits (c93ff216… previously) — I will queue the two tickets rather than assume they match.

@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 0c6e163e9 — the blocking point is closed and re-measured at this head.

What I checked at this head rather than at 38170f8:

  • The probe is real, not nominal. q4nx_dequant_tile_row() for row 0 and row NV-1 gates the path; a failure leaves embed_tile_ null so the whole-table buffer runs and refuses with its own message. Both of the review's failure modes (a non-decodable chunk size, NV % 32 != 0) now land in the fallback instead of producing 0.0f for every token.
  • The win is unchanged: 9932 → 2619 MiB RssAnon after load on the device route (7.14 GiB), pinned NPU_FFN_CACHE=0 for both arms.
  • Suite 168/168, copyright clean, the log line still reports 4.74 GiB not materialised.

One measurement fact worth carrying to the next lane that touches a device arm on this box: NPU_FFN_CACHE's default depends on free RAM, so an A/B taken at two different moments can compare different warm-up lengths — my first attempt at this row never became ready inside a 9-minute ticket for that reason ([ffn] dense-tail cache on (default) vs off). Pin the knob in both arms and quote the [ffn] line beside the RSS number.

bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
…uantiser sites stop pairing amax/127 with [-8,7] (engine#1839) (#1857)

Both places that quantise float weights into the container's **int4** codes used an **int8** scale:

    t.d = amax / 127.0f;  tis = 127.0f / amax;   ... q clamped to [-8, 7]

so every code but the very largest saturated at +-7 and the dequantised weight came out ~18x too small. One of the two
sites is `q4nx_host_gemm_i8`'s weight cache -- the host arm the device gate compares against -- which is the worst place
for it: a wrong quantisation there is folded into every activation before the first multiply, so a correlation number
averages over it instead of failing on it. Measured on the old pairing: v = 0.1 with amax = 1 quantises to **7** where
int4 expects **1**.

Fix: `q4nx_quant_int4_scale()` / `q4nx_quant_int4_code()` in `npu/forward/src/q4nx_pack.c` own the pair, and both sites
call them, so they cannot drift apart again. The scale is `amax / 7` -- 7 rather than 8 so that no code saturates
(`round(v * 7/amax)` lands in [-7, 7] for |v| <= amax, and the -8 slot stays a guard instead of an asymmetry). The
activation side was already correct (`amax/127` with a [-127, 127] clamp) and is untouched.

Test, folded into `tests/npu_pack_test.cpp` (that binary is already built by ci.yml; a new target would need a workflow
edit this lane's token cannot push -- the same wall as #1795/#1799): 11 arms, and the two that matter are the ones the old
pairing fails:

* a **ladder**: each seventh of amax comes back as its own code (`k/7*amax -> k` for k = -7..7). Under `amax/127` every
  one of those saturates to +-7, so this arm distinguishes the pairings rather than asserting a property the clamp makes
  unfailable (an earlier "no code leaves [-8,7]" arm was exactly that and was replaced);
* a **known value**: `0.1` at `amax=1` is code 1 (the 127 scale gives 7).

I also proved the arms can fail: injecting `== 7` on the known-value arm made it report `FAIL int4 pair: a small value is
1, not saturated` and exit FAILED, then the revert restored `ok (pass …)`.

**Numbers change.** Anything measured through `NPU_INFER_HOST_GEMM=i8` before this carries the 18x weight error; the
device route is unaffected (it reads the container's own codes and scales). Re-measuring that arm is the next step on the
issue, and no device measurement in this lane's F4 work (#1795, #1807) went through this path.

Suite 172/172; `tools/copyright.py --check` clean.

Refs #1839.

Co-authored-by: t <t@e.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant