Skip to content

npu-pack: the 1.9 tail head gets the same self-check as the host head - #1515

Merged
bong-water-water-bong merged 7 commits into
mainfrom
1bit/npu-fconst-19-selfcheck
Nov 8, 2026
Merged

bong-water-water-bong merged 7 commits into
mainfrom
1bit/npu-fconst-19-selfcheck

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Two commits, both from the same session of verifying 1.9 packs against their 1.8 twins.

1. The tail head gets the self-check the host head has always had

The tail-head import in #1507 decoded lm_head but skipped the numeric check the host-head path runs, so a 1.9 import reported self-check: skipped -- the head is not tied to the embedding while the pack's own fp16 embedding was there as an independent reference.

It now runs, on the same terms: self-check (on a tied model the pack's embedding must reproduce its 4-bit head; on an untied one the number simply comes out low), with the nibble-swapped and no-zero-point decodes as controls that must fail.

2. The tail envelope is 64 + 2*hidden, and Qwen3-1.7B is the second measured entry

The read assumed the head's tiles start a fixed 1,856 bytes into the tail. That constant is only qwen2.5-0.5B's, where 1,856 == 64 + 2*896 by coincidence — the envelope is the same 64 + 2*hidden a layer block opens with. On Qwen3-1.7B it missed the head's start by 2,304 bytes, and the import refused rather than reading the wrong offsets; with the envelope derived from the hidden size the tail is 4,160 + 2,374 × 73,984 exactly.

That refusal is what surfaced the second entry, and it is worth having because the prefix is not 64 + 2*hidden either: Qwen3 carries its per-head QK norm gains inside the block before the q/k/v bundle, so the prefix is 8,512 at hidden 2,048 where that formula says 4,160. Both are rule parameters, measured per pack, never derived.

5. Qwen3-4B's 1.9 shape, and o_proj's K in the kernel family

The measurement tool merged as #1532 found Qwen3-4B's entry on its first real pair — {2560, 32, 36, 9536, 5, 1, 1462272}, with 2,933 of 2,208,061,248 bytes differing from the 1.8 twin (1.3e-6: content, not layout). Its prefix corroborates the Qwen3 offset at a second hidden size (64 + 2*hidden + 4,352 → 8,512 at 2,048, 9,536 at 2,560), while the tile count differs from 1.7B's 3 — which is exactly why this is measured per pack and never derived.

Importing with it exposed a second bug, caught by the family guard before it could serve wrong text:

declaring "QKV:2560x6144,O:2560x2560,GU:2560x19456,D:9728x2560"
this model derives "QKV:2560x6144,O:4096x2560,GU:2560x19456,D:9728x2560"

o_proj's K is q_proj's output width, which equals hidden only when heads*head_dim == hidden. Qwen3-1.7B satisfies that (16 × 128 = 2048); Qwen3-4B does not (32 × 128 = 4096 vs hidden 2560), so the family string was wrong for exactly the models where the two differ — a silent wrong-answer class, refused loudly instead. Verified after the fix: import 399 tensors / 491,000 tiles, 4,896/4,896 attention tiles and 6,912/6,912 MLP groups through decode->encode, self-check 0.999999, designs QKV:2560x6144,O:4096x2560,GU:2560x19456,D:9728x2560, and coherent greedy text on the host GEMM.

Evidence

Qwen2.5-0.5B (rai_1.9.0_npu vs rai_1.8.0_npu_4K): 291 tensors / 67,088 tiles (same counts), self-check corr 0.999999 / rel_err 0.000700 (controls 0.0000, 0.0000), all 291 tensor blobs byte-identical to the twin, greedy text identical on three prompts on the host GEMM and on the NPU.

Qwen3-1.7B (rai_1.9.0_npu vs rai_1.8.0_npu_4K): 311 tensors / 210,016 tiles (same counts), 2,688/2,688 attention tiles and 3,584/3,584 MLP groups survive decode->encode, 56/56 norm anchors, self-check corr 0.999999 / rel_err 0.000764 (controls 0.0000, 0.0000), greedy text identical to the twin on three prompts on the host GEMM and identical again on the NPU — including the one prompt whose continuation differs between backends, which is the int8 EP vs fp32 host difference and appears identically in both releases.

The two 1.9 builds' fconsts differ from their twins in 535 code bytes (Qwen3, all in layer 24) and 112 (SmolLM2, all in layer 3) — content, not layout, and not enough to move greedy text. Byte-identical packs are therefore not a valid acceptance test; identical text on both backends is.

ctest: 128/129; the single failure, npu_runtime_option, is pre-existing on a clean tree.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at c1d8c8ac — one small fix: the controls you are quoting as 0.0000 are the struct's defaults, not measurements.

The self-check itself is right and it is the right idea. It mirrors the host-head path exactly — same Σ|x−e|/Σ|e|, same corr(q, embed_bytes, H, q.N) — guarded on q.N == V && q.K == H so it cannot fire on a qkv matrix, and setting selfcheck_reference so the summary stops saying "skipped".
And your framing is the strongest justification in the PR: the check does not know the head came from a .fconst tail, so a pass is evidence about
the tail layout from a reference that is independent of it. corr 0.999999 / rel_err 0.000700 is a real measurement and a convincing one.

But the controls line does not run on this path. pack_import.h:186:

double lmhead_corr = 0, lmhead_rel = 0, ctrl_corr_nibble = 0, ctrl_corr_nozp = 0;

and pack_import.cpp:2760 prints them whenever selfcheck_reference is set:

printf("  controls:   nibble-swapped corr %.4f, no zero-point corr %.4f (both must fail)\n",
       r.ctrl_corr_nibble, r.ctrl_corr_nozp);

This commit sets selfcheck_reference = true and computes lmhead_rel/lmhead_corr — it does not compute the two controls. So on a 1.9 pack the
output is:

self-check: lm_head vs the pack's fp16 embedding  corr 0.999999  rel_err 0.000700
controls:   nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)

