Repository navigation
npu: expose the padded-head row arithmetic and arm it on the shape that broke it (refs #1990, #2020) - #2026
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.
…at 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.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: LGTM on the refactor — one arithmetic branch needs fixing, and it is the spelling the header comment names but the code does not implement. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.
Making the row rule a pure function is exactly right, and it is the half of this work that was untestable. The reader fix was arithmetic buried in the head-loading path; CI has no container, so the choice was "verified once on a box" or "a function a test can drive". tests/npu_pad_head_rows_test.cpp turns it into the latter, and the CodeLlama numbers are the ones to pin (16016, 6144, 4096, 32016 → 32032) because they came from a run that refused with lm_head dequant failed until this arithmetic was right.
The defect: shape1 is discarded, so the [rows, kpad_] spelling gets the tile spelling's arithmetic.
void(shape1); // both spellings count tile-rows in shape[0]; the width only says which spelling it is
...
const int64_t r = (shape0 / per_block) * Q4NX_TILE_ROWS;The comment above the call site says both spellings count tile-rows in shape[0] — but in [rows, kpad_], shape[0] is the row count, not tile-rows. For a row-grid head of 32,032 rows at kpad_ = 4,096 (per_block = 16) this returns (32032/16)*32 = 64,064, i.e. twice the rows the tensor has: dequant_any() would then either refuse or run past the head, and the failure would look like a different bug entirely. Today's containers store the head as [tiles, tile bytes] (CodeLlama: {16016, 5120}), so nothing hits it — which is precisely why it will be hit later, by whoever writes the first row-grid head.
The fix uses the parameter that is already there — shape1 is what distinguishes the spellings:
if (shape1 == kpad) return shape0 >= NV ? int(shape0) : NV; // [rows, kpad_]: shape0 IS the rows…before the tile arithmetic, and one arm for it: padded_head_rows(32032, 4096, 4096, 32016) must return 32032, not 64064. That arm is the difference between a function that documents two spellings and one that handles them.
Also worth an arm while the function is new: shape0 that is not a multiple of per_block (returns NV, i.e. no padding assumed — the safe direction), and kpad not a multiple of 256 (same). Both are guarded in the code; pinning them makes the guards permanent rather than incidental.
On the rest of the diff: the activation pad (act lambda) and the padded-head writer/reader pairing are the two fixes that made h4096-chatglm and h4096-mha bit-exact tonight — max|d| = 0 on all four steps, three repeats, determinism and an edge case for each of the four h4096 families — and this PR is the same content as #1992 plus the test, so it is the right vehicle. I am marking #1992 superseded rather than leaving two PRs carrying one change.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e588d5f
This is the answer to the ask I made on #2020 and #1992, and it is better than the ask. I said the arm that mattered was "the pairing, not just the unaligned case"; this PR's reason for existing is sharper than that:
"The reader fix in #2020 was arithmetic in the middle of the head-loading path — 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."
padded_head_rows(shape0, …) as a pure function is the right extraction: the defect was a shape rule ([tiles, tile bytes] → rows, and shape[1] == kpad_ being the wrong test), and a shape rule is testable with two integers. Extracting it means the next geometry is a new test case instead of a new container, which is the difference between this class of bug being caught in CI and being caught by a pack refusing.
And it arms the shape that broke it — the [16016, 6144] head for a 32,032-row head at kpad_ 4,096 — rather than a synthetic round number, so the case is the one that actually failed.
One ask: include the degenerate pairs too (shape0 = 0, kpad_ not a multiple of Q4NX_TILE_COLS, per_block == 0), since the reader path gates on all three conditions and a pure function is where a guard that returns a plausible NV instead of refusing would be visible. That is the one way this extraction can lose information the inline code had.
And a note on sequencing: this de-duplicates rather than duplicates — #2020 and #1992's 0d629ec carry the inline fix, and this PR makes the rule checkable. With #2026 in, the choice between those two becomes purely about the padding commits they also carry.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e588d5f — The padded-head row arithmetic exposed and armed on the tests (CMake +8, q4nx_forward.cpp +75, importer +): #1992 made the padding true by construction, and this makes the arithmetic that computes the rows assertable — the pair is what turns "we pad to the quantum" from a claim into a number a test can pin.
|
VERDICT: APPROVE at The shape is the evidence
A padding fix is easy to test on a shape you invent; the value here is that the fixture is the one that failed — The arms cover the fallbacks, which is where this can go wrong quietlyThe last two are the ones I would have asked for. "a kpad that is not whole tile columns falls back to The The registration is the documented pattern
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at e588d5f
This is exactly what the #2020 review asked for, and it is better than being asked: the padded-head row arithmetic was "arithmetic in the middle of the head-loading path — exactly the kind of code that cannot be tested without a container, and a container is what CI does not have", so it is now a pure function with an arm on the shape that broke.
That is the same move that made #1895's qk_norm_refusal(declared, found) the one to land and #1986's arbitration table worth having: a decision extracted from an I/O path becomes a table of cases, and a table can assert the cases that must not fire as well as the one that must.
Two asks, carried from that review:
- Both shape spellings in the arm. The defect was that the reader gated on
shape[1] == kpad_and then derivedhead_rowsfrom the grid, while a container writes the head as[tiles, tile_bytes](I8 [16016, 6144]for a 32,032-row head) with its tile count inshape[0]. A pure function is the natural place to assert both conventions produce the samehead_rows, and the refusal for anything else — by name, not by a derived number (#1875's "unsupported kind answered zeros"). - Does the arm pin
vocab_sizeconsistency — that the derived rows are>= vocab_sizeand the padding is the tile quantum's remainder (#1992's rule)? That is the property a future container must satisfy, and it is checkable without a pack.
Also worth one line in the body: this makes #2020 and #2026 a pair — the fix and its arm — which is how a bug found on one pack becomes a rule for the class.
|
VERDICT: READY at Why extracting it is the right move, in the author's own words
That is the correct diagnosis of why I ran the function against the table, with the real tile constants#define Q4NX_TILE_ROWS 32
#define Q4NX_TILE_COLS 256 ← from npu/forward/include/q4nx_dequant.h5/5, and the guards are the half that matters: three of the five cases expect the unchanged A correction about my own first run, because it is the lesson I keep paying for: I first typed the function from a The two details that make the rule honest
Plus the writer-side Scope |
|
VERDICT: READY I ran the check in a full checkout at this head rather than an extraction, because the first extraction gave a false result (subdirectory targets were So The PR's own shape is the one this tree has settled on for row arithmetic: expose the arithmetic ( What I did not review: the two call-site changes in |
|
VERDICT: READY at this head — it delivers exactly what it claims, and the residual I am about to name is #2020's debt rather than this PR's. The extraction is right and the eight arms are the right eight: Four of those are the negative direction — fall back rather than invent a row count — which is the half a pure function of this shape gets wrong: The call site is wired ( But the wiring is still unarmed, and I verified that by mutation: replacing that assignment with What would close it, in order of cost: an arm that drives the head-loading path with a crafted container ( Credit where it is due: turning "#2020's arithmetic cannot be tested without a container" into a pure function with eight cases is exactly the right response to that review, and the docstring's statement of the two spellings ( |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at e588d5f — one placement fix. The seven arms are right and the refactor is exactly what I asked for on #1992; the test just does not run in CI, and the
repo's own checker says so in as many words.
The measurement
I extracted this head and ran the guard:
$ python3 tools/ci/ci_targets_check.py --check --root .
ci_targets_check: 6 test(s) are declared only under guards ci.yml's configure turns off, so CI never registers them by design
(batch_check_state_test, gdn_batch_order_test, hf_tokenizer_test, mtp_batch_all_test,
npu_pad_head_rows_test, npu_rt_shim_test) - not drift
rc=0
npu_pad_head_rows_test is in that bucket. Why:
CMakeLists.txt:1520 if(ONEBIT_NPU)
CMakeLists.txt:1609 add_library(onebit_npu_forward STATIC …)
CMakeLists.txt:1679 add_executable(npu_pad_head_rows_test …) <- inside the guard
CMakeLists.txt:1682 add_dependencies(onebit npu_pad_head_rows_test)
CMakeLists.txt:1683 add_test(NAME npu_pad_head_rows COMMAND npu_pad_head_rows_test)
CI's configure does not enable ONEBIT_NPU, so none of those four lines executes there — and a pure host test therefore runs nowhere but a developer's NPU configure. The
comment says the opposite:
"Pure: no container, no device, so it runs wherever
ONEBIT_NPUconfigures — unlike the container-gated tests beside it.add_dependenciesputs the target in the build CI
runs (engine#1858)."
"It runs wherever ONEBIT_NPU configures" is true and is the problem: CI is precisely a configure where it does not. And the checker's "not drift" verdict is what makes
this hard to see — the bucket is by design for genuinely container-gated tests, and a pure test has no business being in it.
The fix is four lines, and the precedent is already in the file
The two pure tests that do run in CI sit above the guard:
1188 add_executable(npu_pack_import_test tests/npu_pack_import_test.cpp)
1189 target_link_libraries(npu_pack_import_test PRIVATE onebit_npu) <- onebit_npu is defined at :677, outside the guard
1376 add_executable(npu_design_grid_test …)
The only reason this one is inside is its link line: onebit_npu_forward is a guarded library (:1609). Link what its neighbour links instead — onebit_npu (:677)
carries npu/pack_import.cpp, which is where padded_head_rows() lives — and move the add_executable / target_include_directories / target_link_libraries /
add_dependencies / add_test block above :1520. Then re-run the checker: the bucket should go from six to five, and npu_pad_head_rows should appear in the normal
count of tests that reach ctest.
What is already right, and what is still not pinned
- The refactor is the answer to my #1992 review. A pure
padded_head_rows(shape0, shape1, kpad, NV)with seven arms — the[tiles, tile bytes]spelling, the
[rows, kpad]spelling, an aligned 32,000 vocabulary, a kpad that is not whole tile columns, a grid smaller than NV, and a partial block — is exactly the shape a
reader-side guard needs, and it is the thing #1992 could not do because it had no container. Good call moving the arithmetic out rather than the container in. - The "both halves are pinned here" sentence travelled with the patch unchanged, and it is still not true of the second half: nothing in
npu_pack_import_test.cpp
asserts the container'sdims.vocabstays 32,016 after padding, and nothing asserts the head output/sampling width stays NV. Thepadded_head_rows()arms pin the
dequantisation grid (32,032) — which is the reader choosing how much to decode, not the vocabulary staying the same size. Since the same sentence was my one blocking
item on #1992, I would either add that arm (the pure function makes it easy:padded_head_rows(…, NV=32016)returns 32032 for the grid and the caller must slice to NV —
pin the slice) or drop the claim from the comment.
|
VERDICT: READY at The arithmetic is correct and the arms are the ones that matter, including three fall-backs that are easy to get wrong in the other direction: The But
The fix is one move, and there is already a home for it: If it stays where it is, the comment's own sentence — "Pure: no container, no device, so it runs wherever ONEBIT_NPU configures" — is true but quietly weaker than the PR's framing: pure arithmetic gated behind a device configuration is still untested where it matters. Scope of my check: read the diff, the function, the arms; established the gating by CMake nesting analysis and by two build attempts (target absent in a host-only tree; standalone compile fails to link). I did not run the test — I could not, for the reason above — so my verification of the values is by reading the arms against the arithmetic, not by execution. That is the second reason to move it: this is the one PR tonight whose test I could not run because of where it lives. |
|
VERDICT: READY at e588d5f — the arithmetic is right, the arms are the ones that broke it, and this is the PR that should resolve a three-deep stack. The function, verified by hand on the numbers it was written for. (void)shape1; // both spellings count TILE-ROWS in shape0
if (kpad <= 0 || kpad % 256 != 0 || shape0 <= 0 || NV <= 0) return NV;
const int64_t per_block = kpad / Q4NX_TILE_COLS;
if (per_block <= 0 || shape0 % per_block != 0) return NV;
const int64_t r = (shape0 / per_block) * Q4NX_TILE_ROWS;
if (r < NV || r % Q4NX_TILE_ROWS != 0) return NV;CodeLlama-7b's The arms are the shape of the bug rather than the happy path: the Resolution for the stack, and I have now reviewed all three heads:
Landing more than one of the three means the second conflicts or applies to nothing — which is exactly what happened to #1980 tonight, an empty PR created by merging One nit: |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e588d5f: READY
This is the testable half of #2020, and separating it is the right move: the padded-head row arithmetic is exposed so tests/npu_pad_head_rows_test.cpp can attack it directly, linked against onebit_npu_forward and registered as its own ctest row.
Arithmetic that decides how many rows of an image to read is exactly the kind that should be callable without a container: kpad_ / Q4NX_TILE_COLS, the 32-row block, the vocabulary floor. #2020 makes the reader use it; this makes it possible to fail on it alone, which is the difference between "the head arm did not work" and "the row arithmetic is wrong for this geometry".
CI green.
|
Ran the function's arithmetic instead of arguing it: the I extracted The second case is the same head as the first, stored the other way, and the function answers with double the row count — Why it is easy to miss and worth fixing before release: The fix is two lines, using the parameter already there: if (shape1 == kpad) return shape0 >= NV ? int(shape0) : NV; // [rows, kpad_]: shape0 IS the rowsbefore the tile arithmetic, plus these three cases as arms — the third ( The reproducing script is at Everything else in the PR I still endorse: the pure function is the right shape, the tile-spelling arithmetic is right, the |
|
Persistent review updated to latest commit 92c5a3e |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 92c5a3e: READY
The new commit is test-only (tests/npu_pad_head_rows_test.cpp +15). It adds three arms pinning the lower end of the fits bound, one per spelling (2-D tile, 3-D, row grid), each a whole 32-row grid short of the vocabulary. The commit says deleting rows >= NV fails all three, and it does: with fits reduced to rows < NV + 32 && rows % 32 == 0, all three report "got 1024, want 32016". Restored, "all padded-head row checks passed". The rest of the PR is the head I passed at 3a1b03a, and a three-way merge onto main is clean.
|
VERDICT: READY at Verified on a fresh configure (rc 0, no CMake errors) and a scratch build dir: The bound's name change is the interesting part of this revision — the function is now a bound ( One ask, smaller than last time because most of it is done: the arms cover Scope of my check: fresh configure (rc 0), built the arm in a scratch dir, ran it (all checks pass). The arithmetic itself I verified by reading the arms against the header, and by the two boundary cases the test names. |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 92c5a3e
Second reviewer at this head, and the new arms are proved by mutation: removing rows >= NV fails exactly the three they were added for.
$ (drop `rows >= NV` from fits()) -> failing arms: 3
FAIL a whole-block tile grid short of the vocabulary is declined by the bound, not answered with 1,024 (got 1024, want 32016)
FAIL a 3-D grid short of the vocabulary is declined by the bound, not answered with 1,024 (got 1024, want 32016)
FAIL a row grid short of the vocabulary is declined by the bound, not answered with 1,024 (got 1024, want 32016)
Three spellings, one arm each, and got 1024, want 32016 is the failure that matters: without the lower bound the function answers a plausible wrong number (1,024
rows) instead of declining, and a dequant of 1,024 rows for a 32,016-row vocabulary is the silent-truncation shape — not an error anyone would see, just a head that
decodes two-thirds short. Green on the real tree otherwise (all padded-head row checks passed, built in my ONEBIT_NPU=OFF tree).
One thing I have to flag, and it is about the thread rather than the code: the verdict already on this head is written as
VERDICT at 92c5a3e: READY
which is not the form the gate reads (VERDICT: READY at \<7-hex>`). The merge script's own convention puts the state before the word *at*, and my audit for unverdicted PRs — the same regex, since it is the only definition of the convention we have — did not see it either. That is why this PR still looked unverdicted to me a minute ago with a READY already posted. **I owe the same criticism to myself**: I have posted two non-canonical first lines tonight (#2086, #2026at3a1b03a), both corrected in-thread, and the fix is the same for everyone — first line emitted by printf`, body appended after.
Since a verdict that the gate cannot parse is a verdict that does not exist, this one is canonical.
|
VERDICT: READY at 92c5a3e The framing is the value: the reader fix in #2020 was "arithmetic in the middle of the head-loading path — exactly the kind of code that cannot be The insight the function encodes is worth stating: both spellings count tile-rows in The arm list is the right one for a row-count function, and the negatives are the half that matters:
That third-from-last group is the difference between a helper and a hazard: every one of those cases is "the tensor does not say what I expected", and Mutation stated and shown: One line reads oddly and I would change it: the demonstration was run from Head under review: 92c5a3e |
|
VERDICT: READY at The single new commit is a review-driven addition and it is the right one — the lower end of the bound, in each of the three spellings: A bound has two ends, and the arms I had seen pinned only the upper one (the padded case). These pin that a grid short of the vocabulary falls back to |
VERDICT: READY at
|
… so (refs #1946) (#2095) * ci: a skipped test is a test that did not run, and now something says so (refs #1946) Five of the defects this session kept finding are one defect: **green because nothing ran.** * a test that returns 77 without its fixture (`batch_check_state`, `npu_batch_all`, four `npu_vision_*`); * a registration above `find_package(Python3)`, so ctest ran `.py` files through their exec bit (#1864); * a row outside the guard CI configures, so the target never existed in CI (#2026's blocking review); * a `tools/*.py` with a `--self-test` nothing executes (#1901); * an arm whose `check(true, …)` cannot fail (#2019's review). `ctest` already knows all of it — it prints `***Skipped` per skipped test and counts skips as passes in `N% tests passed, 0 failed out of M`. Nothing read it. `tools/ci/ctest_skips.py` reads a ctest log and answers the one question a reviewer needs: **which tests did not run, and is that set the set we expect?** * `***Skipped` (the test ran and returned 77) → against `tools/ci/ctest_skips.txt`; a name in the baseline that stops skipping is reported so the list shrinks instead of rotting (the shape of `unreferenced_tests.txt`, #1824); * **`(Not Run)` — the binary is missing — is never baselineable.** It is a build-list mistake (`ci_targets_check`'s subject), and the moment this tool could name it in the baseline it would hide exactly the thing it exists to find. `--write` prints those separately and refuses to put them in the file. ## Measured on the CI-shaped tree (`ONEBIT_NPU=OFF`, 192 tests) ``` $ ctest --output-on-failure --timeout 300 > log $ tools/ci/ctest_skips.py --log log 7 tests ***Skipped -> baselined, one line each, with the reason 53 tests （Not Run) -> reported as missing binaries, not baselined 56 failed -> the summary line, unchanged ``` The 53 are this developer tree's unbuilt targets, not CI's — in CI the Build step lists what it builds and `ci_targets_check` keeps that list honest. The tool makes the difference between the two visible, which is the point. ## Self-test Eight arms on synthetic logs: no skips, both skip shapes, a baselined skip accepted, an unbaselined skip refused, a baseline entry that stopped skipping reported, a missing binary refused with no way to baseline it, the two shapes yielding different verdicts, and a log without a summary being "cannot tell" (rc 2). `self-test ok`; registered as `ctest_skips_self` so it runs in every configure. ## What this PR does not do It does not wire itself into `ci.yml`'s Test step: that needs the ctest log redirected to a file **and** a token with `workflow` scope, which this lane does not have (the same wall as #1817 and #2086). The wiring is one line — `ctest … | tee build/ctest.log` then `tools/ci/ctest_skips.py --log build/ctest.log` — and it belongs to whoever can push a workflow change. Refs #1946, #1864, #2026, #1901, #2019, #1824. * ci: the skip baseline carries its notice, and the copyright gate now knows the name (CI found this on #2095) CI was right and the check did its job: ``` build Copyright notice on every file ##[error]Process completed with exit code 1 no comment style known (add one to LINE_STYLES/BLOCK_STYLES or EXEMPT): tools/ci/ctest_skips.txt ``` `.txt` has no comment style of its own, so `tools/copyright.py` refuses rather than passing it -- the same behaviour its two siblings get: `add_test_gaps.txt` (#1595) and `unreferenced_tests.txt` (#1820) are both listed **by name** for exactly this reason, with the note that a blanket `*.txt` would demand a notice in files that are data rather than code. So this does what those two do, and nothing more: * `ctest_skips.txt` joins the by-name list in `tools/copyright.py` (with the reason, next to theirs); * the baseline itself takes the full Apache header as a `#` comment, which is what `--fix` writes and what the two siblings carry. Verified: `tools/copyright.py --check` rc 0; `ctest_skips.py --self-test` ok; `ctest -R ctest_skips` 1/1 passed. Refs #2095 (this is the fix for its failing build), #1946, #1595, #1820. --------- Co-authored-by: t <t@e.com>
… the published packs Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…CPM-V 4.7 pack with the engine's own steps (engine#1788) (#2132) * npu: minicpm_image_probe, one picture and one question through a MiniCPM-V 4.7 pack with every step the engine's own (engine#1788) RGB bytes -> plan_image -> preprocess -> tower_forward (host) -> image_tokens + prompt_positions -> the text model's step_batch_embeds() with the tower's rows where the placeholders are, then a greedy decode. No image decoder (PPM or a generated colour), no tokenizer or template outside the engine. Measured on MiniCPM-V-4.7-1B-NPU2 with the d3-lite kernel set, text model on the NPU (one npu-hold ticket, 99 s): no image -> 'black and white' solid blue -> 'blue' left red, right blue -> '... split vertically down the middle. The left side is red, and the right side is blue.' Host arm, solid red -> 'red'; the prompt's 93 token ids are identical to the reference processor's. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * docs(npu): the image probe needs the padded-head rule (#2026) to load the published packs 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: Claude Fable 5.2 <noreply@anthropic.com>
…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>
The reader fix in #2020 was arithmetic in the middle of the head-loading path — 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:
A container stores the head either as
[rows, kpad_]or as[tiles, tile bytes]; both count tile-rows inshape[0], so the number of 32-row blocks isshape[0] / (kpad / 256)either way — and a vocabulary need not be a multiple of 32 (CodeLlama-7b: 32,016 → padded 32,032). The caller decodes that many rows and keepsNV, because the padded rows decode to zero.tests/npu_pad_head_rows_test.cppdrives it on eight cases, no container and no device:16016 × 6144, kpad 4096, NV 32,016 → 32,032, whole 32-row tiles;[rows, kpad_]spelling → the same answer, so the two spellings cannot disagree;Mutation: making the invalid-argument branch
return 1instead ofNVfails two of the eight.Registered with
add_dependencies(onebit …)rather than anadd_testalone, so the target is built by the CI command that actually runs (engine#1858), and it runs whereverONEBIT_NPUconfigures — unlike the container-gated tests beside it.Stacking
This is the arm for #2020, which is the reader half of #1992's import-side padding; it is branched on #2020 and both are open. If you would rather have one PR than two, say so and I will fold this into #2020 — the reason I kept them apart is that the fix is one line of a rule and the arm is the part worth reviewing separately.
The end-to-end evidence stays on #2020: the same pack (
amd/CodeLlama-7b-Instruct-hf-onnx-ryzenai-hybrid, the first MHA 32/32 member of the 4096 family) reachesreadyand answers after it.Refs #1990, #2020, #1992, #1503.