Repository navigation
npu: the padded-head reader counts tile-rows in shape[0], so a [tiles, tile bytes] head dequantises (refs #1990, #1992) - #2020
bong-water-water-bong wants to merge 14 commits into
Conversation
…private re-quantisation The arm was merged in #1809 with a quantisation rule of its own: per 128-K group, int4 codes in [-8, 7]. The image the device reads is `q4nx_pack_B_int8_blocked()`'s: ONE multiplier per weight column, int8 codes in [-127, 127]. Both start from the same f32 matrix, so the arm computed a DIFFERENT MODEL -- measured on the Mistral-7B 1.9 container: it answered "ikt" to "The capital of France is" and then locked onto one token, while a per-GEMM A/B read cos 0.9999 against f32 on the same tensors. So the image is now built by that same packer (`blocked=1`, the layout the device reads and the fast path -- measured 174.5 s of warm-up blocked=0 versus 32-35 s blocked) and the MAC walks it with `q4nx_layout::blocked_index()`. Measured after, on current main: warm-up 35.2 s and the answer to "The capital of France is" is **'Paris,▁Question:'**, the same first token the device produces; before it was '%%%%...' / '▁/******/'. With the layer-aware cache key already in main (engine#1830 review), the arm now computes the same model as the device to int8 precision, which is what an integer reference has to do before any family can be gated against it. `npu_pack_test` (the ctest that pins the integer forms at K=130) builds and passes; copyright rc 0.
…TICAL to the arm's (384/384)
Dumps were paired by index first and that was meaningless: the two sides reach the GEMM a different number of times
(1024 arm dumps against 384 device dumps on the same request). Keyed by (layer, N, K, ordinal) they pair exactly.
Result, one token, Mistral-7B 1.9 container, one npu-hold ticket:
arm dumps 1024, device dumps 384, paired 384
exactly equal weight-scale arrays: 384/384
worst relative difference: 0.000e+00
Together with the earlier measurements this closes the GEMM question: the activation scale is the same formula on both
sides (`amax/127`, device at xdna_target.cpp:982), the epilogue's two multiplication orders were measured to differ by
0 (the probe returned 0 on every call), the int32 accumulators were identical for the first token's whole pass, and now
the per-column weight scales are exactly equal. So the device's GEMMs are bit-exact against this arm.
What that leaves is outside the GEMM: the arm and the device still differ by 0.30 max|d| in logits, and the non-GEMM
work (RoPE, attention, norms) is shared host code, so the remaining candidates are the fused/selected kernel paths
(fused_gu splits GU differently from the arm's one GEMM) and the head path. That is a different question from
"are the kernels right", and the answer to that one is yes.
Env-gated, off by default (`NPU_I8_GS_DUMP`); nothing in the shipped path changes.
…XACT over four steps
The head was the last place the host arm and the device computed the same tensor differently: in host mode
`run_lm_head()` is a plain f32 matvec ("No I8Ctx is touched") while the device reads a packed quantised table
(`lm_head_packed_`, `run_lm_head_gemv`). That was the whole of the remaining `max|d| = 0.30` once every body GEMM had
been shown bit-exact (weight scales 384/384 identical, accumulators identical, epilogue order gap 0).
The device packs its head tiles with `lctx.packB_into(...)` -> `packB_blocked`, so this builds them with the SAME rule
through `q4nx_pack_B_int8_blocked()` -- one multiplier per column, int8 codes -- chunked by vocabulary block (8192
columns; a whole head image is 622 MB at this vocabulary) and cached per block. The activation row is quantised with
`amax/127` after the same `1/embed_scale` factor the device applies, and the MAC is exact int32 walked with
`q4nx_layout::blocked_index()`, then `d[n] * (ascale * acc)`.
Measured, one npu-hold ticket, device vs this arm, logit dumps compared per step:
step 0: argmax 3916 == 3916 cos 1.000000 max|d| 0
step 1: argmax 278 == 278 cos 1.000000 max|d| 0
step 2: argmax 28725 == 28725 cos 1.000000 max|d| 0
step 3: argmax 22478 == 22478 cos 1.000000 max|d| 0
BIT-EXACT over 4 step(s) at max|d| tolerance 0.0
That is TNPU-ALL section 4's bar met literally for this family rather than read down to "at the accumulator": the
device's arithmetic and the integer reference agree to the last bit, on every step, for a full request.
…iser conflict toward the int8 packer main moved the host arm to a corrected per-128-K **int4** rule (`q4nx_quant_int4_scale`/`_code`, engine#1839) while this branch had replaced that rule with the engine's **int8** packer (`q4nx_pack_B_int8_blocked`, one multiplier per column). One hunk, same lines, two self-consistent answers. Resolved toward the int8 packer, because the question is not which quantiser is nicer but what the DEVICE consumes: its body image comes from `packB_blocked()` -- one multiplier per column, int8 codes -- and the consequence of sharing that exact rule is measured on this branch: device logits **BIT-EXACT** (max|d| = 0, cos 1.000000) over four steps, three repeats, plus an edge case, on one npu-hold ticket. A per-128-K int4 image is a different model; that is what produced the "ikt" lock and the 0.30 residue in the first place. `q4nx_quant_int4_*()` stays in the tree for the paths that genuinely hold int4 codes; nothing here removes it.
The integer arm was only discoverable from the code. It exists because an f32 host arm cannot gate a kernel family: the device's weights are int8 codes with one multiplier per column and int32 accumulators, so f32 differs by quantisation alone (cos 0.9960-0.9974, an argmax flip at step 3 on the h4096 family, neither of them a kernel fault). Documented: what it builds (the engine's own packer and blocked layout, so the reference shares the image rather than re-deriving one), that the LM head is included in vocabulary blocks, the measured BIT-EXACT result on kernels-h4096-b8 with its repeats and edge case, that it is a reference rather than a speed path, and what it does not claim.
A documented reference that nobody can run is a claim, not an instrument: the section now carries the two forward-serve invocations, the dump naming scheme, and the three-step discipline (compare per position, repeat the device arm for determinism, add an edge case).
… at all
`kFconst19Rules` held the 32-layer h4096 row and nothing for 36 layers, so `npu-pack info` refused
`amd/Qwen3-8B_rai_1.9.0_npu` by design: "its block shape has not been measured for hidden 4096, 32 heads, 36
layers … the 1.8 pack of the same model is what establishes it". Measured now, with
tools/fconst19_measure.py against amd/Qwen3-8B_rai_1.8.0_npu_16K:
{4096, 32, 36, 12608, 5, 1, 2297856}
26,104 of 3,756,627,264 bytes differ (6.95e-06 of the file) -- content, not layout
Every field but the layer count is identical to the 32-layer row (prefix 12,608, five tiles of 2,297,856,
rotation 1), which is the first row here where the layer count moves nothing but the number of blocks: the h2048
pair moves both the prefix and the division with it (4,160/9 tiles at 36 layers against 8,512/3 at 24), so this
row is measured rather than inferred from that one.
The consequence beyond this pack: a kernel family and an fconst rule row are not the same partition. One family can
span several rows, so each new pack of a family may cost a measurement before it costs a gate.
Verified end to end: the import now completes -- 5,760/5,760 .fconst tiles survive decode->encode byte for byte,
144 attention projections plus 108 MLP projections with fp32 scale/zero exact for 100.00% of groups, 8 full_i8 ELFs
linked from kernels-h4096-im12288-b8.
…'s own comment requires
CI caught the omission and said exactly why: "fconst19 rule: the table has exactly the 11 measured rows (12); a
dropped row is a silent un-measure of a whole class, an added one must be added here too". The expected rows are
literals copied from the measured table rather than read back from it, so that a wrong digit or a dropped row fails --
which is the arm working, not a nuisance. Added {4096, 32, 36, 12608, 5, 1, 2297856} to it, and the arm now passes.
…eep vocabulary imports CodeLlama-7b-Instruct's head is 32,016 rows and q4_1 tiles hold 32-row multiples, so the whole import refused with "q4_1 tiles need N % 32 == 0" -- a head the grid cannot hold, not a head that is wrong. The projections already had this rule (the pack's own padding stays, and the .fconst tail carries ceil(V/64)*64 rows: Hunyuan-0.5B has 120,832 for 120,818), but the graph-weight head path had no padding at all. pad_rows_to_tile_quantum() pads with rows that decode to zero (code 0, scale 0, zero 0 -- the quantised value is code*scale + zero, so a zeroed group is 0.0, not a weight) and LEAVES vocab_size at the vocabulary's own number, so the padding is never addressed. That invisibility is the part a test has to pin: widening N without it would put two plausible extra tokens at the end of every distribution. Measured on the pack that found it: the import now completes -- 387 tensors, 806,544 q4_1 tiles, container read back through q4nx::dequant_tile with rel_err 0.000e+00 and max|diff| 0.000e+00, 8 full_i8 ELFs linked from kernels-h4096-mha-b8 (the 4096x12288 family TNPU-ALL W3b asks to verify end to end).
…0, and dequant on the tensor's grid The first gate run on CodeLlama refused with "lm_head dequant failed", and it was right twice over. 1. The padding wrote SCALE 0 for its rows. Arithmetically code 0 * 0 + 0 = 0.0, but a zero scale is not a zero weight to a reader -- it is a malformed parameter block, and the dequant refuses it. The test asserted exactly the wrong property (scale == 0.0f), which is how a wrong assumption gets to look pinned. Both now say scale 1.0, with the reason written next to them. 2. The forward asked for exactly NV rows, and NV here is the VOCABULARY (32,016), which is not a multiple of the tile's 32 rows -- so dequant_any() refused a head that was perfectly well formed. The container's vocab_size is the vocabulary; the tensor carries the padding; the reader has to dequantise on the tensor's own grid and keep only the vocabulary's rows. That is the same convention the .fconst path already relies on (Hunyuan-0.5B: 120,832 rows for 120,818), and it was simply not implemented on the graph-weight path. The extra rows decode to zero and are never sampled: the logits vector stays NV long. So this commit is importer + reader, and the PR's claim is only true with both: "the padding is invisible to the vocabulary" is a property of the pair, not of the writer.
…o a non-256-aligned IM gates host_gemm_i8() read its activation row with a[k] for k < K, and K is the PADDED design width. chatglm3-6b's IM is 13,696 and its D design is K = 13,824, so the arm read 128 floats past the caller's row -- into the next row's data, or past the buffer on the last one. Measured before the fix on kernels-h4096-chatglm-b8: cos 0.40 -> -0.003 -> 0.06 -> 0.005 against the device, argmax unrelated, while the f32 arm tracked the device at 0.985-0.988. After: max|d| = 0 on all four steps, cos 1.000000. The f32 leg has always padded -- q4nx_host_gemm_f32_padded() "owns the pad-and-read pair so neither leg can be handed an unpadded A", a comment that records a review catching exactly this -- and the integer leg, written later, did not. One lambda now serves both loops (amax and quantise), so the two cannot drift again. Why this looks like three unrelated mysteries until it is one: the families whose shapes are all 256-aligned were bit-exact (Mistral, Qwen3-8B), the one with a padded K was wildly wrong (chatglm), and the one with no padding but a residual (CodeLlama) is a different, smaller defect. This is the first hybrid pack with a non-aligned IM.
…ork plus this branch's activation pad main's lane solved the padded-geometry problem on the IMAGE side (unpadded K*N images, blocked order only where blocked_fits(K,N), q4nx_layout::image_index) -- more general than anything here, so every image-side hunk is theirs. What main does not have is the other half: host_gemm_i8() still read a[k] for k < K with K the PADDED design width, so chatglm3-6b (IM 13,696, D design K 13,824) read 128 floats past every row: cos 0.40 -> -0.003 against the device while the f32 arm tracked it at 0.985-0.988. Kept on top as one lambda serving both loops, with the measurement in the comment. Together: the image must not assume tile alignment, and the activation must not be read past its row.
… and make the head block width a knob The merge that took main's unpadded-image work wholesale over q4nx_forward.cpp also took main's head dequant, which asks for exactly NV rows -- so the padded-head reader (10 references, reduced to 3) was silently lost and CodeLlama went back to "lm_head dequant failed" on the next run. Restored verbatim from 8932ac0. That is the second time a resolve-to-main has cost a fix on this file, which is why the block now carries its own measurement in the comment. Also: NPU_I8_HEAD_TB (default 8192, multiple of 128). CodeLlama's residual is confined to exactly sixteen logit positions starting at 16384, identical at every step -- a WEIGHT difference localised at a block edge, not accumulation drift -- and 16384 is where this arm's first head block ends. Moving the block width decides whether the chunking or the device's head read is responsible, in one ticket instead of an argument.
…, tile bytes] head dequantises (refs #1990, #1992) `q4nx_forward.cpp`'s padded-head path (engine#1990) gated on `ltd->shape[1] == kpad_` before deriving `head_rows` from the tensor's grid. A container that stores the head as `[tiles, tile bytes]` -- which is what the importer writes, e.g. CodeLlama-7b's `I8 [16016, 6144]` for a 32,032-row head at `kpad_` 4,096 -- fails that test, so `head_rows` stayed at `NV` (32,016) and `dequant_any()` refused the tensor: `lm_head dequant failed`, on a head that is exactly what the importer had just been asked to pad and write. Both spellings count **tile-rows** in `shape[0]` (`[rows, kpad_]` and `[tiles, tile bytes]`), so the number of 32-row blocks is `shape[0] / per_block` either way -- only the row width in `shape[1]` differs. The gate is now just "the block count divides evenly", and the padded rows are decoded and dropped as before (`head_rows != NV` copies only `NV` rows out). ## Measured end to end, on the pack this is for `amd/CodeLlama-7b-Instruct-hf-onnx-ryzenai-hybrid` -- the **first MHA 32/32 member of the 4096 family** (H 4096, QKV 12,288, GU 22,016, D 11,008) -- imported with `--weight-layout b8` (387 tensors, 806,544 q4_1 tiles, head padded 32,016 → 32,032, `vocab_size` untouched), kernels `~/ogaload/oga13/kernels-h4096-mha-b8`: ``` before: 1bit forward-serve: forward init failed: lm_head dequant failed after : 1bit forward-serve: row-cl7b ready on http://127.0.0.1 (full ELF) [npu-target=xdna] "The capital of France is" -> ' Paris. The capital of France is Paris. …' "The first element in the periodic table is" -> ' hydrogen, which is the lightest element.' "2 + 2 =" -> ' 6\n # 1 + 2 + 3 + 4 = 10\n …' (a code model, continuing a list) 15 prompt tokens, ttft 3.0 s; 64 tokens in 66,735 ms -> 0.96 tok/s ``` 0.96 tok/s is a *measurement*, not a pass -- the dx bar is 9.2 tok/s, so the row reads `OOB | 0.96 tok/s` like the other 7B rows, and the point of this PR is that the pack reaches `ready` and answers at all. Refs #1990, #1992 (which pads the head at import; this is the reader half), #1503.
|
VERDICT at The overlap. Both PRs replace the
I reviewed Why this one earns the READY on its own terms: the measurement is end-to-end and falsifiable — the same pack goes from One note, carried over from |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38b70f9 — but this exact fix is already the third commit of #1992 (0d629ec), so only one should land
Both PRs patch the same block and the same line:
- if (ltd->ndim >= 2 && kpad_ > 0 && kpad_ % Q4NX_TILE_COLS == 0 && ltd->shape[1] == kpad_) {
+ if (ltd->ndim >= 2 && kpad_ > 0 && kpad_ % Q4NX_TILE_COLS == 0 && ltd->shape[0] > 0) {
const int64_t per_block = kpad_ / Q4NX_TILE_COLS; // tiles per 32-row block
#1992's 0d629ec says it as "npu: the head's rows come from its TILE count, [not shape[1]]"; this PR says it as "a [tiles, tile bytes] head dequantises [wrongly] because the reader counts tile-rows in shape[0]". Same defect, same remedy, same hunk. They will conflict textually on merge, and the fleet's rule from the last two days applies: merge one, notice the other.
Which one I would keep: #1992, because it is the whole of the shape. It carries the head padded to the tile quantum, the integer arm's padded activation, and this tile-count row derivation — the three directions that a padded buffer creates as one problem. Landed alone, this PR fixes the third direction and leaves the first two to another PR, and the review evidence for why they belong together is in #1992's own history: each one was found by running the previous one.
But the argument for this PR separate is real and worth stating: 148 lines with its own arms is easier to review than a three-commit PR, and if #1992 is going to take another round, landing this on its own unblocks CodeLlama-7b sooner. So: if #1992 is close to merging, close this and let it carry the fix. If #1992 is going to take another cycle, land this and drop #1992's third commit — either is coherent; merging both is not.
One line worth keeping either way, in the comment: "CodeLlama-7b: I8 [16016, 6144] for a 32,032-row head at kpad_ 4,096" — the concrete triple is what makes the shape [tiles, tile bytes] checkable by a reader who has never seen the pack.
|
VERDICT: APPROVE at The defect, in one line
Asking the container for exactly The arms are the two that matter
One thing worth stating where the reader isThe comment explains the padding exists because tiles are 32-row multiples; the reader half should say what it does when |
|
VERDICT: READY on the fix — The mechanism is now stated better here than anywhere else: a container that stores the head as The overlap: Two asks: the arm should use CodeLlama-7b's shape ( |
|
VERDICT: READY The new row is What makes this row reviewable, and the reason I trust it more than the ones I checked earlier tonight:
The reader fix (tile-rows counted in |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38b70f9 — this is the deeper fix for the same defect #1992 carries a weaker version of, and its key insight is better than the one I suggested. Two things to settle: which PR owns the reader, and one number.
The insight, and why it beats mine
Both spellings count TILE-ROWS in
shape[0]—[rows, kpad_]and[tiles, tile bytes]— so the number of 32-row blocks isshape[0] / per_blockeither way; only the row WIDTH inshape[1]differs.
That is the correct decomposition, and it makes the shape[1] == kpad_ gate unnecessary rather than merely wrong: per_block = kpad_/Q4NX_TILE_COLS turns either spelling into a block count without needing to know which one the container used. Checked against your example:
vocab 32,016 → padded 32,032; per_block = 4096/256 = 16
blocks = 16016/16 = 1001 head_rows = 1001 × 32 = 32,032 ≥ NV ✓
On #1992 I asked for exactly this sharpening (shape[0] == ((n+31)/32)·(kpad_/256)) as a suggestion; you have done it properly by deleting the assumption instead of tightening it, which is better.
Two PRs, one defect — please sequence them
#1992 (in review) also rewrites the same block, more weakly: it replaces shape[1] == kpad_ with shape[0] > 0 and leaves dequant_any(…, NV, kpad_, …) asking for exactly NV rows. It does not fix the refusal you describe, it just makes the gate pass; your change is what actually dequantises on the tensor's grid and keeps the vocabulary's rows.
So: this PR should own the reader, and #1992 should keep its importer-side padding and its {4096, 32, 36, …} table row and drop its q4nx_forward.cpp hunk. As it stands the two will conflict on that block, and landing #1992 first would put a permissive gate in place of your derived one — the worse of the two shipped first, which is how fixes get re-broken.
One number to reconcile
Your comment says the head is I8 [16016, 6144]; #1992's comment says {16016, 5120} for what reads like the same CodeLlama-7b tensor, and this tree's npu/forward/include/q4nx_dequant.h:53 defines Q4NX_TILE_BYTES 5120. Your per_block derivation is independent of the tile bytes, which is exactly why the fix is robust — but a reader will hit those two comments and one of them has to be wrong. model.c:279 already notes that "every model until Gemma3-1B had shape[1] == 5120", so a 6144-byte tile is plausible for an I8 layout that differs — if so, naming which container has it (and that it is not the 4-bit 5120) makes both comments true. Worth one clause, because in this tree a comment is the evidence a reviewer reads first.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38b70f9 — but first the sibling measurement, because this is the fourth head of one story and the overlap is large enough to matter:
#1992 (0d629ec): 127 added lines
#2020 (38b70f9): 133 added lines
shared: 121 #2020 is NOT a strict subset of #1992, and #1992 is not of it
121 of 133 lines are in both, and its title says "refs #1990, #1992" — so this is a carried-forward variant, not an independent fix. That distinction is worth stating plainly because it decides merge behaviour: whichever lands second will conflict across ~121 lines, and the two will have to be reconciled by hand. Merge one; if both are wanted, the second must be reduced to its ~12 unique lines.
The fix itself is the right one and it agrees with #1992's: the reader now counts tile-rows in shape[0] rather than comparing shape[1] against kpad_, so a [tiles, tile bytes] head dequantises instead of being refused. That is the same misreading I reviewed on #1992's third head — "the check and the thing checked were indexed differently" — so the two PRs have converged on the same correction independently, which is a good sign for the diagnosis and a bad sign for the queue.
+51 test lines for the reader path. The arm I would look for, and the reason this fix needs one on the reader side rather than only the importer's: a tile tensor whose shape[0] (tile count) is not a multiple of the grid — the geometry the padding work makes reachable. #1992's padding arms cover the writer; this is the reader that has to agree with it, and a reader that computes rows from shape[0] slightly differently from the writer's padding rule would produce a plausible dequantisation of the wrong rows.
Which to keep is the authors' call, not mine, but two facts should inform it: #1992 has three heads of review history and a max|d| = 0 measurement attached; #2020 is 12 lines newer and carries fewer of the doc/registry changes. If #2020 is the better final state, it should absorb what #1992's history established (the padding arms and the reproducibility clauses I asked for) rather than the queue carrying both to conflict.
|
VERDICT: READY at Three things make this the right level of fix:
The one thing to pinAn arm with Nothing else. |
|
Duplicate: this and #1992's own latest commit fix the same defect, and one of them should be dropped. Both change the same test in the same block:
And this branch is stacked on #1992's branch (its parent is A question on the numbers in the new comment, because they do not reconcile on their face. It says the head is Three readings I can see, and I cannot tell from the PR which is meant:
This matters because the guard's acceptance test is Independent of that, the rest of my #1992 review stands: the logits bound stays at |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 38b70f9
The gate was on the wrong axis: the padded-head path checked ltd->shape[1] == kpad_ before deriving head_rows, but a container that stores the head as [tiles, tile bytes] — which is what the importer writes, e.g. CodeLlama-7b's I8 [16016, 6144] for a 32,032-row head — has its tile count in shape[0], so the reader derived rows from a number that is not rows. That is the same family as #1992's activation overread and #1999/#2001's allocation mismatch: a tensor's shape being interpreted by one convention while it was written by another, and the failure is silence rather than a fault.
Three asks, and the first is now the fourth appearance tonight:
- Does the derivation come from the importer's rule, or is it re-derived here? Tonight has produced the tile-grid invariant in #1992 (
pad_rows_to_tile_quantum, head rows), #1999/#2001 (weight images,blocked_bytes), #1992's second commit (activation rows) and now this (head rows from a[tiles, bytes]shape). Four independent statements, four files. If the importer knows how many rows a head has, the reader should be asking it rather than reconstructing it from a shape — that is the single-owner fix, and it is the only thing that stops the fifth. - Which spellings does the reader accept now —
[tiles, tile_bytes]and[rows, kpad], or only one? If both are legal on disk (and the PR implies at least two exist), say which, and refuse the rest by name rather than deriving a row count from a shape it does not understand (#1875's unsupported chunk kind answering zeros is the failure to avoid here). - An arm with both spellings, asserting the derived
head_rowsisvocab_size-consistent in each. That is device-free and it is the exact regression: a future container written in the other convention must not silently produce a wrong row count.
Also worth keeping in the body: that CodeLlama-7b's head is 32,032 rows for a 32,016 vocabulary — the padding quantum from #1992 — because it makes the two PRs read as one story rather than two bugs that happen to touch the same word.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38b70f9 on the fix — and it is the same fix as #1992, which the queue has to resolve before either lands.
The two PRs are one change. I diffed the non-comment added lines of #1992 and #2020: same file, same message, same guard family, and the same
engine#1990: the container's head may carry MORE rows than the vocabulary … comment block opening both. The differences are cosmetic within the same idea —
#2020 factors the block count into a blocks variable and guards if (blocks > 0), #1992 inlines shape[0] / per_block in the r expression and guards with
kpad_ % Q4NX_TILE_COLS == 0 && shape[0] > 0. So: one of them should land, and the choice is the queue's, not something I should make by picking a favourite.
On the substance, #2020's variant is at least as safe as #1992's, and slightly more legible: the blocks > 0 guard is the one condition that makes the following
division meaningful, and naming the intermediate (blocks) is what lets the comment say tiles per 32-row block and blocks separately. Both derive the same
head_rows = blocks × 32 and both keep the four-condition narrowness (kpad_ % TILE_COLS == 0, shape[0] % per_block == 0, r >= NV, r % TILE_ROWS == 0) that
keeps a tile grid from being read as a matrix.
The one wording point from my #1992 review applies here too, so whoever lands the survivor should fold it in: the comment says the padded rows "decode to
zero", but what the code guarantees is that they are never sampled — the logits vector stays NV long and the copy loop stops at NV. The first promise depends
on the importer having written zeros (true today, so state it as why); the second is enforced here and nowhere else. A comment claiming "decode to zero" invites the
next reader to look for a reader bug that cannot exist.
Unverified: the CodeLlama import itself (needs the pack and a built onebit) — the grid arithmetic is what I checked, and it agrees with #1992 independently:
32,016 % 32 = 16 → 32,032 rows = 1,001 tiles = {16016, 5120} with per_block = 16.
|
Duplicate: this PR is #1992's own newest commit, and the two versions of the fix are functionally identical. Both derive Which to keep: #1992, because it carries this commit plus the i8-arm documentation, the padded-head restore and the What I would keep from this one regardless: the comment is the better of the two. "the tile COUNT is shape[0] when the tensor is stored as [tiles, tile bytes] … and it is shape[0] / per_block when it is stored as a row grid. Gating on And the number that makes it checkable, from this PR's own comment: CodeLlama-7b stores |
|
VERDICT: CHANGES at this head — the fix is right and the forward change has no arm. Reverting the gate to the old behaviour leaves the suite green: - if (ltd->ndim >= 2 && kpad_ > 0 && kpad_ % Q4NX_TILE_COLS == 0) {
+ if (false) /* MUTANT: old NV-only gate */The three And this is the path where an arm matters most, because #1990's whole subject is padding: the reader is asked to accept two spellings of the same head (
The third is the one I would pin hardest: it is silent by construction, and the existing comment says the padding "decodes to zero and is never sampled", which is exactly the kind of claim an arm should hold. The ask, with the machinery already in the tree: a crafted container whose What I have no complaint about: the reasoning is correct — both spellings count tile-rows in |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 38b70f9 — a HOLD for duplicate reconciliation, not a defect. This is the second patch for the same defect, and I measured which copy is more accurate.
The duplicate: this PR is stacked on #1992 (it contains 7dcc361b7, "restore the reader's padded-head block") and adds 38b70f905. But #1992 has since grown its own commit for the same fix — 0d629ec64, "the head's rows come from its TILE count", with the same derivation "Rows must come from shape[0] and the grid". So there are two implementations of one fix, in the same function, on two branches: merge one, not both.
And here is the fact that decides part of it — I read the container. Both comments describe CodeLlama's head tile tensor, and they disagree on its second dimension: #1992 says {16016, 5120}, this PR says [16016, 6144]. From ~/scratch-packs/pi-moe-npu/codellama7b-b8.q4nx/model.q4nx:
lm_head.weight I8 [16016, 6144]
6144 is right — this PR's comment is the accurate one, and #1992's is wrong by one tile stride. That matters beyond the comment: 6144 B/tile is what a reader will use to reason about the padded width, and 5120 is a Q4-era figure that would mis-derive it.
What I would fold, in one direction:
- keep #1992 as the merging PR — it has the device proof (
max|d| = 0, four steps, three repeats, block-width invariant atNPU_I8_HEAD_TB8192 and 16384) and theNPU_I8_HEAD_TBknob, and it is the older thread; - fold this PR's two contributions into it: the correct 6144 comment, and the observation that both spellings —
[rows, kpad_]and[tiles, tile bytes]— count tile-rows inshape[0], which is the sentence that makes the guard read as a rule instead of a special case; - keep #1992's
&& ltd->shape[0] > 0, which this PR's guard drops. A zero tile count is nonsense rather than a case to compute through, and the extra condition costs nothing.
If you would rather this branch be the one that merges (it is the one with the accurate comment), then it needs #1992's remaining commits — the padded-head restore and the knob — which is a rebase, not a cherry-pick, and I would rather not have two lanes re-litigating which is ahead.
Not reviewed further until that is settled, because reviewing two copies of one function is how a reviewer signs off on the copy that does not merge. Everything else I know about this fix is in my READY on #1992 at 0d629ec.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 38b70f9: READY
The padded-head arithmetic is derived from the container rather than assumed: per_block = kpad_ / Q4NX_TILE_COLS (tiles per 32-row block), and head_rows starts at the vocabulary before being raised for a padded head. The comment states the reason the head can carry more rows than the vocabulary — the same reason a padded geometry pads anything: the tile grid needs whole blocks.
This is the reader side of the same family as #1992 (padding the head to the q4_1 tile quantum) and #1999/#2001 (what to do when a geometry is not whole tiles). The three now agree on the rule: the packer pads, and the reader derives its row count from the tile grid instead of inferring it from vocab. That agreement is what was missing this afternoon, when a padded head was read with a vocabulary-sized row count and the tail of the image went unread.
Guarded on ltd->ndim >= 2 && kpad_ > 0 && kpad_ % Q4NX_TILE_COLS == 0, so an unpadded container keeps the old path rather than being divided by a tile width it does not have.
CI green.
|
VERDICT: READY at Reposted with the head in the line — the previous verdict named none, so the merge script would skip it. The change closes the case #1992 left open, and the That is the right fix for the right reason: gating on Keeping the standing property from #1992: this loosens the row-count test, not the arithmetic — a zero scale is still a malformed parameter block, and a geometry |
…uns it (review of #2026) The reviewer's blocking item was right and it is the failure this session keeps finding: `npu_pad_head_rows_test` sat inside `if(ONEBIT_NPU)` because it linked `onebit_npu_forward`, and **CI configures without the NPU runtime** -- so the arm was an assertion nothing ran. Their check, reproduced: ``` $ awk '…guards open at the registration…' -> open guard: 1520: if(ONEBIT_NPU) (closes at 1714) $ ninja npu_pad_head_rows_test -> unknown target, in an ONEBIT_NPU=OFF tree ``` So the rule moves to `npu/forward/include/pad_head_rows.h` -- header-only, inline, no library -- and the registration moves to the top level (line 343, outside every guard), next to the other pure rows. `q4nx_forward.cpp` includes the header instead of defining the function, and the test includes it instead of linking. Verified in **both** configurations, which is the point: ``` ONEBIT_NPU=OFF (~/wt/mtp-holdfix-build) ninja npu_pad_head_rows_test -> builds ./npu_pad_head_rows_test -> all padded-head row checks passed ONEBIT_NPU=ON (~/.cache/scratch-mtploop/build) ninja onebit -> builds (the forward compiles against the header) ``` That is the difference between "the arm exists" and "the arm runs where a regression would land". Refs #1990, #2020, #2026.
…at broke it (refs #1990, #2020) (#2026) * npu: the integer arm's operands come from the engine's packer, not a private re-quantisation The arm was merged in #1809 with a quantisation rule of its own: per 128-K group, int4 codes in [-8, 7]. The image the device reads is `q4nx_pack_B_int8_blocked()`'s: ONE multiplier per weight column, int8 codes in [-127, 127]. Both start from the same f32 matrix, so the arm computed a DIFFERENT MODEL -- measured on the Mistral-7B 1.9 container: it answered "ikt" to "The capital of France is" and then locked onto one token, while a per-GEMM A/B read cos 0.9999 against f32 on the same tensors. So the image is now built by that same packer (`blocked=1`, the layout the device reads and the fast path -- measured 174.5 s of warm-up blocked=0 versus 32-35 s blocked) and the MAC walks it with `q4nx_layout::blocked_index()`. Measured after, on current main: warm-up 35.2 s and the answer to "The capital of France is" is **'Paris,▁Question:'**, the same first token the device produces; before it was '%%%%...' / '▁/******/'. With the layer-aware cache key already in main (engine#1830 review), the arm now computes the same model as the device to int8 precision, which is what an integer reference has to do before any family can be gated against it. `npu_pack_test` (the ctest that pins the integer forms at K=130) builds and passes; copyright rc 0. * npu: keyed dumps settle it -- the device's weight scales are BIT-IDENTICAL to the arm's (384/384) Dumps were paired by index first and that was meaningless: the two sides reach the GEMM a different number of times (1024 arm dumps against 384 device dumps on the same request). Keyed by (layer, N, K, ordinal) they pair exactly. Result, one token, Mistral-7B 1.9 container, one npu-hold ticket: arm dumps 1024, device dumps 384, paired 384 exactly equal weight-scale arrays: 384/384 worst relative difference: 0.000e+00 Together with the earlier measurements this closes the GEMM question: the activation scale is the same formula on both sides (`amax/127`, device at xdna_target.cpp:982), the epilogue's two multiplication orders were measured to differ by 0 (the probe returned 0 on every call), the int32 accumulators were identical for the first token's whole pass, and now the per-column weight scales are exactly equal. So the device's GEMMs are bit-exact against this arm. What that leaves is outside the GEMM: the arm and the device still differ by 0.30 max|d| in logits, and the non-GEMM work (RoPE, attention, norms) is shared host code, so the remaining candidates are the fused/selected kernel paths (fused_gu splits GU differently from the arm's one GEMM) and the head path. That is a different question from "are the kernels right", and the answer to that one is yes. Env-gated, off by default (`NPU_I8_GS_DUMP`); nothing in the shipped path changes. * npu: the integer head, and the gate it unlocks -- device logits BIT-EXACT over four steps The head was the last place the host arm and the device computed the same tensor differently: in host mode `run_lm_head()` is a plain f32 matvec ("No I8Ctx is touched") while the device reads a packed quantised table (`lm_head_packed_`, `run_lm_head_gemv`). That was the whole of the remaining `max|d| = 0.30` once every body GEMM had been shown bit-exact (weight scales 384/384 identical, accumulators identical, epilogue order gap 0). The device packs its head tiles with `lctx.packB_into(...)` -> `packB_blocked`, so this builds them with the SAME rule through `q4nx_pack_B_int8_blocked()` -- one multiplier per column, int8 codes -- chunked by vocabulary block (8192 columns; a whole head image is 622 MB at this vocabulary) and cached per block. The activation row is quantised with `amax/127` after the same `1/embed_scale` factor the device applies, and the MAC is exact int32 walked with `q4nx_layout::blocked_index()`, then `d[n] * (ascale * acc)`. Measured, one npu-hold ticket, device vs this arm, logit dumps compared per step: step 0: argmax 3916 == 3916 cos 1.000000 max|d| 0 step 1: argmax 278 == 278 cos 1.000000 max|d| 0 step 2: argmax 28725 == 28725 cos 1.000000 max|d| 0 step 3: argmax 22478 == 22478 cos 1.000000 max|d| 0 BIT-EXACT over 4 step(s) at max|d| tolerance 0.0 That is TNPU-ALL section 4's bar met literally for this family rather than read down to "at the accumulator": the device's arithmetic and the integer reference agree to the last bit, on every step, for a full request. * docs/npu: say what NPU_INFER_HOST_GEMM=i8 is for, and what it measured The integer arm was only discoverable from the code. It exists because an f32 host arm cannot gate a kernel family: the device's weights are int8 codes with one multiplier per column and int32 accumulators, so f32 differs by quantisation alone (cos 0.9960-0.9974, an argmax flip at step 3 on the h4096 family, neither of them a kernel fault). Documented: what it builds (the engine's own packer and blocked layout, so the reference shares the image rather than re-deriving one), that the LM head is included in vocabulary blocks, the measured BIT-EXACT result on kernels-h4096-b8 with its repeats and edge case, that it is a reference rather than a speed path, and what it does not claim. * docs/npu: the two-arm command sequence for the bit-exact gate A documented reference that nobody can run is a claim, not an instrument: the section now carries the two forward-serve invocations, the dump naming scheme, and the three-step discipline (compare per position, repeat the device arm for determinism, add an edge case). * npu-pack: the h4096 36-layer fconst row, so the Qwen3-8B pack imports at all `kFconst19Rules` held the 32-layer h4096 row and nothing for 36 layers, so `npu-pack info` refused `amd/Qwen3-8B_rai_1.9.0_npu` by design: "its block shape has not been measured for hidden 4096, 32 heads, 36 layers … the 1.8 pack of the same model is what establishes it". Measured now, with tools/fconst19_measure.py against amd/Qwen3-8B_rai_1.8.0_npu_16K: {4096, 32, 36, 12608, 5, 1, 2297856} 26,104 of 3,756,627,264 bytes differ (6.95e-06 of the file) -- content, not layout Every field but the layer count is identical to the 32-layer row (prefix 12,608, five tiles of 2,297,856, rotation 1), which is the first row here where the layer count moves nothing but the number of blocks: the h2048 pair moves both the prefix and the division with it (4,160/9 tiles at 36 layers against 8,512/3 at 24), so this row is measured rather than inferred from that one. The consequence beyond this pack: a kernel family and an fconst rule row are not the same partition. One family can span several rows, so each new pack of a family may cost a measurement before it costs a gate. Verified end to end: the import now completes -- 5,760/5,760 .fconst tiles survive decode->encode byte for byte, 144 attention projections plus 108 MLP projections with fp32 scale/zero exact for 100.00% of groups, 8 full_i8 ELFs linked from kernels-h4096-im12288-b8. * npu-pack test: the new 36-layer row goes in BOTH places, as the guard's own comment requires CI caught the omission and said exactly why: "fconst19 rule: the table has exactly the 11 measured rows (12); a dropped row is a silent un-measure of a whole class, an added one must be added here too". The expected rows are literals copied from the measured table rather than read back from it, so that a wrong digit or a dropped row fails -- which is the arm working, not a nuisance. Added {4096, 32, 36, 12608, 5, 1, 2297856} to it, and the arm now passes. * npu-pack: pad the head's rows to the q4_1 tile quantum, so a 32,016-deep vocabulary imports CodeLlama-7b-Instruct's head is 32,016 rows and q4_1 tiles hold 32-row multiples, so the whole import refused with "q4_1 tiles need N % 32 == 0" -- a head the grid cannot hold, not a head that is wrong. The projections already had this rule (the pack's own padding stays, and the .fconst tail carries ceil(V/64)*64 rows: Hunyuan-0.5B has 120,832 for 120,818), but the graph-weight head path had no padding at all. pad_rows_to_tile_quantum() pads with rows that decode to zero (code 0, scale 0, zero 0 -- the quantised value is code*scale + zero, so a zeroed group is 0.0, not a weight) and LEAVES vocab_size at the vocabulary's own number, so the padding is never addressed. That invisibility is the part a test has to pin: widening N without it would put two plausible extra tokens at the end of every distribution. Measured on the pack that found it: the import now completes -- 387 tensors, 806,544 q4_1 tiles, container read back through q4nx::dequant_tile with rel_err 0.000e+00 and max|diff| 0.000e+00, 8 full_i8 ELFs linked from kernels-h4096-mha-b8 (the 4096x12288 family TNPU-ALL W3b asks to verify end to end). * npu: a padded head is only invisible if the READER agrees -- scale 1.0, and dequant on the tensor's grid The first gate run on CodeLlama refused with "lm_head dequant failed", and it was right twice over. 1. The padding wrote SCALE 0 for its rows. Arithmetically code 0 * 0 + 0 = 0.0, but a zero scale is not a zero weight to a reader -- it is a malformed parameter block, and the dequant refuses it. The test asserted exactly the wrong property (scale == 0.0f), which is how a wrong assumption gets to look pinned. Both now say scale 1.0, with the reason written next to them. 2. The forward asked for exactly NV rows, and NV here is the VOCABULARY (32,016), which is not a multiple of the tile's 32 rows -- so dequant_any() refused a head that was perfectly well formed. The container's vocab_size is the vocabulary; the tensor carries the padding; the reader has to dequantise on the tensor's own grid and keep only the vocabulary's rows. That is the same convention the .fconst path already relies on (Hunyuan-0.5B: 120,832 rows for 120,818), and it was simply not implemented on the graph-weight path. The extra rows decode to zero and are never sampled: the logits vector stays NV long. So this commit is importer + reader, and the PR's claim is only true with both: "the padding is invisible to the vocabulary" is a property of the pair, not of the writer. * npu: the integer arm reads a padded activation row through the pad, so a non-256-aligned IM gates host_gemm_i8() read its activation row with a[k] for k < K, and K is the PADDED design width. chatglm3-6b's IM is 13,696 and its D design is K = 13,824, so the arm read 128 floats past the caller's row -- into the next row's data, or past the buffer on the last one. Measured before the fix on kernels-h4096-chatglm-b8: cos 0.40 -> -0.003 -> 0.06 -> 0.005 against the device, argmax unrelated, while the f32 arm tracked the device at 0.985-0.988. After: max|d| = 0 on all four steps, cos 1.000000. The f32 leg has always padded -- q4nx_host_gemm_f32_padded() "owns the pad-and-read pair so neither leg can be handed an unpadded A", a comment that records a review catching exactly this -- and the integer leg, written later, did not. One lambda now serves both loops (amax and quantise), so the two cannot drift again. Why this looks like three unrelated mysteries until it is one: the families whose shapes are all 256-aligned were bit-exact (Mistral, Qwen3-8B), the one with a padded K was wildly wrong (chatglm), and the one with no padding but a residual (CodeLlama) is a different, smaller defect. This is the first hybrid pack with a non-aligned IM. * npu: restore the reader's padded-head block, which the merge dropped, and make the head block width a knob The merge that took main's unpadded-image work wholesale over q4nx_forward.cpp also took main's head dequant, which asks for exactly NV rows -- so the padded-head reader (10 references, reduced to 3) was silently lost and CodeLlama went back to "lm_head dequant failed" on the next run. Restored verbatim from 8932ac0. That is the second time a resolve-to-main has cost a fix on this file, which is why the block now carries its own measurement in the comment. Also: NPU_I8_HEAD_TB (default 8192, multiple of 128). CodeLlama's residual is confined to exactly sixteen logit positions starting at 16384, identical at every step -- a WEIGHT difference localised at a block edge, not accumulation drift -- and 16384 is where this arm's first head block ends. Moving the block width decides whether the chunking or the device's head read is responsible, in one ticket instead of an argument. * npu: the padded-head reader counts tile-rows in shape[0], so a [tiles, tile bytes] head dequantises (refs #1990, #1992) `q4nx_forward.cpp`'s padded-head path (engine#1990) gated on `ltd->shape[1] == kpad_` before deriving `head_rows` from the tensor's grid. A container that stores the head as `[tiles, tile bytes]` -- which is what the importer writes, e.g. CodeLlama-7b's `I8 [16016, 6144]` for a 32,032-row head at `kpad_` 4,096 -- fails that test, so `head_rows` stayed at `NV` (32,016) and `dequant_any()` refused the tensor: `lm_head dequant failed`, on a head that is exactly what the importer had just been asked to pad and write. Both spellings count **tile-rows** in `shape[0]` (`[rows, kpad_]` and `[tiles, tile bytes]`), so the number of 32-row blocks is `shape[0] / per_block` either way -- only the row width in `shape[1]` differs. The gate is now just "the block count divides evenly", and the padded rows are decoded and dropped as before (`head_rows != NV` copies only `NV` rows out). ## Measured end to end, on the pack this is for `amd/CodeLlama-7b-Instruct-hf-onnx-ryzenai-hybrid` -- the **first MHA 32/32 member of the 4096 family** (H 4096, QKV 12,288, GU 22,016, D 11,008) -- imported with `--weight-layout b8` (387 tensors, 806,544 q4_1 tiles, head padded 32,016 → 32,032, `vocab_size` untouched), kernels `~/ogaload/oga13/kernels-h4096-mha-b8`: ``` before: 1bit forward-serve: forward init failed: lm_head dequant failed after : 1bit forward-serve: row-cl7b ready on http://127.0.0.1 (full ELF) [npu-target=xdna] "The capital of France is" -> ' Paris. The capital of France is Paris. …' "The first element in the periodic table is" -> ' hydrogen, which is the lightest element.' "2 + 2 =" -> ' 6\n # 1 + 2 + 3 + 4 = 10\n …' (a code model, continuing a list) 15 prompt tokens, ttft 3.0 s; 64 tokens in 66,735 ms -> 0.96 tok/s ``` 0.96 tok/s is a *measurement*, not a pass -- the dx bar is 9.2 tok/s, so the row reads `OOB | 0.96 tok/s` like the other 7B rows, and the point of this PR is that the pack reaches `ready` and answers at all. Refs #1990, #1992 (which pads the head at import; this is the reader half), #1503. * npu: expose the padded-head row arithmetic and arm it on the shape that broke it (refs #1990, #2020) The reader fix in #2020 was arithmetic in the middle of the head-loading path, which is exactly the kind of code that cannot be tested without a container -- and a container is what CI does not have. So the rule is now a pure function: ```c++ int padded_head_rows(int64_t shape0, int64_t shape1, int kpad, int NV); ``` A container stores the head either as `[rows, kpad_]` or as `[tiles, tile bytes]`; both count TILE-ROWS in `shape[0]`, so the number of 32-row blocks is `shape[0] / (kpad / 256)` either way, and the vocabulary need not be a multiple of 32 (CodeLlama-7b: 32,016 -> padded 32,032). The caller decodes that many rows and keeps `NV`, because the padded rows decode to zero. `tests/npu_pad_head_rows_test.cpp` drives it on eight cases, all without a container or a device: * the shape that broke it -- `16016 x 6144`, kpad 4096, NV 32,016 -> **32,032**, whole tiles; * the same tensor in the `[rows, kpad_]` spelling -> the same answer, so the two spellings cannot disagree; * an exactly-aligned vocabulary -> NV (no padding invented); * a kpad that is not whole tile columns, a grid smaller than NV, and a partial block -> NV, so the caller refuses rather than reads past the tensor or invents rows; * NV <= 0 -> passed through, not invented. Mutation: making the invalid-argument branch `return 1` instead of `NV` fails two of the eight. `all padded-head row checks passed`; registered with `add_dependencies(onebit …)` so the target is built by the CI command that runs (engine#1858). Refs #1990, #2020, #1503. * npu: the padded-head arm moves out of the NPU guard, so CI actually runs it (review of #2026) The reviewer's blocking item was right and it is the failure this session keeps finding: `npu_pad_head_rows_test` sat inside `if(ONEBIT_NPU)` because it linked `onebit_npu_forward`, and **CI configures without the NPU runtime** -- so the arm was an assertion nothing ran. Their check, reproduced: ``` $ awk '…guards open at the registration…' -> open guard: 1520: if(ONEBIT_NPU) (closes at 1714) $ ninja npu_pad_head_rows_test -> unknown target, in an ONEBIT_NPU=OFF tree ``` So the rule moves to `npu/forward/include/pad_head_rows.h` -- header-only, inline, no library -- and the registration moves to the top level (line 343, outside every guard), next to the other pure rows. `q4nx_forward.cpp` includes the header instead of defining the function, and the test includes it instead of linking. Verified in **both** configurations, which is the point: ``` ONEBIT_NPU=OFF (~/wt/mtp-holdfix-build) ninja npu_pad_head_rows_test -> builds ./npu_pad_head_rows_test -> all padded-head row checks passed ONEBIT_NPU=ON (~/.cache/scratch-mtploop/build) ninja onebit -> builds (the forward compiles against the header) ``` That is the difference between "the arm exists" and "the arm runs where a regression would land". Refs #1990, #2020, #2026. * npu: the head's stored rows are read from its whole shape: three spellings, one bound (answers CHANGES at 63a47be; engine#1788) padded_head_rows() ignored shape[1] and read shape[0] as a tile count, so a head really stored [rows, kpad] would have been asked for twice its rows (review of #2026 at 63a47be), and FastFlowLM's own three-dimensional spelling [row tiles, column tiles, bytes] was refused: MiniCPM-V 4.7 1B's I8 [7753, 4, 8704] head (248,096 rows for a vocabulary of 248,094) failed with 'lm_head dequant failed'. The rule now takes the shape and says where the row count is in each spelling, and one bound holds for all of them: padding only reaches the next 32-row boundary, so an answer outside [NV, NV + 32) is not this tensor's row count. The arm uses the real row-spelled shape (32032, 4096) and both MiniCPM heads. A duplicated log line is removed. Measured: with the d3-lite kernel set (the same geometry), MiniCPM-V 4.7 1B's text container now reaches ready and answers ' Paris.' on the host arm and on the device. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * npu: the padded-head bound's lower end is pinned in all three spellings (answers CHANGES at 3a1b03a) The review showed that deleting 'rows >= NV' from the bound left every arm green: the existing short-grid arm was declined for not being whole blocks, not for being short. Three arms now feed whole 32-row grids of 1,024 rows for a vocabulary of 32,016, one per spelling ([512, 6144], [32, 16, 6144], [1024, 4096]); only the lower bound declines them. rows >= NV deleted: 3 FAIL (got 1024, want 32016) rows < NV + 32 deleted: 2 FAIL unmodified: all padded-head row checks passed (19) Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: t <t@e.com> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
|
VERDICT: READY at #2026 has merged, which makes this PR redundant in every file it touches — close it rather than resolving its conflict. This is the duplicate I flagged when I reviewed #2026: #2020 is a strict subset of it, measured at the time by #2026 merged (
The conflict this PR now reports ( Recommendation: close unmerged. Nothing in this branch is worth carrying forward — the only content unique to it was the absence of the test and the registration. If anything here is worth keeping it is the reviewer trail: this pair is the eighth duplicate cluster found by diffing content rather than titles, and #2026 superseded it by adding |
…whose work is already on base) is invisible to a diffstat (#2308) Two shapes cost this fleet a round trip each tonight, and both look exactly like a PR writing its own code: * #2286's branch carried #2287's and #2252's files byte-identical, so merging it would have landed two other PRs' merge decisions inside its own; * #2020 was a md5-identical copy of #2026, and #2086 re-added files #2045 had already merged -- the work was on base, so the new PR was a fourth copy of something that landed an hour earlier. A diffstat cannot separate either from a PR that wrote the file. Blob equality can: pr_carry_check.py <repo> <ref>... [--base origin/main] CARRIED a path 2+ refs add with the same bytes and base does not have LANDED a path a ref changes to the blob base already has Read-only: no network (caller fetches into refs), no checkout, no index writes, so it is safe in a worktree someone is using. Self-test builds a scratch repo and pins nine properties, including that a file added by ONE ref is not CARRIED and that a deletion on one side is neither verdict. Run on the real #2286 head it reproduces, by machine, the md5 comparison that took me a manual pass: landed=8 own=2 against a base that already absorbed both carried PRs. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
q4nx_forward.cpp's padded-head path (engine#1990) gated onltd->shape[1] == kpad_before derivinghead_rowsfrom the tensor's grid. A container that stores the head as[tiles, tile bytes]— which is what the importer writes, e.g. CodeLlama-7b'sI8 [16016, 6144]for a 32,032-row head atkpad_4,096 — fails that test, sohead_rowsstayed atNV(32,016) anddequant_any()refused the tensor:lm_head dequant failed, on a head that is exactly what the importer had just been asked to pad and write.Both spellings count tile-rows in
shape[0]([rows, kpad_]and[tiles, tile bytes]), so the number of 32-row blocks isshape[0] / per_blockeither way — only the row width inshape[1]differs. The gate is now just "the block count divides evenly", and the padded rows are decoded and dropped as before (head_rows != NVcopies onlyNVrows out).Measured end to end, on the pack this is for
amd/CodeLlama-7b-Instruct-hf-onnx-ryzenai-hybrid— the first MHA 32/32 member of the 4096 family (H 4096, QKV 12,288, GU 22,016, D 11,008) — imported with--weight-layout b8(387 tensors, 806,544 q4_1 tiles, head padded 32,016 → 32,032,vocab_sizeuntouched), kernels~/ogaload/oga13/kernels-h4096-mha-b8:0.96 tok/s is a measurement, not a pass — the dx bar is 9.2 tok/s, so the row reads
OOB | 0.96 tok/slike the other 7B rows. The point of this PR is that the pack reachesreadyand answers at all, which it did not before.What this is not
It is not the arm for the padding itself: the importer-side property (padded rows decode to 0, vocabulary rows untouched) is pinned in #1992's test. What is missing on both sides is a runnable check that a container whose vocabulary is not a multiple of the tile quantum reaches
ready— that needs a container in CI, which is the same gapbatch_check_state_testhas (#1946). Say the word and I will take that as its own issue rather than bolt it on here.Refs #1990, #1992 (which pads the head at import; this is the reader half), #1503.