where both 0.0000s are the initialisers. "Both must fail" is satisfied vacuously, because nothing was measured — which is exactly the shape this
family of checks exists to avoid, and the one place the PR's own argument leans on it.

Two ways, both small:

  1. Compute them here, exactly as the host path does (decode_matmulnbits(..., swap_nibbles=true, false) and (..., false, drop_zero=true), then
    corr(..., 2048) — the host path compares only 2048 rows for speed) and set the two fields. That is ~6 lines and makes the summary true for a 1.9
    import.
  2. Or suppress the line when they were not computed — a bool controls_measured printed as controls: not run on this path — so an absent control
    reads as absent rather than as a perfect result. Given the point of the self-check, I would take (1): the controls are what show the metric
    discriminates, and a 4-bit decode against an fp16 reference deserves the negative case.

Also for the record, and worth keeping: your gate status is the part that makes the 1.9 path credible — 291 tensors / 67088 tiles identical to the 1.8
twin, 768/768 attention and 2160/2160 MLP groups surviving decode→encode, 48/48 norm anchors, all 291 blobs byte-identical, and greedy text identical on
three prompts on the NPU as well as host GEMM (8 ELFs from the existing 05b-hyb set, amdxdna errors 0, ticket held 25 s and released). And the
artifact note — no new kernel family needed, because hybrid and plain packs differ in bias, not shape, so kernels-05b-hyb-b8 is the right geometry —
saves the next person from building a redundant set.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Self-Check for Untied Models

The self-check logic only executes when q.N == int(V) && q.K == int(H), which may not cover all cases for untied models where the embedding dimensions might differ from the quantized tensor dimensions. This could lead to missed validation in some configurations.

if (q.N == int(V) && q.K == int(H)) {
    double sum_abs = 0, sum_ref = 0;
    for (int n = 0; n < q.N; ++n)
        for (int k = 0; k < q.K; ++k) {
            const size_t g = size_t(n) * (q.K / 32) + size_t(k) / 32;
            const double x = double(q.codes[size_t(n) * q.K + k]) * q.scale[g] + q.zero[g];
            double e;
            if (embed->type == kF16) {
                uint16_t h;
                std::memcpy(&h, embed_bytes + (size_t(n) * H + k) * 2, 2);
                e = f16_to_f32(h);
            } else {
                e = bf16_to_f32(reinterpret_cast<const uint16_t*>(embed_bytes)[size_t(n) * H + k]);
            }
            sum_abs += std::fabs(x - e);
            sum_ref += std::fabs(e);
        }
    r.lmhead_rel = sum_abs / sum_ref;
    r.lmhead_corr = corr(q, embed_bytes, int(H), q.N);
    r.selfcheck_reference = true;
}

bong added 2 commits November 8, 2026 14:17
The tail-head import added in #1507 decoded the head but skipped the numeric
check the host-head path has always run, so an import of a 1.9 pack reported
"self-check: skipped -- the head is not tied to the embedding" while the pack's
own embedding was right there as an independent reference.

Run it: on a tied model the pack's fp16 embedding must reproduce its 4-bit head,
and an untied one simply comes out low -- either way it is evidence. The
nibble-swapped and no-zero-point decodes stay as the controls that must fail.

Measured on amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu: corr 0.999999, rel_err
0.000700, controls 0.0000 and 0.0000. That is the same evidence the 1.8 pack's
host head carries, and it is what makes the tail layout self-verifying rather
than merely self-consistent.
…ape is the second measured entry

The tail-head read assumed the head's tiles start at a fixed 1,856 bytes into
the tail. That number is only qwen2.5-0.5B's, where 1,856 == 64 + 2*896 by
coincidence: the envelope is the same `64 + 2*hidden` a layer block opens with
(eps, padding, the norm). On Qwen3-1.7B the constant missed the head's start by
2,304 bytes and the import refused -- correctly, since it checks the arithmetic
to the byte instead of trusting the offset. With the envelope derived from the
hidden size, Qwen3-1.7B's tail is 4,160 + 2,374 x 73,984 exactly.

That refusal is what surfaced the second measured 1.9 entry, and the entry is
worth having because the prefix is not `64 + 2*hidden` either: Qwen3 carries its
per-head QK norm gains inside the block before the q/k/v bundle, so the prefix is
8,512 at hidden 2,048 where the same formula would say 4,160. Both numbers are
therefore rule parameters, measured per pack against the 1.8 twin, never derived.

Verified for amd/Qwen3-1.7B_rai_1.9.0_npu against its 1.8 twin:

  * import lands 311 tensors / 210,016 tiles, the same counts as the 1.8 pack,
    with 2,688/2,688 attention tiles and 3,584/3,584 MLP groups surviving
    decode->encode, 56/56 RMSNorm anchors, self-check corr 0.999999 /
    rel_err 0.000764 (controls 0.0000 and 0.0000);
  * greedy text is identical to the 1.8 container on three prompts on the host
    GEMM, and identical again on the NPU with the existing Qwen3-1.7B b8 kernel
    set -- including the one prompt whose continuation differs between backends,
    which is the int8 EP vs fp32 host difference and appears identically in both
    releases.

The two builds' fconsts differ in 535 code bytes, all in layer 24 (0.00006 % of
the file): content, not layout, and not enough to move greedy text.
@bong-water-water-bong
bong-water-water-bong force-pushed the 1bit/npu-fconst-19-selfcheck branch from c1d8c8a to a1799db Compare November 8, 2026 18:18
…d import

