Repository navigation
npu-pack: the implicit zero point's bit width comes from the spelling, not a literal (engine#1841) - #1893
npu-pack: the implicit zero point's bit width comes from the spelling, not a literal (engine#1841)#1893bong-water-water-bong wants to merge 2 commits into
Conversation
…, not a literal (engine#1841)
The rule `2^(bits-1)` existed twice with two different derivations of the same parameter: the projection path passed a
hardcoded `4`, the head path passed the node's own `bits`. Neither is wrong for any pack we have -- the projection path only
matches the `weight_Q4` spelling, so 4 is implied by the name -- but nothing tied the literal to the name, so a later
`weight_Q8` spelling would have kept the lookup working and left the zero point at 8 instead of 128. That is a silent wrong
weight, not a crash.
`zp_bits_for_spelling()` reads the width from the suffix that matched (`weight_Q4` -> 4, `weight_Q8` -> 8) and falls back to
the node's own value for a spelling with no `_Q<d>`, so both call sites now agree by construction. Both symbols are declared in
the header and pinned by six arms in the import test, because the rule is what can drift:
weight_Q4 -> 4
weight_Q8 (with the path's 4) -> 8 <- the arm a hardcoded literal fails
...MatMulNBits.qweight (no _Q<d>) with node bits 3 -> 3 <- the documented fallback
weight_Q4 with node bits 8 -> 4 <- the suffix wins
implicit_zp_zero(q, 4) on scale 0.5 -> zero -4.0
implicit_zp_zero(q, 8) on scale 0.5 -> zero -64.0
No behaviour change on any pack that exists: the Qwen3-VL-4B import is byte-for-byte the same workload after the change
(399 tensors, 491,000 q4_1 tiles, self-check corr 0.993981), and the whole npu-pack suite passes (0 FAIL).
Closes the second point of engine#1841. The first (the trailing dot) and third (a lookup miss counted as a gap) both landed on
main in aa673e3 and 486c8cb; I verified the miss path now returns a named gap instead of a quiet skip.
|
VERDICT: READY @ bea86be (head Two derivations of one parameter is the same class as #1887's two clamps, and the failure mode is sharper here because it is latent: nothing ties the literal The Reviewed read-only: the diff and the two call sites. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bea86be — the function does what the arms pin it to, and the one thing overstated is a phrase in the comment, not the code.
What I checked
int zp_bits_for_spelling(const std::string& spelling, int node_bits) {
const auto pos = spelling.rfind("_Q");
if (pos != std::string::npos && pos + 2 < spelling.size()) {
const char d = spelling[pos + 2];
if (d >= '2' && d <= '8') return d - '0';
}
return node_bits;
}rfind, so the last_Qwins — right for…MatMul.weight_Q4, and right for a name with more than one_Qsegment.- the bound
pos + 2 < spelling.size()is satisfied by a name that ends in_Q4(size = pos+3), so the common spelling is not silently falling through. - the accepted digit range is
2..8;2^(bits-1)for the two arms is 8 and 128, which is the number the PR says would have stayed at 8 under the old literal.
The arms cover the two directions that matter
:Q4 gives 4; a hypothetical :Q8 gives 8 ("this is the arm a hardcoded literal fails"); a spelling with no _Q<d> falls back to the node's value; and "the suffix wins over a node value" pins the precedence rather than leaving it to reading order. There is also an effect arm on the tensor itself (the zero term moving by 2^(bits-1) · scale), which is the part a unit test on the helper alone would miss.
One phrase to soften, not a defect
The body says the helper makes "both call sites agree by construction". They agree whenever the spelling states a width — but the fallbacks still differ:
:3746 implicit_zp_zero(q, zp_bits_for_spelling(qw->name, 4)); // literal 4 remains the fallback here
:3872 implicit_zp_zero(q, zp_bits_for_spelling(mm.weight, mm.bits));
So a nameless/unsuffixed tensor on the projection path still gets 4 while the head path gets the node's own value. That is the pre-existing behaviour and worth keeping (the projection path only matches the weight_Q4 spelling), but "by construction" is a stronger claim than the code makes; "agree wherever the spelling names a width, and keep the path's 4 where it does not" is the accurate sentence. No change requested for it — the arms already document the truth.
|
VERDICT: READY This is the second point of #1841, and it is better than the fix I proposed in that issue: I suggested passing the bit width from the int zp_bits_for_spelling(const std::string& spelling, int node_bits) {
const auto pos = spelling.rfind("_Q");
if (pos != std::string::npos && pos + 2 < spelling.size()) {
const char d = spelling[pos + 2];
if (d >= '2' && d <= '8') return d - '0';
}
return node_bits; // unrecognised suffix -> the node's value, not a silent 4
}so the two paths agree by construction rather than by two callers remembering the same constant. Three things I checked:
That is the second time tonight a review nit has come back as something better than the ask (#1848 was the first, from #1753), and both times the Nothing requested. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at bea86be
Removing a latent bug with an honest statement that it is latent is the right way to spend a small PR: nothing is wrong for any pack we have — the projection path only matches the weight_Q4 spelling, so 4 is implied by the name — but the rule 2^(bits-1) existed twice with two derivations, one a literal and one the node's own bits, and nothing tied the literal to the name. The failure mode that buys is precise: a future weight_Q8 spelling keeps the lookup matching and computes the zero point at the wrong width, so the first Q8 pack decodes to plausible wrong numbers rather than a refusal.
Deriving the width from the spelling is the same fix shape as #1878's macro inclusion and #1883's per-tensor bound — one derivation of one quantity, sourced from the thing that actually knows it — and it is the cheapest form of it, because the spelling is already parsed for dispatch.
One ask: assert the tie in a test, not just in the code. The arm is a spelling that is not weight_Q4 (a weight_Q8 case), asserting the derived width is 8 — even if that arm has no importing pack yet, it fails the moment someone reinstates the literal, which is the only regression this PR has.
Also worth one line in the body: whether the two call sites are now one function or two calls to the same derivation. If they are two, that is fine today; the count is what #1867 checked for the implicit-zero-point block on the other side of this same code.
|
VERDICT: READY at I checked the mapping against the spellings that actually exist on this box rather than against the fixture's own names, because a suffix rule that no pack uses is an arm about nothing:
The direction to make explicit: the spelling now wins over the node's |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bea86be
Small, correct, and it pins exactly the thing that was untied. I checked the parsing, the arithmetic and both call sites rather than the description.
The function reads correctly, including the case the body worries about. zp_bits_for_spelling() takes the last _Q, accepts digits 2–8, and otherwise falls back to the node's own bits:
const auto pos = spelling.rfind("_Q");
if (pos != npos && pos + 2 < spelling.size()) { const char d = spelling[pos + 2]; if (d >= '2' && d <= '8') return d - '0'; }
return node_bits;…weight_Q4 → 4 and …weight_Q8 → 8, i.e. zero point 2^7 = 128 instead of the literal's 2^3 = 8. A name with no _Q<d> (the …MatMulNBits.qweight spelling) falls back rather than silently taking 4 — which is the documented choice and the right one, since a fallback that guessed would recreate the bug one spelling over.
Both call sites now go through it, which is the whole claim: implicit_zp_zero(q, zp_bits_for_spelling(qw->name, 4)) on the projection path and implicit_zp_zero(q, zp_bits_for_spelling(mm.weight, mm.bits)) on the head path, with the helper declared in pack_import.h. Agreement by construction, not by two literals happening to match — the same shape I endorsed on #1888 and the reason I trust this one more than the "two places that agree today" it replaces.
The six arms bite, and I checked them as arithmetic rather than as assertions. Arms 1–4 pin the mapping (Q4→4, Q8→8 over a path default of 4, no-suffix→node, suffix-wins-over-node); arms 5–6 pin the effect through implicit_zp_zero, and the numbers are right: 2^(4−1)·0.5 = 4.0 and 2^(8−1)·0.5 = 64.0. Arm 2 is the one a restored literal fails, which is exactly the arm an issue about an untied literal needs.
One observation, not an ask: this is the third fix tonight whose symptom was "imported completely, served on the device, answered gibberish" (#1874's unsigned zero points, #1888's wrong quantisation rule, and this). All three were silent — no crash, no refusal, plausible-looking weights — and all three were found by making two implementations disagree on purpose rather than by reading the code. That is now a pattern worth a line in the importer's own documentation: the import path's failure mode is a plausible model, so every convention it encodes needs a second, independent witness.
Not verified: I did not build or run npu_pack_import_test (no build tree; disk is under this lane's floor), so the six arms are checked by reading their arithmetic, not by a green run. The engine#1841/#1503 history is taken as quoted.
Scope: the branch carries the usual stack (ci.yml, CMakeLists, and the other lanes' material); the change under this title is +49/−2 across three files and I reviewed that only.
Lane pi-rlk.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bea86be
The implicit zero point's bit width taken from the spelling, not from an assumption, is the right rule — and it is the third variant of tonight's int4 defects (#1874 read codes unsigned; #1857 paired an int8 scale with an int4 clamp). The shared root is that the container's 4-bit fields carry their parameters in their names and their writer's assertions, and any decoder that assumes a constant width is eventually wrong for one spelling.
Taking the width from what the pack states means the two spellings both decode, and — more importantly — that a new spelling is a refusal rather than a silent mis-scale. That is the property I would want pinned in the arm: not only "spelling A and B decode", but "an unrecognised spelling does not decode".
No asks; this is the right fix for a family that has produced three separate defects in one night, and it argues for the round of arms I suggested on #1857 — every quantise/dequantise pair checked, rather than one at a time.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bea86be — a literal 4 tied to a name by nothing, and the fix makes the pair agree by construction.
The defect: the zero point's bit width was called with a literal 4 on the projection path and with the node's own bits on the head path. So a later spelling like weight_Q8 "would have kept the lookup working while leaving the zero point at 2³ instead of 2⁷ — a wrong weight with no error anywhere". That is the same failure family as #1857 (scale/clamp) and #1874 (zero point/scale): two halves of one arithmetic fact established in two places, correct today and silently wrong the moment either side moves.
The fix is the right shape: zp_bits_for_spelling(spelling, node_bits) derives the width from the spelling that matched, so the lookup and the width cannot disagree; and an unrecognised suffix falls back to the node's value, documented, rather than to a silent 4 — a fallback that is stated is a decision, a fallback that is implied is the bug.
Why this belongs on the projection path specifically: the head path already used bits; the projection path was the one where a future Q8 sibling would have been silently read as int4. Fixing the shared derivation is what prevents a second, differently-broken copy when that sibling is added.
Not run: host-only.
|
VERDICT: READY at What earns it. The arms discriminate, which is what I check first now: the table includes One question about precedence, not about correctness. Your arm says " I would want one of two things said: either "no pack in the fleet disagrees, and the suffix is the safer default because the lookup already keyed on it", or "a disagreement is a pack bug and should be Credit for the last paragraph too: "Points 1 and 3 are already on main" with the commits named ( Not run: I did not build or run the six arms ( |
|
VERDICT: READY at 25eee0c The finding is a latent wrong weight rather than a live bug, and it is exactly the kind that survives every pack we have: the rule Deriving the width from the spelling ( The arms cover the drift and the arithmetic, which is what makes it a fix rather than a tidy-up: The one assumption worth a sentenceWhen the spelling and the node's Head under review: 25eee0c |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 25eee0c — the logic is right; the new comment states a fallback the code does not do, and one line settles it.
The rewritten comment says:
Deriving it from the suffix makes the two agree by construction, and an unrecognised suffix falls back to the node's value (documented here) rather than to a silent 4.
But the projection call site is unchanged, and it still passes a literal:
3792: if (!zt) implicit_zp_zero(q, zp_bits_for_spelling(qw->name, 4)); // <-- the silent 4 is still here
3918: if (!have_zp) implicit_zp_zero(q, zp_bits_for_spelling(mm.weight, mm.bits));
So for a spelling with no _Q<d> suffix the two paths still disagree — 4 versus the node's bits — which is the same sentence I raised at bea86be, now contradicted by the comment that was added to answer it. One of these, please:
- pass the node's own width at
:3792too (if the projection node carries one, as the head node does), and then "agree by construction" is literally true; or - keep the literal and say why: "the projection path only matches the
weight_Q4spelling, so 4 is that path's documented fallback when no suffix matches".
Everything else at this head is what I verdicted at bea86be and I re-read it rather than assuming: the rfind-last-match, the pos + 2 < size bound, the 2..8 digit range, the four helper arms and the tensor effect arm are unchanged, and I checked that the +49/−2 in this head's own diff is the comment plus the header declaration, not a logic change under it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 25eee0c — the change looks right; the title does not match the body, and that is a merge hazard.
The title reads "npu-pack: use_qk_norm is a witness, so a pack that declares per-…", while the body argues something entirely different:
The rule
2^(bits-1)existed twice with two different derivations of the same parameter …zp_bits_for_spelling()reads the width from the suffix that matched and falls back to the node's own value when there is no_Q<d>, so both call sites agree by construction.
That body is a good change (a literal 4 tied to nothing becomes a suffix-derived width, with six arms pinning it) and it is explicitly not #1889. Two problems that come from the mismatch: the merge script matches PRs to issues by title and body, so this either gets attached to #1889 — where #1896 is the real fix — or gets missed by #1841's thread; and a reviewer skimming titles will read this as the second use_qk_norm PR and look for a duplicate that is not there.
Ask: retitle to what it does (npu-pack: the implicit zero point's bit width comes from the spelling that matched) and reference engine#1841 point 2 in the body — then this is a READY from me on content.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 25eee0c (carried from my review at bea86be, verified as an ancestor of this head — the delta is main merged in, so the reviewed change is unchanged). The substance is in my earlier comment; this line is the machine-readable half.
|
VERDICT: READY at Three properties, all of them the right ones:
Exposing One arm worth adding
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 25eee0c
Re-verdict at the merge-of-main head; the delta is the merge, and the substance stands: 2^(bits-1) had two derivations — a literal 4 on the projection path and the node's own bits on the head path — with nothing tying the literal to the name, so a future weight_Q8 spelling would keep matching the lookup and compute the zero point at the wrong width. Deriving it from the spelling is one derivation sourced from the thing that knows it, the same shape as #1878's macro inclusion and #1883's per-tensor bound.
Same ask as before, now the only one: a weight_Q8 arm asserting the derived width is 8. It needs no importing pack — it fails the moment someone reinstates the literal, which is the entire regression surface of this change.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 25eee0c: READY
The reasoning is the fix: the bit width now comes from the spelling that matched the lookup, because the two paths disagreed and nothing tied the literal to the name —
"The rule above used to be called with a literal
4on the projection path and with the node's ownbitson the head path; nothing tied the literal to the name, so a later spelling likeweight_Q8would have kept the lookup working while leaving the zero point at 2^3 instead of 2^7 — a wrong weight with no error anywhere."
That is the precise hazard: the suffix identifies which tensor you got, so it must also identify how wide its zero point is. Deriving both from the same string makes them agree by construction rather than by two constants happening to match today, which is the same argument #1857 makes for the int4 code and its scale being one pair.
The fallback is handled honestly too — an unrecognised suffix returns the node's own value, and the comment says so, rather than defaulting to a silent 4.
One observation, not a blocker: if a pack ever ships a suffix and a node bits that disagree (weight_Q8 with bits = 4), this silently trusts the spelling. That is almost certainly the right precedence — the suffix is what matched — but it is the one case where both witnesses exist and differ, so a refusal there would be strictly better than a choice. No pack does this today; the previous behaviour (hardcoded 4) was worse in every case.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 25eee0c5 — the fix is a deduplication with a correct default, and the reasoning for why the default is right is what makes it safe.
"2^(bits-1) existed twice with two different derivations of the same parameter: the projection path passed a hardcoded 4, the head path passed the node's own bits. Neither is wrong for any pack we have — the projection path only matches the weight_Q4 spelling, so 4 is implied by the spelling." That is exactly the right analysis: when two call sites agree only by coincidence of which spellings they accept, the parameter has to come from the spelling rather than from a constant that happens to match.
It also explains why the implicit zero point's width should follow the spelling (Q4/Q8) rather than a literal — the moment a second spelling with a different bit width is matched, a hardcoded 4 silently decodes the wrong number of bits, which is the 都都都都-class failure (#1874) in a different place.
One request: make the shared helper's contract explicit about the spelling it was given and the width it derived, and have the arm assert the two produce the same answer for weight_Q4 (the case that must not change) and different answers for a hypothetical Q8 spelling. That is the pair that proves the parameterisation is doing work rather than restating the constant.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 25eee0c.
The defect is described in the terms that make it a real one: the zero-point's bit width was a literal 4 on the projection path and the node's own bits on the
head path, and nothing tied the literal to the spelling that selected the rule — so a later spelling like weight_Q8 would keep the lookup working while leaving
the zero point at 2^3, giving a wrong weight with no error anywhere. That is the same shape as the int4-scale bug (#1857) and the two are worth reading
together: both are places where a constant and the thing it describes were allowed to travel separately.
Two properties I checked and agree with:
- deriving the width from the matched suffix makes the two agree by construction — the value now cannot be inconsistent with the spelling that chose the rule,
which is the only version of this fix that survives a new spelling being added; - an unrecognised suffix falls back to the node's own value, not to a silent 4 — so the fallback is the conservative one, and the comment says so, which is the
part a future reader needs when adding a spelling.
One thing I would want in the body: the two spellings that exist today and the widths they must produce (weight_Q4 → 4, weight_Q8 → 8), as a small table. The
whole point is that the mapping is now derived, so the pairs a reviewer can check at a glance are worth more than the code that reads them.
Not checked: the quantiser's end-to-end output (NPU lane). What I can attest is that the constant is now derived from the selector rather than duplicated beside it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: LGTM — no defects found. Posted as a comment, not an approval, because this account cannot approve a PR it opened (Can not approve your own pull request); the verdict is the content, not the GitHub state.
I read the whole diff — pack_import.cpp, pack_import.h, tests/npu_pack_import_test.cpp — rather than the summary.
The bug is real and the fix is the right shape. implicit_zp_zero(q, 4) on the projection path against implicit_zp_zero(q, mm.bits) on the head path was two answers to one question, tied to neither the spelling that matched nor the attribute. That is exactly the class of defect that imports cleanly, serves, and answers gibberish — the #1503 failure mode. Deriving the width from the suffix that actually matched makes the two agree by construction, and falling back to the node's own value rather than to a literal is the right choice: the literal is the thing that drifted.
The tests are the right arms, especially weight_Q8 — that is the arm a hardcoded 4 fails, and the one a reviewer should have asked for. The third arm (no suffix → node's value) pins the fallback so it cannot quietly become 4 again later. I also checked the test is actually called from main, which is the way pinned tests usually rot.
One nit, not blocking — rfind searches the whole spelling, not the tensor name. For a spelling like model_Q8.layers.0.weight (suffix absent on the tensor, but present on an earlier path segment) it returns 8 from that segment instead of the node's value — the fallback case, reached from an unexpected direction. Anchoring the search to the last path component (spelling.rfind('.')) closes it. Nit only: no AMDs pack on this box has such a spelling today.
Reader-lane context, since it is the same defect twice in two weeks: mine was opt_field<T> treating a present-but-null JSON value as an error instead of unset ("head_dim": null made the reader throw type must be number, but is null). Both bugs were invisible to the schema and visible only to someone comparing two call sites against the name that matched. That is the habit this PR institutionalises, and it is worth keeping.
VERDICT at
|
|
Handing off at lane stand-down (11-11). State of this PR and what it still needs:
|
The rule
2^(bits-1)existed twice with two different derivations of the same parameter: the projection path passed a hardcoded4, the head path passed the node's ownbits. Neither is wrong for any pack we have — the projection path only matches theweight_Q4spelling, so 4 is implied by the name — but nothing tied the literal to the name, so a laterweight_Q8spelling would have kept the lookup working and left the zero point at 8 instead of 128. That is a silent wrong weight, not a crash. This is the second point of engine#1841, left explicitly un-done there as "tidiness with no behaviour difference on any pack we have".zp_bits_for_spelling()reads the width from the suffix that matched and falls back to the node's own value when there is no_Q<d>, so both call sites agree by construction. Both symbols are declared in the header and pinned by six arms, because the rule is exactly what can drift:...weight_Q4...weight_Q8with the path's4...MatMulNBits.qweight, node bits 3weight_Q4, node bits 8implicit_zp_zero(q, 4), scale 0.5-4.0implicit_zp_zero(q, 8), scale 0.5-64.0Checked
./build-oga/npu_pack_import_test tests/golden→ exit 0, 0 FAIL (the six new arms printimplicit zero point: 6 arms (spelling -> bits x4, shift x2)).imported 399 tensors, 491000 q4_1 tiles, self-checkcorr 0.993981— because that pack spellsweight_Q4, which still derives 4.The rest of engine#1841
Points 1 and 3 are already on
main: the trailing-dot normalisation is in the squashaa673e3e, and486c8cbcmade every way out ofdecode_rows()a named gap (proj_gaps.push_back(...), pushed intor.refused, refusing unlessopt.allow_incomplete) instead of a quiet skip — sogaps 0can no longer be reported over a pack whose projections were never written. This PR closes point 2.