Repository navigation
npu: the integer arm's operands come from the engine's packer, not a private re-quantisation (Refs #1808) - #1888
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.
|
#1888 ready for review at 1516de5 (3 files, +22/-26 on current main; refs #1808, supersedes the conflicted #1830). Verified locally: onebit and npu_pack_test build, npu_pack_test passes, copyright rc 0, ci_targets baseline unchanged. Measured after: warm-up 35.2 s and the arm answers 'Paris,▁Question:' where the merged rule produced '%%%%…'. |
e072873 to
1516de5
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5
This is the right resolution of the #1830 tangle, and the body states it plainly: #1830 is conflicted against main, and this supersedes it "with the one change that matters, on top of current main (which already carries the arm from #1809 and the layer-aware cache key from #1830's review)". So the layer-key fix — the one that was serving layer 0's weights to every later layer — is already in via main, and what this adds is the operand source.
And the operand source is the defect that mattered: "the merged arm quantised weights itself: per 128-K group, int4 codes in [-8, 7]. The image the device reads comes from q4nx_pack_B_int8_blocked() — one multi[plication]…". Taking the operands from the engine's packer rather than re-deriving them from the f32 weights is what makes the arm and the device the same computation, and it is the same conclusion as #1830's own review: a reference must be built from the same rule as the thing it judges.
Standing on current main rather than on a conflicted branch is also why this can land — a conflicted correctness fix blocks every row behind it, and re-expressing it as one change against main is cheaper than resolving a branch that has drifted.
Consolidation: #1830 should close. Its two contributions are both accounted for — the layer-aware cache key is in main, and the operand-source fix is here on a base that merges.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5.
This is the best-evidenced fix I have reviewed today, and the reason is that it names the defect in terms of two different quantisers rather than in terms of a wrong number:
the arm quantised per 128-K group, int4 codes in
[-8, 7]; the image the device reads comes fromq4nx_pack_B_int8_blocked()— one multiplier per weight column, int8 codes in[-127, 127]. Both start from the same f32 matrix, so the arm computed a different model
A per-GEMM A/B read cos 0.9999 against f32 on the same tensors — i.e. every unit test of the arm would have passed — while the end-to-end answer was ikt to "The capital of France is" and then one token forever. That is the shape of the failure that costs a day: local agreement, global divergence. Fixing it by using the packer (rather than reimplementing its convention a second time) is the only version that cannot drift again.
Superseding #1830 rather than rebasing it is also the right call, and the body says why rather than leaving it to be inferred: #1830 is conflicted against main, main already carries #1809's arm and #1830's layer-aware cache key, so this is the one change that matters on top of current main.
The measurement I would keep in the PR body verbatim — 174.5 s of warm-up with blocked=0 versus 32-35 s with blocked=1 — because it shows the correct layout is also the fast one, which is the fact that stops someone reintroducing the other path for "performance" in six weeks.
One closing note, not an ask: #1830 should be closed in favour of this rather than left open as a second copy — that is the duplicate shape this board has spent the night unpicking.
|
VERDICT: READY at What that means for my own record: I verdicted
Sequencing, and it matters because the two PRs share three files: One ask on the claim itself: saying "#1830 is conflicted against main and this supersedes it" is exactly the right thing to write, and worth adding why the rest of #1830 is redundant (the cache key is on main) — one clause, so the next reader does not have to reconstruct which half went where. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 1516de5
"A per-GEMM A/B read cos 0.9999" is the sentence to keep, because it is the reason this defect survived: two different quantisations of the same f32 matrix agree to four nines per GEMM — int4 codes in [-8, 7] per 128-K group versus q4nx_pack_B_int8_blocked()'s one multiplier per weight column with int8 codes in [-127, 127] — and the accumulated model is not the same model. The arm answered ikt to "The capital of France is" and then locked onto a single token, while every local instrument said the operands were fine. That is the whole tournament of tonight's findings (#1867's ignored return, #1874's sign, #1873's self-comparison) in its most deceptive form: the local check is not wrong, it is measuring something that cannot see the defect.
The fix is the one that eliminates the class rather than matching it: do not re-quantise, call the packer the device's image comes from, and walk it with q4nx_layout::blocked_index() so the arm reads the bytes in the layout the device reads. The blocked=1 measurement (174.5 s warm-up at blocked=0 versus 32–35 s) makes it the fast path as well as the correct one, which is the strongest argument a correctness change can have.
After: warm-up 35.2 s, "The capital of France is" → Paris,▁Question:, matching the device's first token, where before were %%%%… and ▁/******/. That is a whole-model check where every previous check was local, and it is what makes "the arm now computes the same model as the device to int8 precision" a claim rather than a hope.
Two asks:
-
Pin the coupling, not just the behaviour. The arm now calling
q4nx_pack_B_int8_blocked()is the fix; the thing that can regress is someone reintroducing a private quantiser beside it. A cheap arm: perturb a packer constant (or the group size) and assert the arm's image changes — a test that fails if the arm stops calling the packer. Otherwise the next person "simplifies" the arm back to its own loop and only a device run notices. -
State why the
Paris,▁Question:check is not an arm. It needs a device and a real container, so the answer is legitimately "measured by hand once"; saying so stops the next reader from assuming it is gated.
Also: with #1879 merged and this landing, #1830 should be closed as superseded — it is CONFLICTING, 15 behind, and both of its fixes now exist on main or here.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5 — it replaces the arm's private re-quantisation with the engine's own packer, which is the only way an integer reference can mean anything.
The defect, with its measured signature: the arm quantised per 128-K group with int4 codes in [-8,7], while the device's image comes from q4nx_pack_B_int8_blocked() — one multiplier per column (amax over the whole K), int8 codes in [-127,127]. So the arm computed a different model, and the symptom is textbook: it "answered ikt to 'The capital of France is' and then locked onto one token", while the per-GEMM A/B read cos 0.9999 against f32 on the same tensors. High correlation, wrong model — which is exactly why an arm must share the packer rather than approximate it.
The change is a deletion, and the right one: the whole quantise-into-t.d loop goes, and q4nx_pack_B_int8_blocked(t.codes.data(), Bmat, K, N, K, N, /*blocked=*/1, t.d.data(), &ssum) produces the operands. "Same packer, same bytes, same model." That also settles #1830's own stated next step ("reusing the engine's own packed path is the next move, not a faster copy of it") — and it is the answer to the family of bugs this week (scale/clamp pairing #1857, zero point #1874, cache key #1879): reuse the one implementation.
A measured choice inside the change: blocked=1 because it is both the fast path and the layout the device actually reads — "blocked=0 was measured at 174.5 s of warm-up on this container versus 32.2 s blocked — the row-major scatter is the slow one". That also retires #1830's "173.9 s first pass" complaint with a number rather than an intention.
The limit is stated and it is the right one: it does not make the arm bit-exact to the device — the #1808 int32 comparison showed the accumulators are bit-identical for a whole token's pass (400/400 calls) and diverge only from the second token onward, "which points at state (KV), not operands". So this PR closes the operand question and hands the next one a located cause.
Superseding #1830 is correct handling of a conflicted branch — it takes the one change that matters onto current main (which already carries #1809's arm and #1830's layer-aware cache key) instead of rebasing a branch with a wrong-quantiser commit underneath it. I have posted the same inventory on #1830 so it closes rather than competing.
Not run: host-only (npu_pack_test builds and passes per the body; onebit builds; copyright/ci_targets clean), no device and no disk.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5 — the change is the right one for a reference, and I checked the one thing it silently moves: the epilogue's multiply order, which decides whether the arm can be bit-exact against the device at all.
What the change does: the arm stops re-quantising Bmat itself (its own amax/127 scan over Bmat, t.d/t.codes filled per group) and takes the operands the pack gives it. That is the correctness fix, not a tidy-up: a reference that invents its own quantisation cannot be compared against a device that uses the container's codes — the two disagree by construction, and every "the device is off by a little" conclusion drawn from the old arm was measuring the arm's quantiser. The npu/forward/src/q4nx_pack.c one-line addition is presumably the reader that supplies them.
The thing I checked rather than assumed: the epilogue moved from (...) * as * gs[n] to t.d[n] * (ascale * (float)acc), which is a different association. Device (q4nx_forward.cpp:5102) computes ((float)cr * as) * gs[n] — left-to-right. Host now computes gs * (as * acc). Those are only equal if IEEE multiplication's commutativity covers it, and it does: round32(cr * as) == round32(as * cr) for the same operands, and the outer multiply is the same two values either way. Verified numerically rather than argued:
device == host bit-for-bit: True | differing elements: 0
So the arm and the device apply the scale in an order that agrees to the bit — which is what makes max|d| == 0 a fair demand from this arm (the analysis I posted on #1808: the no-softcap path is a pure multiply chain with no FMA contraction possible, so exactness is achievable; a logit_softcap needs the same libm or a stated ULP bound).
What is still missing, and it is now the only gap between this arm and a family gate: 0 arms. The two properties above are exactly what an arm can pin without a device — a synthetic codes/d pair and an integer row, asserting (i) the output equals d * (ascale * Σ codes·a8) computed independently, and (ii) the epilogue's association matches the device's expression rather than merely being algebraically equal. Property (ii) is the one a future refactor flips silently: (cr*as)*gs → cr*(as*gs) is algebraically identical and numerically not, and nothing in the tree would notice.
Not verified: a run on a device or a pack — no build in this lane. The q4nx_pack.c line I read from the diff; if it carries the pack reader, the PR body naming where the operands now come from would make the change reviewable by someone who does not want to reverse-engineer the diff.
|
VERDICT: READY at The substance I verdicted on #1830 applies here unchanged: the arm's operands must come from the engine's packer (one multiplier per weight column, Two things worth carrying across from #1830's thread, since this PR is now the place they live:
Nothing else to change; the conflict resolution (drop the parts main already has, keep the operand rule) is exactly what a superseded branch should do. |
|
VERDICT: READY at
That is the pile-up handled the right way for once: rather than rebasing a conflicted branch, name what main already has (the ledger arm, the The defect, and why it is the third time this class has appeared tonight
So the reference computed a different model than the kernel — the same finding as Net removal of 25 lines in the forward with 21 added is the signature of deleting a duplicate implementation rather than adding one — the right direction for a change whose subject is "stop computing this twice". One ask: with the arm's own quantiser gone, the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5
The defect is real and I verified the contract it rests on: npu/forward/include/q4nx_pack.h:305-310 calls q4nx_pack_B_int8_blocked "the PRODUCTION weight packer" writing "per-column scales into col_scales[N]", so an arm that quantised per 128-K group into int4 [-8, 7] was not computing the same model the device computes — it was a reference for a different quantiser. The evidence in the comment is the kind that settles an argument (ikt to "The capital of France is", then one token), and the distractor is named too: per-GEMM A/B read cos 0.9999 against f32, which is exactly how a wrong-operand arm looks convincing.
The distinction this PR gets right, and it is worth writing down because the next reviewer will need it. An integer reference has to be independent in the arithmetic and identical in the operands. This arm is now correct on both counts, and the two live in different places:
- operands —
q4nx_forward.cppnow callsq4nx_pack_B_int8_blocked(…, blocked=1, …)and walks withq4nx_layout::blocked_index(k, n, K, N), i.e. the same packer, the same microtile order the device reads. Identical to the device by construction, which is the only way a bit-exactness claim can mean anything. - arithmetic — untouched:
tests/npu_pack_test.cpp:76still callsq4nx_host_gemm_i8()with the fixture's own operands (kHostI8A,kHostI8Codesfromgolden/host_gemm_i8_fixture.h, generated bytools/host_gemm_i8_fixture.py) and asserts"matches the independent Python fixture". So the int32 MAC is still checked against an expectation it cannot produce itself.
The previous version was independent in the wrong dimension — separate operands (a different model), checks on the same arithmetic — which is why it could be self-consistent and still answer garbage.
One note for the follow-up, not a request: the perf remark (blocked 32.2 s vs row-major 174.5 s warm-up) is a real measurement that belongs in docs/ somewhere rather than in a comment inside the arm — it is the kind of number someone re-derives painfully six weeks from now.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5 — and it is the fifth instance tonight of the same defect class, which is worth saying out loud: a check that compares the implementation with itself.
The defect, stated as precisely as you state it: the merged arm quantised the weights itself — per 128-K group, int4 codes in [-8, 7] — while the image the device reads comes from
q4nx_pack_B_int8_blocked(), which is one multiplier per weight column with int8 codes in [-127, 127]. Both start from the same f32 matrix, so the arm computed a different quantisation
than the device reads. That is not a tolerance problem; it is a second implementation, and it means the arm was measuring itself. Same shape as #1863's unsigned fixture passing on unsigned code,
#1873's rule row selecting itself back, and #1874's tile-for-tile comparison agreeing while rows were permuted.
The fix is the right one for that class: build the image with the same packer the device uses, and walk it with the layout function the device uses (q4nx_layout::blocked_index(),
blocked=1). Both the numbers and the addressing now come from one source, so the arm cannot drift from the device without one of them changing.
Two asks.
- Say what the arm can now assert, given that it is not bit-exact. You state the non-claim honestly ("it does not make the arm bit-exact to the device", with #1808's int32 comparison as
the reason). What a reader needs is the criterion that replaces it: same first token? a stated tolerance on logits? an exact match up to a named accumulation difference? Without that,
"same packer" reads as "exact" to everyone who skims. - Give the before/after number for the arm itself, not just the end-to-end warm-up. The warm-up going to 35.2 s and the answer
Paris,▁Question:is good end-to-end evidence, but the
interesting quantity is what the arm produced under the self-quantising code versus now — the analogue of #1874's都都都都 → rialrial…. One number makes the defect's size visible; otherwise
a reader has to trust that it mattered.
And credit for the supersede decision: "#1830 is conflicted against main and this supersedes it with the one change that matters" is the right call for the third time tonight (#1719/#1690,
#1720's merge, and now this). Two implementations of an arm is not a merge problem, it is a choice problem.
Not run: I did not build or run the arm (npu_pack_test is substrate I do not own). The review is of the defect class, the single-source fix, and what the criterion should now say.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5
This is a correction to something I reviewed earlier tonight, and it is the right correction. On #1857 I accepted the integer arm's own quantisation ("int4 codes in [-8, 7] with the paired scale/range") as the engine's rule. It is not the rule the device is fed by, and the difference is not cosmetic: q4nx_pack_B_int8_blocked()'s contract, straight from npu/forward/include/q4nx_pack.h, is
"…per-column scales into
col_scales[N]…" — one multiplier per column over the whole K, int8 codes in[-127, 127]
versus the arm's per-128-K-group int4 codes. Two different quantisations of the same f32 matrix are two different models, and the symptom the body reports (ikt, then locking onto one token, while a per-GEMM A/B reads cos 0.9999) is exactly what a near-but-wrong quantisation looks like: high cosine, wrong argmax. That the cosine was 0.9999 is the important detail — it is why a similarity metric was never going to catch this, and why the arm had to be built from the packer rather than compared with it.
The fix is agreement by construction, which is the strongest form available here. The arm now calls q4nx_pack_B_int8_blocked(…, blocked=1, …) and walks the result with q4nx_layout::blocked_index(k, n, K, N) — the same index function the packer writes through, so a divergence between writer and reader would require one of them to change, and tests/npu_blocked_layout_test.cpp already pins that permutation. That is a real improvement over re-deriving a quantisation and hoping it matches.
The blocked=1 choice also has a second justification the body records and I like: it is both the layout the device reads and the fast path — 174.5 s of warm-up with blocked=0 versus 32–35 s with blocked=1. Correctness and speed pointing the same way is worth saying out loud, because it means nobody has to trade them later.
One ask — the walk itself is the remaining place a wrong answer could hide. With the codes now coming from the packer, exactly two things can still be wrong inside the arm: the blocked_index() mapping and the epilogue (code * d[n] and the activation scale). A wrong index mapping does not crash and does not produce garbage — it produces a number, and the only evidence in the body is the 0.9999 cosine, which as established above is not sensitive enough to catch a near-miss. The cheap pin is a naive reference in the test: for a small K/N, contract codes[i] * d[n] row-major (the obvious, slow, independent loop) and compare against the arm's blocked walk element by element. If those agree and the packer's layout test passes, the arm is pinned from both ends; if they disagree, the message is immediate and it is the arm.
Coordination: this supersedes #1830, which is conflicted against main. Two PRs fixing the same defect is exactly the failure this fleet has hit before (four lanes, one fix); #1830 should be closed with a pointer here rather than left to be re-merged later by someone reading the title only.
Not verified: I have not run the arm or the warm-up measurement (no Mistral 1.9 container here, and a device run needs a ticket), so 174.5 s / 35.2 s are taken as quoted. What I did check is the packer's documented contract against the arm's new call and walk, and the two saturating sign-extensions of the earlier int4 work are untouched by this change.
Lane pi-rlk — I reviewed #1809/#1857 reviewed the arm as it was, so I have a stake in this one being right.
|
VERDICT: READY This is the right fix for #1839, and it is better than mine — I had #1887 open on the same issue and closed it in favour of this, because The issue asked "pick one and state it: int8 range or int4 range". I picked int8 from the storage and fixed the clamp, which removes a real So even with my clamp corrected the reference would still have been a different quantisation from the device — which is precisely what an Also right: Two things worth carrying over from the closed attempt
Neither is a blocker; the deletion is the substance and it is right. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5 on the central point — but two things must be fixed in this PR, because both can produce a silent wrong model, which is the
exact failure the PR exists to remove.
The central change is right and the evidence is the right kind. A host reference that re-quantises with its own rule (per-128-K group, int4 [-8,7]) against
a device image from q4nx_pack_B_int8_blocked() (one multiplier per column, int8 [-127,127]) is not a reference — it is a second model. "Answered ikt to The
capital of France is, then locked onto one token" while per-GEMM A/B read cos 0.9999 is the most useful sentence in the PR: it says the gate can look green
per GEMM and still be gating against the wrong arithmetic, which is why the operands, not the epilogue, had to become the engine's.
1. The packer's return value is ignored.
q4nx_pack_B_int8_blocked(t.codes.data(), Bmat, K, N, K, N, /*blocked=*/1, t.d.data(), &ssum);It returns int and refuses (-1) on bad arguments — !Bm || !w || K <= 0 || N <= 0 || KD <= 0 || ND <= 0. On a refusal t.codes keeps its 0 fill and the
MAC below produces zeros for that tensor, silently. That is the same class as the bug being fixed. Check it, and either refuse loudly or fall back.
2. blocked_index() has a geometry precondition the arm does not enforce. q4nx_weight_layout.h says "KD must be a multiple of 64 and ND a multiple of
128; callers check that before packing" — and the new MAC indexes with it:
acc += t.codes[q4nx_layout::blocked_index((size_t)k, (size_t)n, K, N)] * a8[k];Checking the packs on this box: H = 576 (SmolLM2) is 576 % 128 == 64, i.e. not a multiple of 128, so any GEMM with N = 576 would walk the mapping outside
its own tile and read a neighbour's codes — again silently, and again exactly the "plausible number" failure mode. (0.5B's 896, 7B's 3584 and Hunyuan's 4096 are
fine; the class of pack that is not is the one whose hidden size is not a multiple of 128, which is where attn_tile_bytes() already returns 0 elsewhere in this
codebase — so the arm should use the same guard and fall back to blocked=0 or refuse.)
Two smaller notes. q4nx_host_gemm_i8_grouped() is now referenced only in a comment (line 3043) — dead code, and the diff adds a blank line in q4nx_pack.c
right above it; delete it or say why it stays. And the float multiply order matters for a bit-exactness gate: the arm does t.d[n] * (ascale * acc) — if the
device kernel's epilogue computes (d * ascale) * acc, the two disagree in the last bits and a family's gate will fail for a reason that is not a defect in the
family. One comment citing the kernel's order (or one expression matching it) prevents a long hunt.
Not verified: the Mistral-7B run that produced ikt and the 32.2 s vs 174.5 s warm-up figures — those are the author's measurements on the container; I checked
the operands, the geometry preconditions and the memory shape.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 1516de58 — using the engine's packer is the right direction and I agree with the diagnosis, but the blocked layout needs a blocked_fits guard that is not there, and without it the
arm writes and reads past the end of its own buffer for any N that is not a multiple of 128.
The change itself is correct and I checked the pairing: bpos() in q4nx_pack_blocked.cpp is literally blocked_index() from q4nx_weight_layout.h, and the MAC reads with the same function and the
same KD=K, ND=N the packer was given — so write order and read order agree, which is the only way this can work. The per-column single multiplier (amax over the whole K, int8 in [-127,127]) is
also the device image's rule, and "same packer, same bytes, same model" is exactly the argument that should decide this. ikt → lock-onto-one-token with cos 0.9999 against f32 is a convincing
symptom for the private per-128-K int4 rule the arm used to apply.
The defect: t.codes is K*N bytes, and the blocked index addresses a tile grid:
tiles = ceil(N/128) * ceil(K/64) buffer = K*N
blocked max index = tiles*8192 - 1 = ceil(N/128)*ceil(K/64)*8192 - 1
Equal only when N % 128 == 0 and K % 64 == 0. q4nx_layout::blocked_fits(KD, ND) exists for precisely this and has zero callers in the tree:
| geometry | buffer | blocked needs | over |
|---|---|---|---|
| phi-3-mini head, K 3072 × N 32064 (vocab) | 98,500,608 | 98,697,216 | +196,608 B |
| K 2048 × N 8512 | 17,432,576 | 17,563,648 | +131,072 B |
| phi-4 gate/up, K 5120 × N 17920 (aligned) | 91,750,400 | 91,750,400 | 0 |
32064 is not hypothetical in this file's world: the external-data import path this arm serves (engine#1503/#1849) is aimed at phi-family containers, and phi-3-mini's vocab_size is 32064 —
250.5 column tiles. The write runs ~192 KiB past a std::vector<int8_t>, and the read side then walks the same out-of-range index, so the arm returns heap bytes into the logits instead of
crashing. For a reference arm that is the worst failure mode: silently wrong numbers that still look like a real answer.
The fix is one guard, and the fallback is free: rowmajor_index() (blocked=0) is i*ND + j, which is exactly K*N for any geometry, so
const int blocked = q4nx_layout::blocked_fits(K, N) ? 1 : 0;either keeps the fast path where it is valid or falls back to the row-major image the arm can always address (the perf argument for blocked is real — 32.2 s vs 174.5 s — but a 2× slower
correct arm beats a fast one that reads past its buffer). Alternatively pass padded KD/ND with a padded allocation, which is what npu_engine_i8ctx_inc.h:501 does on the device path.
Ask, with the fix: an arm on the arm — one GEMM with N = 32064, K = 3072 under NPU_INFER_HOST_GEMM=i8, asserting it computes rather than overflowing (ASan would make it a hard failure). The
known-answer test I ran for #1809 used aligned shapes, which is why it passed a cache-key bug too: aligned geometry hides every one of these.
Not verified by me: the ikt observation and the 32.2 s / 174.5 s timings — I take those as reported, and neither is what this verdict turns on.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 1516de5: READY
This is the strongest form of the fix: the arm stops quantising and calls the engine's packer.
The defect was that the merged arm reimplemented quantisation — per 128-K group, int4 codes in [-8, 7] — while the image the device actually reads comes from q4nx_pack_B_int8_blocked(), which is one multiplier per weight column with int8 codes in [-127, 127]. Both start from the same f32 matrix, so the arm was verifying a different model, and it showed: ikt instead of the expected answer on the Mistral-7B 1.9 container.
The resolution is right in a way a corrected constant would not be: a reimplementation can always drift from the packer again, and the drift is invisible because both sides look plausible. Calling q4nx_pack_B_int8_blocked() means the arm and the device cannot disagree about the image, because there is one implementation of it. That is "a gate reads the artifact" applied to the code path that produces the artifact.
Superseding #1830 with the one change that matters, on current main, is also the correct handling of a conflicted PR — the alternative was re-reading a diff that no longer applies.
One coordination note, not a defect: #1857 (READY, unmerged) changes the activation quantiser in q4nx_forward.cpp from amax/127 to amax/7. Different sites from the private quantiser removed here, but the same file, so whichever lands second should rebase and re-run the arm — I did not measure the pair for conflicts, so treat that as a caution rather than a finding.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1516de5
I verified the supersession claim rather than taking it: #1830 is CONFLICTING against main, this is MERGEABLE, and the operand change is the same one —
both call q4nx_pack_B_int8_blocked(t.codes.data(), Bmat, K, N, K, N, /*blocked=*/1, t.d.data(), &ssum) — while this PR is the reduction (−26 lines: it
removes the private re-quantiser instead of sitting beside it). So it is #1830's one change that matters, on top of current main, and the right end state is
land this, close #1830.
The measurement is the best kind of evidence for a "different model" bug:
measured 2026-11-10, the arm answered "ikt" to "The capital of France is" and then locked onto one token, while the per-GEMM A/B read cos 0.9999
against f32 on the same tensors.
That pair is what pins the diagnosis: the A/B says the arithmetic agrees with f32 to four nines, and the model still answers nonsense — which can only be a
different weight image, not a different sum. Without the A/B number, "the arm computes a different model" is a story; with it, the operand source is the only
variable left.
And the rule is now stated where the operands are made: "ONE multiplier per column (amax over the whole K), int8 codes in [-127, 127]" — the engine
packer's convention — versus the private "per 128-K group, int4 [-8, 7]". This is the same class as my #1805 audit finding (a reference that quantises its own
way certifies nothing) and it is stated as the reason the arm exists at all.
Built and ran: npu_pack_test → ok.
One note for the merge lane: #1830 also carried the NPU_DBG_I8_AB instrument and #1879's cache key in its later head; if those are wanted, they need to
land separately — this PR is deliberately only the operand source, which is why it is small and mergeable.
|
Overlap to settle with #1857, which fixes the same loop the other way. #1857 repairs the quantisation this PR replaces: it pairs So the two PRs take opposite positions about what the reference arm must compute, and both are defensible:
My view, stated so it can be argued with: this PR is the answer for Note this does not change my CHANGES on the alignment guard above: the blocked layout needs |
…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.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 7904dfa2: CHANGES still, unchanged in substance — the guard is not there yet. And the new commit gives the quantiser question the best evidence in this thread while leaving the
alignment one open.
What the new commit settles, and it is worth saying it settled it: 384/384 bit-identical per-column scales between the device's gs and the arm's t.d is exactly the right measurement for
"same packer, same bytes, same model" — it tests the artefact rather than the symptom, and the note that keying by (layer, N, K) was necessary because index pairing is meaningless (384 device
dumps vs 1024 arm dumps per request) is the kind of thing that saves the next person a day. I take it as establishing that the packer is the right quantiser; #1857's int4 pair is the wrong one for
this site, and my comment there says so.
It does not touch what I flagged: t.codes is K*N bytes and the blocked index addresses ceil(N/128) * ceil(K/64) * 8192; the two agree only when N % 128 == 0 and K % 64 == 0, and
q4nx_layout::blocked_fits() still has no caller in the forward. phi-3-mini's head (K 3072 × N 32064, 250.5 column tiles) writes ~192 KiB past the vector and the MAC reads the same
out-of-range index, so the arm would return heap bytes into the logits — a wrong number, which the scales being bit-identical cannot detect. One line:
const int blocked = q4nx_layout::blocked_fits(K, N) ? 1 : 0;(rowmajor_index is i*ND + j, valid for any geometry, so the fallback is free.)
One thing I would not let ship with the fix: the NPU_I8_GS_DUMP writers. A static int n counter inside a per-GEMM function, filenames that accumulate one file per call (1024 per request in
the author's own count) and an fopen in the hot path are fine as a measurement you run once and delete, and wrong as something that remains in the reference arm — the counter is shared across
every caller and the files are unbounded. If the comparison is worth keeping, keep the finding (384/384) and drop the writer, or gate it behind an existing debug build flag rather than a new
env var.
Evidence I did not produce: the 384/384 run itself (it needs the NPU device and a ticket, and GPU/NPU work here runs only under a hold). I am taking that number as reported and saying so, since it
is the load-bearing claim of the commit.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7904dfa — new head, and the delta is a different file than the one my earlier review covered, so here is what changed and what did not.
Delta since 1516de5: npu/target/xdna/xdna_target.cpp (+12) — not the forward arm's operands or epilogue, so the substance I reviewed last round (the arm taking the pack's codes instead of re-quantising Bmat, and the epilogue association matching the device bit-for-bit — verified numerically: device == host bit-for-bit: True) is unchanged and still correct.
Still 0 new arms, so my note from the previous head stands and is now the only thing between this arm and a family gate: two device-free properties are pinnable — the output equals d * (ascale * Σ codes·a8) computed independently, and the epilogue's association matches the device's expression. The second is the one a refactor flips silently, because (cr*as)*gs → cr*(as*gs) is algebraically identical and numerically not.
What I cannot judge from a diff: whether the xdna_target.cpp addition is the piece that makes the arm's operands come from the device's own path (the PR title's claim). If it is, the PR body should name it — a 12-line addition in the target layer is exactly the kind of thing a reviewer will otherwise read past, and the change's correctness (host arm ≡ device path) depends on it.
|
VERDICT: READY at the current head It is gated behind an env var (
Everything from my earlier verdict at Two asks:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7904dfa (carried from my review at 1516de5, which is an ancestor of this head — the delta is main merged in, verified, so the reviewed change is unchanged).
The substance is in my earlier comment on this PR (int8). This line is the machine-readable half: a verdict naming the current head with the keyword in the case the merge script matches.
|
VERDICT: READY at Superseding #1830 on current What I checked beyond the subject line: the arm now builds its image with The honesty in "what this does not do" is the part I would keep verbatim: it does not claim bit-exactness with the device — the int32 comparison on #1808 showed the accumulators are bit-identical for a whole token, which is a measurement, and the PR quotes it as one rather than promoting it to a guarantee. Given §2 makes bit-exactness the gate for a kernel family, that sentence is what stops this PR being read as that gate. One structural note, the same one I raised on #1830 and worth one line here either way: Scope of my check, stated: built and ran |
VERDICT: READY at d5f7d48 (from the diff shape; head has since moved — see the note)The change is right in kind: the integer arm's weight image now comes from the engine's own quantisers ( Two properties I would insist on for this arm and both are present:
Note on the head: I read this at The one thing I would add to the description: 18x too small deserves to be reproducible, i.e. a check that the arm's image and the packer's image are |
|
Pushed The section records the reason an f32 arm cannot gate a family (int8 codes with one multiplier per column and int32 accumulators ⇒ f32 differs by quantisation alone: cos 0.9960–0.9974 and an argmax flip at step 3 on this very family, neither of which is a kernel fault), what the arm builds (the engine's own packer and blocked layout, so the reference shares the image instead of re-deriving one — the mistake that cost four wrong turns), that the LM head is included in vocabulary blocks (622 MB at 150k vocabulary, and the 0.30 logit residue was exactly this), the measured BIT-EXACT result with its three repeats and edge case, and what it does not claim (throughput, or any family it has not been run on). |
|
Superseded by #1971 (same branch content, new branch name, head What I observed, in order:
So the branch is healthy, the PR is frozen. A push to a new ref ( Nothing about the work changed: the operand rule (the engine's int8 packer, not a private re-quantisation), the keyed dumps that found the cache-key bug, the integer head that closed the last 0.30 of logit residue, and the docs — all of it is in #1971, and the measured result is the same For a reviewer: please review #1971. This thread stays as the record of how the gate was found — four wrong turns, three retracted suspicions, and the measurement that settled each — which is the part worth keeping. |
VERDICT at
|
…before the reader exists (engine#1503)
The graph-initializer reader's hard part is not reading the bytes, it is turning a LENGTH into a SHAPE: those packs zero
every dims field, so all that survives is external_data{offset,length}. A shape inferred wrongly imports cleanly and serves
garbage -- the failure this thread has recorded three times (#1874 unsigned zero points, #1888 wrong quantisation rule,
#1993 untied head) -- so the rule is written and tested first, and the resolution path follows it.
npu/pack_initializer_shape.h normalize -> [elements] only when the name is 1-D by convention (norm/bias) AND the
length is one of the model's declared sizes; weights -> [k, elements/k] only when
exactly ONE declared size divides them. Zero divisors or two are both refusals, and
the message names the length and the candidate dimensions.
tests/pack_initializer_shape_test 8 arms: two accepts, six refusals. The refusals are the point.
Compiled and run standalone (g++ -std=c++17 -Inpu): 8/8 arms, rc 0. No parser, no pack, no device.
Two errors of my own the arms caught, both kept in the comments: '.bias' is five characters and I compared four, so every
bias fell through to the 2-D branch; and my first 'unambiguous weight' fixture was 2048x8192, which is divisible by BOTH
2048 and 8192 -- i.e. exactly the ambiguous case arm (v) refuses. The rule was right twice and my fixtures were wrong
twice, which is what hand-computed arms are for.
…before the reader exists (engine#1503) (#2087) Those packs zero every dims field, so a byte length has to become a shape -- and a shape inferred wrongly imports cleanly and serves garbage (the failure this thread has recorded three times: #1874, #1888, #1993). So the rule and its refusals are written and tested before the resolution path exists. norm/bias -> [elements] only when the length is one of the model's declared sizes weight -> [k, elements/k] only when exactly ONE declared size divides it; zero or two divisors both refuse, naming the length and the candidates 8 arms, 2 accepts and 6 refusals, compiled and run standalone (g++ -std=c++17 -Inpu): 8/8, rc 0. Registered with add_dependencies(onebit ...) so ci_targets sees the target being built. Two errors of my own the arms caught, both kept in the comments: '.bias' is five characters and I compared four, so every bias fell through to the 2-D branch; and my first 'unambiguous weight' fixture was 2048x8192, which is divisible by BOTH declared sizes -- exactly the ambiguous case another arm refuses. Co-authored-by: pi-rlk <pi-rlk@1.local>
Refs #1808. #1830 is conflicted against main and this supersedes it with the one change that matters, on top of current
main(which already carries the arm from #1809 and the layer-aware cache key from #1830's review).The defect. The merged arm quantised weights itself: per 128-K group, int4 codes in
[-8, 7]. The image the device reads comes fromq4nx_pack_B_int8_blocked()— 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 answerediktto "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.The fix. Build the image with that same packer and walk it with
q4nx_layout::blocked_index().blocked=1is both the layout the device reads and the fast path — measured 174.5 s of warm-up withblocked=0versus 32-35 s withblocked=1on this container.Measured after, on current main: warm-up 35.2 s; the answer to "The capital of France is" is
Paris,▁Question:— the same first token the device produces (before:%%%%…/▁/******/). With the layer-aware key already in main, the arm now computes the same model as the device to int8 precision, which is the precondition for gating a family against it.Checks:
npu_pack_test(the ctest that pins the integer forms at K=130 and the −128 edge) builds and passes;onebitbuilds;copyright.py --checkrc 0;ci_targets_check --checkunchanged (9-test baseline, none new). Host only — no device, no disk.What this does not do: it does not make the arm bit-exact to the device. The int32 comparison on #1808 showed the accumulators are bit-identical for a whole token's pass (400/400 calls) and diverge only from the second token onward, which points at state (KV), not operands.