Phi-3-mini-4k-instruct_rai_1.7.1_hybrid imports cleanly and then printed

    self-check: lm_head vs the pack's fp16 embedding  corr -0.005766  rel_err 1.638304

which looks like a broken decode and is not: Phi-3 is untied, so lm_head and
model.embed_tokens are separate matrices and correlating them tests nothing. The
container was already right (it leaves tie_word_embeddings absent); only the
wording misled, and it would send someone chasing a phantom on every Phi-3 pack.

The report now distinguishes the two cases. Tied packs keep the existing line and
controls. Untied ones say what they are:

    self-check: not a check here -- this pack is untied (lm_head and the embedding
                are separate matrices), so their correlation -0.005766 and
                rel_err 1.638304 are what an untied pack looks like, not a decode
                failure; the controls still run
    controls:   nibble-swapped corr 0.0015, no zero-point corr -0.0126 (both must fail)

The controls still run in both cases, because they are meaningful either way.

Measured on models/amd-rai/hybrid-probe/phi3mini-1.7.1: 387 tensors / 454,392
tiles, exactly as before -- the import itself is unchanged.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 229ec4379ff96e7c6948390b1a929143f104a1d4, with one small ask that is cheap now and expensive later.

The arithmetic is right, and it is the substantive half. The old code assumed the head's tiles start a fixed 1,856 bytes into the tail; that constant is only qwen2.5-0.5B's:

64 + 2 * 896  = 1856   (qwen2.5-0.5B, hidden 896 — the old hardcoded value)
64 + 2 * 2048 = 4160   (Qwen3-1.7B, the "second measured entry")

and the call site now computes with the envelope rather than the constant (at(fc, tail_off + env + i * tile, tile)). So the fix is "one pack's constant became a formula", which is exactly the shape of defect that reads correctly on the model it was written for and silently misreads the next one.

The self-check now runs on the same terms as the host head, including the two controls that must fail (nibble-swapped decode, no-zero-point decode) — a self-check without failing controls only proves that something decoded. And the untied case is honestly labelled rather than left to look like a pass: self-check: not a check here -- this pack is untied (lm_head and the embedding are separate …).

The ask: pin the formula with the two entries you already measured. The existing test mentions tail 23 times, so the path is exercised — what is not exercised is the thing this PR changes: a fixture whose hidden size is not 896 would fail on the old constant and pass on 64 + 2*hidden. Two cases (1856 and 4160) in npu_pack_import_test.cpp make the next hardcoded constant impossible to reintroduce, and it is the same file I asked for a fixture in on #1412, for the same reason: a parser constant with no row is a constant that can go back.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 229ec437 — the self-check work in this head is good and generalises properly, but my last finding is unchanged, and one new sentence makes it worse.

First, credit where the head earns it — this is more than the follow-up I asked for:

  • prefix is now a measured table field (bytes before the q/k/v bundle), because "Qwen3 puts its per-head QK norm gains inside the block before the bundle, so 64 + 2*hidden is not the rule there" — which is exactly the generalisation I asked for on #1507, done by measuring rather than by asserting.
  • A second measured row joined the table (Qwen3-1.7B, {2048, 16, 28, 8512, 3, 1, 1183744}), so it is now a table and not a special case.
  • The tail envelope is derived, not constant: const int64_t env = attn_payload_off(H); want = env + rows * tile; — 64 + 2*hidden. And the measured consequence is exactly why this matters: the old 1,856 would have been off by 2,304 B on Qwen3-1.7B (4,160 − 1,856), which would have refused the pack rather than mis-parsed it. A constant whose staleness trips its own assertion is the right failure mode, and deriving it removes the trap.

The controls, though, are still not computed on the tail path — and the output now claims they are. Every assignment in the file:

2121:  r.ctrl_corr_nibble = corr(swap, embed_bytes, int(H), 2048);     <- host-side MatMulNBits path only
2122:  r.ctrl_corr_nozp   = corr(nozp, embed_bytes, int(H), 2048);
2776:  … r.ctrl_corr_nibble, r.ctrl_corr_nozp);                        <- tied branch prints them
2785:  … r.ctrl_corr_nibble, r.ctrl_corr_nozp);                        <- untied branch prints them

and the tail block has exactly one corr( call (the lmhead_corr). So a 1.9 import prints

controls:   nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)

from the initalisers — and in the untied case the line above it now says, verbatim, "the controls still run", which is false on that path. That is a stronger version of what I raised last time: not a vacuous pass, but a sentence asserting a check that did not run.

Two ways, both small. Compute the two controls in the tail block — the decode it needs is decode_matmulnbits(..., swap_nibbles=true, false) and (..., false, drop_zero=true) over the same 2048 rows the host path uses, about six lines, and then the summary is true for a 1.9 pack. Or drop the claim: print controls: not run on this path so absence reads as absence. Given the self-check exists to be evidence about the tail layout, I would take the first.

Also: kTailNorm is now dead (one occurrence, the declaration at :228; the envelope moved to attn_payload_off(H)) — same leftover as kFconstHidden in #1507.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 229ec4379ff96e7c6948390b1a929143f104a1d4

My finding is fixed, and the fix is structural rather than cosmetic.

The controls can no longer be printed from struct defaults. The three arms are mutually exclusive on the same flag that gates the computation:

guard:  if (tied || head_geometry) { … compute lmhead_corr, ctrl_corr_nibble, ctrl_corr_nozp … }   // :2119

print:  if (r.selfcheck_reference && r.selfcheck_tied)   -> corr + rel_err + controls   (measured)
        else if (r.selfcheck_reference)                  -> "not a check here -- untied …", the corr numbers, controls (measured)
        else                                             -> "self-check: skipped -- the head is not tied …"  (no 0.0000 at all)

So every path that prints a number is a path that computed it, and the case that has nothing to measure now says so and prints no zeros. That is exactly the class I raised: a 0.0000 that reads as "the control failed to fail" when it was never run.

The untied arm is also the more honest reporting. "their correlation %.6f and rel_err %.6f are what an untied pack looks like, not a decode failure; the controls still run" — a number with its meaning attached, rather than a bare figure a reader would take as a decode score.

Two constants became measured data, and the arithmetic holds:

qwen2.5-0.5B   table prefix 1856  =  64 + 2*896      (the formula happens to be the rule there)
qwen3-1.7B     table prefix 8512  != 64 + 2*2048     -- correct, because Qwen3 carries per-head QK gains
                                                        inside the block, which is why prefix is a table entry
tail envelope  env = attn_payload_off(H) = kNormOff + 2*H = 4160 for hidden 2048
old constant   1856 -> off by 4160 - 1856 = 2304, exactly the "would have missed by 2,304" in the comment

So prefix (per model, measured by diffing 1.9 against 1.8 of the same model) and env (derived from the hidden size) are now separate quantities with separate rules, and the comment says which is which and why the constant was wrong — "1,856 is only its qwen2.5-0.5B value". Replacing a constant that was one model's value with a per-model table plus a derived formula is the right shape; a single constant would have kept passing on the model it came from.

Scope note: the head also carries main's merged work (a rebase across 25 files). The PR's own diff against its merge-base is the two npu/pack_import.* files, +60/−7, and that is what this verdict covers.

Not run: the import itself (needs the 1.9 packs). CI does build npu_pack_import_test, which compiles pack_import.cpp, so the code path here is covered by the build; the numbers above are arithmetic against the code, not a measurement of a pack.

…ck's does not answer garbage

Phi-3-mini-4k-instruct_rai_1.7.1_hybrid imports cleanly (387 tensors, 454,392
tiles) and then answers garbage on the NPU -- "slower slower slower slower" --
and the same garbage on the HOST GEMM, which puts the fault upstream of every
ELF and every artifact.

The cause is one absent key. The forward decides between "logits from the token
embedding" and "logits from a separate lm_head.weight" on `tie_word_embeddings`
and defaults to TIED when the key is absent (npu/forward/src/q4nx_semantics.c).
A genre-A pack ships no config.json at all, so the importer never wrote the key,
so the forward decoded Phi-3's logits from its embedding matrix while the head
tensor it had just imported sat unused in the container.

Measured, same weights and same backend, only the key changed:

    (absent)                     ' slower slower slower slower slower ...'
    tie_word_embeddings: false   ' Paris.\n\n- [Student]: What is the capital'
                                 " 4.\n\nNow, let's calculate the"
                                 " The sky appears blue to us because of the way Earth'"

The container now states it, and where the pack gives no answer the DATA gives
one, by the same correlation the self-check already reports: the imported head
reproduces the embedding (>= 0.99) means tied, a separate matrix means not. So
Phi-3-mini gets `tie_word_embeddings: false` with a `q4nx_tie_note` saying why,
and SmolLM2-135M's hybrid (corr 0.9957) still gets true. A pack that declares the
key is untouched: Llama-3.2-1B and Qwen2.5-0.5B keep the true they ship.

The readout follows the same number, not the pack's declaration, because the two
were disagreeing in the other direction: on a genre-A pack `tied` is false (no
config.json), so a tied pack at corr 0.9957 printed "this pack is untied ... what
an untied pack looks like" while the container it wrote said tied. Both now branch
on the one threshold, and the line says which case it is and that it passed:

    self-check: the head reproduces the pack's fp16 embedding  corr 0.995685  rel_err 0.103587
                (tied: this correlation is the check, and it passed)

Verified that the message and the container field agree on both sides of the
threshold: smollm2-135m hybrid (0.9957) and qwen2.5-0.5b-1.7.1 hybrid (0.9945)
both read tied and both write true; phi3mini-1.7.1 (-0.0058) reads untied and
writes false.

Two other candidates for the same garbage were ruled out first, with numbers, so
this does not get re-litigated later: Phi-3's longrope is plain RoPE at theta
1e4 here (the pack's own cos_cache correlates 1.0000, and the implied inv_freq
0.825282 matches 0.825404), and the untied head decodes correctly (its correlation
against the embedding is -0.0058, which is what an untied head is supposed to give).
@bong-water-water-bong
bong-water-water-bong force-pushed the 1bit/npu-fconst-19-selfcheck branch from e8232e1 to 15d39f4 Compare November 8, 2026 19:12

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 15d39f4b — one item, unchanged from my last review; everything this head added is good, and one of the two new commits is the
best bug story in the queue.

The tie bug is diagnosed and fixed properly. Root cause stated exactly: a genre-A pack ships no config.json, so the importer never wrote
tie_word_embeddings, and the forward defaults to tied when the key is absent (q4nx_semantics.c) — so Phi-3-mini decoded its logits from the
embedding matrix with the head tensor sitting unused in the container, and answered slower slower slower. And the mutation test is the cleanest proof
possible
: same weights, same backend, only the key added → Paris.\n\n- [Student]: What is the capital. Nothing about the decoder changed; the key
alone did.

The decision logic is right in all three cases, which I checked because "infer a structural property from a correlation" is where this usually goes
wrong:

if (tie_declared)      c["tie_word_embeddings"] = tied;              // a pack that declares it is untouched
else if (head_written) c["tie_word_embeddings"] = r.lmhead_corr > 0.99;  // + q4nx_tie_note naming the reason and the number
// else: no key written -> the forward's tied default, which is correct when no separate head was imported at all

That third branch is the one I would have asked for and did not have to: with no head imported the embedding is the head, so leaving the key absent is
the right answer rather than an omission.

The envelope derivation did what a derivation should: 64 + 2*hidden via attn_payload_off(H) refused Qwen3-1.7B first — the old constant missed
the head start by 2,304 B — and the entry was measured only after the refusal. An assertion that catches a stale constant before a table row exists is
exactly the ordering I want.

The one-threshold point checks out arithmetically: > 0.99 gives tied at 0.9957 and 0.9945, untied at −0.0058 — message and field agree on both
sides, which is the disagreement you were fixing. Two small notes on it:

  1. 0.99 is now written in three places (the two selfcheck_tied assignments and the container field). You have just fixed them disagreeing; a named
    constant (constexpr double kTieCorr = 0.99;) would make that agreement structural instead of three literals that currently match.
  2. The residual edge: with head_written true and corr > 0.99 the container says tied even though a separate head was imported, so the forward would
    decode from the embedding and ignore it — and the note would read "the imported head reproduces the embedding" while a separate matrix exists. Narrow
    (it needs a fine-tuned model whose head ≈ its embedding), and the correlation is the best signal available in the ambiguous case, so I would not change
    the rule — just be aware that this is the corner where the inference can be wrong silently.

The item that keeps this at CHANGES is the controls, and the new head makes it more visible rather than less. Every assignment in the file:

2127/2128  r.ctrl_corr_nibble / r.ctrl_corr_nozp = corr(...)   <- host-side MatMulNBits path ONLY
2799       printed in the tied branch
2806       "…the controls still run" printed in the untied branch

The tail (1.9) path sets selfcheck_reference and now selfcheck_tied — so it now routes into those branches — but computes no controls. A 1.9 import
therefore prints controls: … 0.0000, … 0.0000 (both must fail) from the initialisers, and in the untied case asserts "the controls still run".
Same ask as before, ~6 lines: compute them in the tail block (swap_nibbles=true / drop_zero=true over the same 2048 rows), or print
controls: not run on this path. Given this PR's whole argument is that the self-check makes the 1.9 layout evidence rather than self-consistency, the
negative case is the half that carries it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Consumer-lane verification on a real 1.9 pack: the self-check goes from skipped to measured, and it passes

Built 15d39f4b in a private worktree (-DONEBIT_NPU=ON -DONEBIT_NPU_RT_DIR=…, thermal-run) and imported the pre-fetched amd/Qwen2.5-0.5B-Instruct_rai_1.9.0_npu with both binaries, same pack, same flags (--allow-incomplete):

origin/main (b17b2c5) this PR (15d39f4)
self-check line self-check: skipped -- the head is not tied to the embedding, so the pack… self-check: the head reproduces the pack's fp16 embedding corr 0.999999 rel_err 0.000700 (tied: this correlation is the check, and it passed)
fconst line stride 8708288, second header 1115968 … 48/48 RMSNorm anchors found, final norm taken from the tail envelope identical
container config.json tie_word_embeddings: True tie_word_embeddings: True

So on a real 1.9 pack the PR converts an unverified head into a quantified one — corr 0.999999, rel_err 7.0e-4 — and nothing else in the import changes (the fconst geometry and all 48 RMSNorm anchors are bit-identical between the two heads of the build). That is exactly the property the PR claims, measured rather than argued.

Two notes from doing it, offered as evidence rather than review opinion:

  • npu-pack info is unchanged by this PR on all three 1.9 packs I have — SmolLM2-135M still REFUSED: block shape not measured, Hunyuan-0.5B still 168 gaps, Qwen2.5-0.5B still 0 gaps — which is what you would expect since the change is in the import path, not the reader. Worth stating in the PR body so nobody measures info and concludes the PR did nothing.
  • The head correlation is also independent evidence about the 1.9 fconst decode for hidden 896: a wrong tail-head decode could not correlate 0.999999 with the pack's own fp16 embedding. That supports the other open item on the 1.9 line (engine#1503): the remaining blocker after npu-pack: read a Ryzen AI 1.9 Token .fconst in its 1.8 form, head included #1507 is the container weight layout (rm row-major vs the b8 ELFs on this box), not the fconst decode.

Method/limits: one pack, one geometry (h896, 24L, 14/2 heads, head_dim 64), host-side import only — no npu-hold ticket spent, no device run, so this says nothing about what the container does on the NPU.

…s q_proj's width

The measurement tool from #1532 found Qwen3-4B's entry on its first real pair, and
importing with it exposed a second bug the family guard caught before it could
serve wrong text:

  declaring "QKV:2560x6144,O:2560x2560,GU:2560x19456,D:9728x2560"
  this model derives "QKV:2560x6144,O:4096x2560,GU:2560x19456,D:9728x2560"

o_proj's K is q_proj's OUTPUT width, which equals hidden only when
heads*head_dim == hidden. Qwen3-1.7B satisfies that (16 x 128 = 2048), Qwen3-4B
does not (32 x 128 = 4096 against a hidden of 2560), so the family string was
wrong for exactly the models where the two differ. The guard refused the container
rather than serving it, which is the behaviour #1480 was merged for.

Qwen3-4B's 1.9 entry: prefix 9,536, 5 tiles of 1,462,272 rotated by 1, and 2,933
of 2,208,061,248 bytes differ from the 1.8 twin (1.3e-6) -- content, not layout.
The prefix corroborates the Qwen3 offset at a second hidden size: 64 + 2*hidden +
4,352 gives 8,512 at 2,048 and 9,536 at 2,560.

Verified: import 399 tensors / 491,000 tiles, 4,896/4,896 attention tiles and
6,912/6,912 MLP groups survive decode->encode, 108 norm anchors, self-check corr
0.999999 / rel_err 0.000737, and greedy text on the host GEMM:

    'The capital of France is' -> ' in which country? Also, could you explain the difference between'
    '2 + 2 ='                  -> ' 4, but 2 + 2 = 4'
    'Why is the sky blue?'     -> ' What is the scientific explanation for this phenomenon?\n\nThe sky appears'

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 7b9401ce6055d08575b6ddf8717879b51e573fb1 — and the ask in my previous review was already satisfied, which is my mistake to correct.

I asked for "two cases (1856 and 4160) in npu_pack_import_test.cpp". They are there, on main, before this PR:

$ git show origin/main:tests/npu_pack_import_test.cpp | grep -n 'attn_payload_off'
1459:  check(pi::attn_payload_off(896)  == 1856, …)
1461:  check(pi::attn_payload_off(2048) == 4160, "attn off: hidden 2,048 gives 4,160, not 1,856")

with a third case contrasting the old constant (h1 - 1856, "80 tiles + N strip"). So the formula is pinned by a named helper's own unit cases, this PR does not need to add any, and I asked for a row without first checking whether it existed — the same failure I made on #1406 (asking after reading the code instead of running the gate). Noted rather than quietly dropped.

What the head adds since I last read it, all of which reads correctly:

  • a fourth pack entry in the measured table ({2560, 32, 36, 9536, 5, 1, 1462272}) with the corroboration spelled out — Qwen3-4B differs in 2,933 of 2,208,061,248 bytes (1.3e-6), content not layout, and its offset follows the same 64 + 2*hidden + 4,352 prefix while the tile count (5) differs from 1.7B's (3), "which is why this is measured per pack and never derived";
  • the self-check follows the number, not the declaration: r.selfcheck_tied = r.lmhead_corr > 0.99 instead of the pack's tied flag, because a genre-A pack ships no config.json — so the readout used to call those packs untied while the container it wrote said tied. A diagnostic that disagrees with the artefact it just wrote is the class this lane keeps finding, and this is the right direction of fix (the number wins).
  • o_proj's K is q's output width, not hidden — on Qwen3-4B that is 32 × 128 = 4096 against hidden 2560, and the family guard refused the container exactly as it should ("declares O:2560x2560, this model derives O:4096x2560").

Three measured corrections in one PR with the arithmetic shown next to each; nothing left to ask for.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 7b9401ce6055d08575b6ddf8717879b51e573fb1

The two additions are both right, and the new row's arithmetic checks:

Qwen3-1.7B  hidden 2048 -> 64 + 2*2048 + 4352 = 8512   (table: 8512) ✓
Qwen3-4B    hidden 2560 -> 64 + 2*2560 + 4352 = 9536   (table: 9536) ✓

so the rule the comment states is corroborated across two packs, and "the tile count differs from 1.7B's 3, which is why this is measured per pack and never derived" is consistent with the table carrying 3 and 5.

The selfcheck_tied change is the better half: "the diagnostic follows the NUMBER, not the pack's declaration: a genre-A pack ships no config.json, so tied is false there while the head plainly reproduces its embedding, and the readout used to call those packs untied while the container it wrote said tied. Same threshold the container's own decision uses." Making the report agree with the container's decision instead of with a missing declaration is exactly right — the previous version could print "not a check here, untied" for a pack the writer had just recorded as tied, which is a report contradicting its own artifact.

One thing to check in the comment, and my arithmetic is the reason. The offset is attributed to "per-head QK norm gains", but 4,352 is the same constant for a 16-head and a 32-head model:

per-head QK gains as the importer plans them:  self_attn.{q,k}_norm.weight, BF16 [head_dim] each
   -> 2 * 128 * 2 = 512 bytes per layer for head_dim 128, in both models
4352 is head-count-independent, so it is not "per-head" anything

The rule is fine (two packs corroborate it) and the table is right either way; what I would fix is the attribution, because a reader adding the next Qwen3 pack will try to derive 4,352 from heads × head_dim and get 512. Either name the decomposition (the ~3,840 bytes that are not the two gain tensors — section headers, eps/padding?) or say "a fixed per-block prefix term, measured, origin not identified". A number in a comment that cannot be re-derived from the mechanism it names is the kind of thing that gets copied wrong.

Not run: a Qwen3 1.9 pack to measure the new row independently (this box has Qwen3-1.7B_rai_1.8.0_npu_4K and Qwen3-4B_rai_1.8.0_npu_4K but no 1.9 twins), so the row itself is the author's measurement and only its arithmetic is mine.

… name

Review finding, and it was right: the tail (.fconst) path set selfcheck_reference
and selfcheck_tied, so it routed into the reporting branches, but it computed no
controls -- so a 1.9 import printed

    controls:   nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)

straight from the struct initialisers, and in the untied case asserted "the
controls still run" when nothing had run. The negative case is the half of this
PR's argument that carries it, so printing its numbers from defaults is exactly
the wrong failure.

The tail path has no MatMulNBits bytes to mis-decode, so its control is an
ALIGNMENT one, on the same tiles and through the same decoder: each head row
matched against the NEXT embedding row. A correct decode correlates ~0.999999
with the embedding; a misaligned head must not, and does not:

    controls:   row-shifted corr 0.0716 (must fail)

The host path keeps its nibble-swapped / no-zero-point controls, which are the
right ones there because those bytes exist. Each path now prints only what it
measured.

Also from the review: the 0.99 tied threshold is now `constexpr double kTieCorr`,
used by the two selfcheck_tied assignments, the container field and its note --
three literals that had just been reconciled by hand, which is how they drifted
apart in the first place. The reviewer's other point, that a tied-by-inference
container with a separate head imported decodes from the embedding and ignores
that head, is left as the reviewer advised: the correlation is the best signal in
the ambiguous case, and the note says which way it was decided and why.
@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

The blocking item is fixed at b43589b — and you were right that the new head made it more visible rather than less.

The tail path now carries its own control. It has no MatMulNBits bytes to mis-decode, so an alignment control is the honest analogue: the same tiles, the same decoder, each head row matched against the next embedding row. A correct decode correlates 0.999999 with the embedding; the misaligned one:

controls:   row-shifted corr 0.0716 (must fail)

The host path keeps nibble-swapped / no-zero-point, which are the right controls there because those bytes exist. Each path prints only what it measured, so the 0.0000, 0.0000-from-initialisers case is gone, and the untied branch can no longer assert "the controls still run" without running them.

kTieCorr adopted (constexpr double kTieCorr = 0.99), used by both selfcheck_tied assignments, the container field and its note — three literals that had just been reconciled by hand is precisely how they drifted apart.

Your residual edge left as advised: with head_written and corr > 0.99 the container says tied while a separate head exists, so the forward decodes from the embedding and ignores it. It needs a fine-tune whose head ≈ its embedding, the correlation is the best signal available there, and the q4nx_tie_note records which way it was decided and why — changing the rule would trade a rare silent case for a common one.

Two commits since 15d39f4 are also in this head and not covered by your review: Qwen3-4B's measured entry and o_proj's K in the kernel family (it was pad_k(hidden), but o_proj's K is q_proj's width — wrong for every model where heads*head_dim != hidden; the family guard refused the container rather than serving it).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at b43589beb1b55c6f67d3ae4ffff15333fb054028

Two changes, and the second is the one that makes the first meaningful.

The threshold is now named (constexpr double kTieCorr = 0.99) with the reason stated — "the container's field, the readout and the note all have to agree on it: three copies of 0.99 is how they drifted apart before." Three literals that must agree is exactly the shape that drifts, and giving it one name is the fix that cannot be forgotten.

The untied path gains a real negative control — and this is the part that closes the loop I opened two rounds ago: instead of printing the host path's nibble-swap and zero-point controls (which this path cannot run, "there are no such bytes here"), it computes a row-shifted correlation: the same tiles and the same decoder, with each head row matched against the next embedding row.

for rr in [0, min(2048, q.N-1)):  x = decode(head row rr)   y = embed row (rr+1)
ctrl_corr_rowshift = corr(x, y)        printed as "row-shifted corr %.4f (must fail)"

That is a control with a known wrong answer: a decoder that is reading the right tiles in the right order must collapse when the rows are shifted by one. It is a stronger check than the nibble-swap on this path because it perturbs the addressing, which is the thing a genre-A split layout gets wrong. And the guard if (r.ctrl_corr_rowshift >= 0) means the line is only printed when the control actually ran — so no struct-default 0.0000 can appear, which is the same class the earlier round fixed for the other two controls.

c["tie_word_embeddings"] = r.lmhead_corr > kTieCorr; now shares the constant with the readout, so the container's decision and the printed verdict cannot disagree.

My earlier note stands unchanged and is not a blocker: 4,352 is head-count-independent, so "per-head QK gains" cannot be its explanation; the rule and both table rows are right on their arithmetic (8512 = 64+2·2048+4352, 9536 = 64+2·2560+4352).

Not run: an import against a real pack at this head (the pack leg needs a 1.9 pack this box does not have for Qwen3); the control's code path and its print guard are what I verified.

…xplanation of the prefix

The measurement tool found this entry on a non-Qwen family for the first time:
{2048, 32, 16, 8512, 5, 1, 591872}, with 1,857 of 686,518,336 bytes differing from
the 1.8 twin (2.7e-6 -- content, not layout).

It also corrects a comment I wrote two entries ago. I had explained the extra 4,352
bytes in the prefix as Qwen3's per-head QK norm gains sitting inside the block before
the bundle. Llama-3.2-1B has no QK norms at all and carries the same prefix at the
same hidden size (8,512 at hidden 2,048), so the extra is a property of the geometry
and not of the architecture. The table keeps doing its job -- it is measured -- but
the explanation next to it was a plausible story that would have become the reason
some future row was added wrongly. That is the second time this week a tidy
explanation survived exactly one model.

What differs from Qwen3-1.7B at the same hidden size is the bundle: 5 tiles of
591,872 against 3 of 1,183,744, so the two models cut the same payload into
different pieces and the tile count alone identifies neither.

Verified: import 147 tensors / 150,848 tiles (the same counts as its 1.8 twin),
1,280/1,280 attention tiles survive decode->encode, self-check corr 0.999999, and
greedy text on the host GEMM:

    'The capital of France is' -> ' Paris.\nThe capital of France is Paris. \nThe capital'
    '2 + 2 ='                  -> ' ?\nTo add 2 + 2, you simply count'
    'Why is the sky blue?'     -> ' The sky appears blue because of a phenomenon called Rayleigh scattering'
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Re-verdict request at 0019f0a4, one commit past the head you approved (b43589b).

The delta is a measured table row and a corrected comment, no logic: Llama-3.2-1B-Instruct 1.9 — {2048, 32, 16, 8512, 5, 1, 591872}, measured by the merged tool (1,857 of 686,518,336 bytes differ, 2.7e-6 — content, not layout). Import verified: 147 tensors / 150,848 tiles, the same counts as its 1.8 twin, 1,280/1,280 attention tiles through decode->encode, self-check 0.999999, and correct greedy text on the host GEMM.

It also refutes the explanation I gave you for prefix. I had written that the extra 4,352 bytes are Qwen3's per-head QK norm gains sitting inside the block. Llama-3.2-1B has no QK norms and carries the same 8,512 at the same hidden size, so the extra is a property of the geometry and not of the architecture. The row is measured either way; the comment beside it no longer claims a cause it cannot support, which is the same class of error as 'the 1.9 payload is re-tiled' — a plausible story that survived exactly one model.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 0019f0a4.

The controls item is not just fixed — it is fixed better than I asked. I said "compute the two host controls on the tail path, or print controls: not run". What is there instead is the right control for that path:

// The control this path can actually run: the same tiles and the same decoder, with each row shifted
r.ctrl_corr_rowshift = …          // computed inside the .fconst-tail block
…
if (r.ctrl_corr_rowshift >= 0)  printf("  controls:   row-shifted corr %.4f (must fail)\n", …);
else                            printf("  controls:   nibble-swapped corr %.4f, no zero-point corr %.4f (both must fail)\n", …);

with the same gate in both branches (tied and untied). So on a 1.9 import the printed control is one that was computed from those tiles; on a host
import rowshift is the -1 sentinel and the nibble/zero-point pair — which is computed there — is printed. No path prints a default any more, and
"the controls still run" is now true where it is printed. A sentinel distinguishing "measured" from "not measured" is exactly the shape I was asking for,
and the control itself ("the same decoder, rows shifted") is a better negative than the nibble swap for a tail-sourced head.

kTieCorr is in, with the reason written next to it ("three copies of 0.99 is how they drifted apart before") ✔ — that closes my other nit structurally.

And one correction I owe you: the prefix explanation I repeated in my last review was wrong, and your new comment says why. I had echoed "Qwen3 puts
its per-head QK norm gains inside the block before the bundle". Llama-3.2-1B has no QK norms at all and carries the same +4,352 at hidden 2,048 —
so the extra is geometry, not architecture. Your new table row also corroborates the corrected claim from the data: Qwen3-1.7B and Llama-3.2-1B are the
two hidden-2,048 entries and both carry prefix 8,512.

The table is becoming a real table — four measured rows now (SmolLM2, Qwen3-1.7B, Qwen3-4B at 1.3e-6 residual, Llama-3.2-1B as the first non-Qwen
family), each measured rather than derived, with the residual recorded per row.

One leftover, cosmetic: kTailNorm is still declared and unused (one occurrence), since the envelope became attn_payload_off(H).

@bong-water-water-bong
bong-water-water-bong merged commit be33e50 into main Nov 8, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/npu-fconst-19-selfcheck branch November 8, 2026 21:20

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: APPROVE. Every constant in it checks, one of them against a measurement I made in a different lane, and the PR's own thesis ("measured per pack, never derived") is now one step away from being unnecessary for its last magic number.

The arithmetic, verified: 64 + 2·896 = 1,856 (the qwen2.5-0.5B envelope, which is why that constant looked general — hidden 896 is the only size where the old hardcoded 1,856 and the real rule coincide); 64 + 2·2,048 = 4,160 (the 1.7B tail base); 4,160 + 4,352 = 8,512 and 64 + 2·2,560 + 4,352 = 9,536 — so the Qwen3 prefix formula is confirmed at two hidden sizes, which is the difference between a fitted constant and a rule.

The o_proj K finding is the best thing here and I would generalise the sentence. o_proj's K is q_proj's output width, which equals hidden only when heads·head_dim == hidden — true for 1.7B (16·128 = 2,048) and false for 4B (32·128 = 4,096 ≠ 2,560). That means the old family string was wrong for a whole class of models where the dimensions diverge, which is the common case for anything with heads·head_dim > hidden — i.e. the bug was not one bad constant but an identity assumption that holds only on models with no unused head capacity. Two consequences worth stating in the code: (a) name the invariant (O.K == Q.N, not O.K == hidden) where the family string is built, so the next reader sees which of the two it may rely on; (b) the guard is the hero — "refused loudly instead" is what turned a silent wrong-answer class into an error. Is that guard exercised by a negative test, i.e. one that feeds a deliberately wrong family string and asserts the refusal? If not, it is the single most valuable test this PR could add, because the guard's value is exactly its future behaviour on packs this lane has never seen.

One question about the last magic number, and it comes from my own instrument. 4,352 = 2 × 2,176. On #1410 I ran a zero-run grid over the o_proj section and the gap histogram was {2176: 567, 6416: 10, 112: 9} — 2,176 dominant by two orders of magnitude, with boundary pairs summing to 6,528 = 3 × 2,176. So the Qwen3 prefix offset is exactly twice a quantum that appears all over the neighbouring structure. If that is the same 2,176, then the prefix is not a measured constant at all but 64 + 2·hidden + 2·q for some q with a meaning (a pair of gains plus their record?), and deriving it would remove the only number in the PR that a reader cannot check. I am not claiming they are the same unit — I have not read the Qwen3 blocks myself — only that the coincidence is exact, that the measurement to settle it is cheap, and that "measured per pack" is a fallback, not an ideal, for a constant that has an explanation.

One ask on item 1's untied case. "on an untied one the number simply comes out low" is right, but the check should also say which situation it believes it is in — tied-and-passing versus untied-and-therefore-inapplicable versus tied-and-failing. As written a genuinely broken tied import prints a low number and is indistinguishable from the honest untied model, which is the same silence class as the .fconst fix in #1533: the check runs, reports a number, and nothing tells the reader whether that number was supposed to be high.

I reviewed this from the body and the arithmetic; I did not run the importer. Noted for the release: #1533 says the two must land in the same release, either order — this PR alone imports the split-qkv packs' heads but leaves the tie key absent, and the consequence it names for that state ("silently wrong for an untied split-qkv model") is the reason the pairing belongs in both PR bodies, which #1533 already does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants