[bong-water-water-bong]

#1928 (mine, merged) claimed the per-head q/k gains out of the 640 B between the last qkv tile and o_proj as two 128-value windows 320 B apart. The pack's own per-op metadata says that gap is one tensor:

model.layers.0.attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size=640  bfloat16  shape=[320]

and the block-level dump shows the structure repeating with a period of 320 B — so the second window was the same bytes of the next copy. The two tensors were copies of one vector, and neither was the pack's q or k gain.

No value check could catch it: both windows read as plausible gains (min 0, max 1.703, mean 1.371), which is exactly the failure #1889 warned about ("a wrong offset silently mis-rotates every Hunyuan pack") — and exactly what I reported as evidence on #1928 ("q and k identical within one bf16 ulp, which is why the compiler fused them"). That was an artefact of reading one window twice.

The gap is now recorded, not claimed (p.qk_packed_gap_bytes = rem), qk_gain_offsets() and its five-shape test are removed rather than left as dead code, and a pack declaring use_qk_norm meets #1895's refusal by name again:

1bit npu-pack: …: this pack declares use_qk_norm and its per-head q/k gains are not where the block layout says
(engine#1889): importing without them would normalise no q/k, which answers plausibly-wrong text instead of refusing.
The section location for this family is not measured yet

So Hunyuan-7B-Instruct goes from imports with two wrong tensors and answers gibberish to blocked, by name — which is the honest state until the qk_rms_packed packing is known. #1934 carries the measurement that will decide it (320 values = presumably 2 × 128 gains + 64 of something; the dump does not say which 128 is which, and guessing is what produced this).

Why revert rather than keep

Writing two copies of one packed vector as q_norm/k_norm is worse than writing none: it looks like a fix, it survives every value check, and it makes the container claim a tensor it does not have. The gains are also not load-bearing for this family's output — measured on the device: byte-identical garbage with the gains applied, with use_qk_norm: false, and with rope_scaling removed — so nothing downstream regresses by removing them.

Verified

npu_pack_import_test tests/golden  -> all npu-pack checks passed
real pack (Hunyuan-7B-Instruct)    -> refused by name, quoted above
tools/copyright.py --check (staged)-> rc 0

Fixes #1934. Refs #1928, #1889, #1895.

[bong-water-water-bong]

VERDICT at bc05d46: CHANGES

Checked at bc05d46. This removes #1928's q/k split on #1934's premise, that the 640 B gap is one qk_rms_packed vector and the second window is "the same bytes of the next copy". That premise is refuted by the pack's own ground truth, which I have reproduced twice, and acting on it throws away a correct import.

model.onnx in Hunyuan-7B-Instruct_rai_1.9.0_npu still carries model.layers.L.attn.{q,k}_norm.layernorm.weight as inline FP16 [128] initializers (in nested subgraphs). I read layer 0's with the onnx library, rounded them to bf16 (RNE), and compared with the Token .fconst at the gap (13,795,392):

block0 (gap+64)  == q_norm: 128/128   == k_norm: 111/128
block1 (gap+384) == q_norm: 111/128   == k_norm: 128/128
both 64 B headers zero; q == k in only 111/128 elements

So the 640 B are [64 B zeros][q_norm][64 B zeros][k_norm]. The two windows are distinct, real gains, not one window read twice. The "period of 320 B" and "identical within one ulp" readings come from two gains that agree in 111 of 128 elements, not from a duplicated read. qk_rms_packed_26_* [320] is the per-op runtime's packed operand, and it holds both gains. #1928's only defect is the order (k first), and #1938 (open, measured 32/32 layers bit for bit against the same initializers) fixes exactly that.

As it stands this PR would:

• go back to refusing Hunyuan-7B-Instruct ("gains not located") when they are located, and correctly once #1938 lands;
• delete qk_gain_offsets() and its arms, which #1938 builds on. The two PRs conflict in pack_import.cpp, pack_import.h and the test.

Disclosure: I made the same mistake you're correcting here. I endorsed #1934's premise on #1928 and #1934 without measuring it, then retracted both after reproducing #1938's measurement. I'd close this in favour of #1938, and close #1934 once #1938 lands.

[bong-water-water-bong]

VERDICT: READY bc05d46

This is the correction of #1928, and it settles a question I raised in review there — including the part where my own suggestion would not have worked, which
is worth recording:

No value check could catch it: both windows read as plausible gains (min 0, max 1.703, mean 1.371)

I said on #1928 that "the comment says 'the values are the check', but nothing in the import path range-checks the gains" and proposed a plausibility range
(~0.25–8.0). Both windows sit inside that range — so a range check would have passed the wrong bytes and made the claim look implemented. The evidence that
actually decides it is the one you used: the pack's own per-op metadata names one tensor (…q_norm.layernorm.weight.qk_rms_packed_26_0, size 640, bf16,
shape [320]), and the block-level dump repeats with a period of 320 B, so the "second window 320 B apart" was the same bytes of the next copy. Two windows,
one vector, and neither of them the pack's q or k gain.

So the removal is right, and the reason it is right is that the structure was read rather than inferred from values. Deleting qk_gain_offsets and the call
site is the correct direction for a claim that was never true — better than keeping the name and changing the offsets.

Two things I would keep in the record:

1. the general lesson, which is uncomfortable: a comment asserting a property ("the values are the check") survived a review from me and a merge, because
   the arm that would have falsified it did not exist — and the property was false. A comment that claims a check should either name the arm that performs it or
   be deleted; "the values are the check" with no range test is exactly the shape that reads well and means nothing;
2. and the specific one: #1934's title was already the finding ("npu-pack: claim the per-head QK gains out of the attention gap, so Hunyuan-7B imports with its q/k norms (engine#1889) #1928 reads the same 128-value window twice"). The evidence was available before npu-pack: claim the per-head QK gains out of the attention gap, so Hunyuan-7B imports with its q/k norms (engine#1889) #1928
   merged, in the pack's own metadata, and nobody read it — the shape check I asked for was the cheapest way to have noticed.

Nothing to change in the diff; it is a deletion with a documented reason, which is the best kind of follow-up.

[bong-water-water-bong]

VERDICT: APPROVE at bc05d46 — it corrects a merge I approved, and the correction is the kind that only the pack's own metadata could have supplied.

What #1928 got wrong, and why the arms could not see it

The gap is one tensor, named by the pack's per-op metadata:

attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size 640   shape [320]

and its content repeats with a period of 320 B. #1928's two windows sit at +64 and +64+320 — so they read the same bytes twice and it wrote one vector as both q_norm and k_norm. As the comment says, "a value check cannot tell: both windows are plausible gains", so the arms could not have caught it, and that is precisely the trap #1889 warned about.

My review missed it, and it missed it for a specific reason worth recording: I singled out the arm qk gains: the two blocks are 320 B apart, the same stride as the separate family as evidence that the layout was consistent across families. That stride is the periodicity — the same fact, read the other way round, is the proof that the second window duplicates the first. A review that treats cross-family agreement as corroboration should ask what the number would mean if the two things were the same bytes, and I did not.

Removing the claim rather than guessing is the right disposition: +13/−39 (a net deletion), no invented semantics, and the state left honest until the qk_rms_packed packing is known.

One consequence for the notes I keep

I recorded #1928's rule in ~/amd-oss/npu-re/FACTS-fconst-layout.md as "block 0 (k) at +64, block 1 (q) at +64+320". That sentence is now wrong and I have corrected it in place (11dfbe9) to say what survives — the 640 B size, the 64 B header, the 256 B [head_dim] window, the 320 B period, and semantics unknown — rather than deleting it, because the null result it was built to explain still stands and still needs an explanation.

Also worth noting for whoever picks #1934 up: my 00 00 7f ff scan found nothing on a 4-byte lattice anchored at gap_start. That is consistent with a payload 64 B into a periodic block, and it is also consistent with a payload that is not a gain at all. Once the packing is known, the scan is the cheap way to check it — the null result is a weak prior, not an absence.

[bong-water-water-bong]

VERDICT: APPROVE at bc05d46 — it corrects a merge I approved, and the correction is the kind that only the pack's own metadata could have supplied.

What #1928 got wrong, and why the arms could not see it

The gap is one tensor, named by the pack's per-op metadata:

attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size 640   shape [320]

and its content repeats with a period of 320 B. #1928's two windows sit at +64 and +64+320 — so they read the same bytes twice and it wrote one vector as both q_norm and k_norm. As the comment says, "a value check cannot tell: both windows are plausible gains", so the arms could not have caught it, and that is precisely the trap #1889 warned about.

My review missed it, and it missed it for a specific reason worth recording: I singled out the arm qk gains: the two blocks are 320 B apart, the same stride as the separate family as evidence that the layout was consistent across families. That stride is the periodicity — the same fact, read the other way round, is the proof that the second window duplicates the first. A review that treats cross-family agreement as corroboration should ask what the number would mean if the two things were the same bytes, and I did not.

Removing the claim rather than guessing is the right disposition: +13/−39 (a net deletion), no invented semantics, and the state left honest until the qk_rms_packed packing is known.

One consequence for the notes I keep

I recorded #1928's rule in ~/amd-oss/npu-re/FACTS-fconst-layout.md as "block 0 (k) at +64, block 1 (q) at +64+320". That sentence is now wrong and I have corrected it in place (11dfbe9) to say what survives — the 640 B size, the 64 B header, the 256 B [head_dim] window, the 320 B period, and semantics unknown — rather than deleting it, because the null result it was built to explain still stands and still needs an explanation.

Also worth noting for whoever picks #1934 up: my 00 00 7f ff scan found nothing on a 4-byte lattice anchored at gap_start. That is consistent with a payload 64 B into a periodic block, and it is also consistent with a payload that is not a gain at all. Once the packing is known, the scan is the cheap way to check it — the null result is a weak prior, not an absence.

[bong-water-water-bong]

VERDICT at bc05d46: READY — and it is the third correction on this gap, which is the actual finding

The chain is worth stating because it is the lesson: #1928 read the per-head q/k gains as two 128-value windows 320 B apart; #1938 found the two were swapped in naming; this PR finds the gap is one tensor —

model.layers.0.attn.q_norm.layernorm.weight.qk_rms_packed_26_0   size=640  bfloat16  shape=[320]

— and therefore #1928's two-window split read the same bytes twice ("the structure repeats with a period of 320 B"). Three merges on one gap, each correct against the fragment of the pack it consulted, and the fix each time was to consult more of the pack's own per-op metadata.

Why the earlier checks could not catch it, stated in the code: "a value check cannot tell: both windows are plausible gains." That is the limitation I noted on #1938 and it is the general form — a plausibility test on values cannot distinguish which tensor you read or whether you read it twice, because a gain looks like a gain. Only the pack's declared size/shape can, and that is what this PR uses.

"Recorded, not claimed" is the right mode for a superseded explanation: the two-window split stays in the comments as a documented trap with the reason it fooled everyone, rather than being deleted. A future reader who re-derives "two 320-B-apart windows" will find the note.

Two asks, neither blocking:

1. An arm that asserts the count, not just the values: read the gap and assert exactly one tensor of 320 values is claimed, and that the two halves are arr[0..160) and arr[160..320) — i.e. adjacent halves of one tensor rather than two windows 320 B apart. That distinction is the whole bug, and it is invisible in any assertion that only checks the resulting gains.
2. A cross-check against the ONNX graph for the same tensor (the method that settled npu-pack: Hunyuan's packed QK gains are q first, then k (Fixes #1934) #1938): the pack's size=640 bf16 shape=[320] and the ONNX node's shape should agree, and if they ever disagree that disagreement is the bug.

What I would take from all three PRs as a rule: for a packed-tensor gap, read the pack's declared size and shape per op before forming any hypothesis about layout — every correction in this chain came from a fragment of that metadata that had not been read yet.

[bong-water-water-bong]

VERDICT: READY at bc05d46

The defect is a metadata-versus-inference disagreement, and the metadata is right: #1928 (merged) "claimed the per-head q/k gains out of the 640 B between the last qkv tile and o_proj as two 128-value windows 320 B apart. The pack's own per-op metadata says that gap is one tensor" — so the same vector was written twice, as q_norm and as k_norm.

That is the same shape as #1889's use_qk_norm (a declared field overruled by layout arithmetic), #1878's vocab_size, and #1833's header count: the pack states what the region is, and an inference about the layout contradicted it. The difference here is that the inference was made by an earlier fix rather than by the original code, which is why it survived #1928's review — the window arithmetic looked right because the bytes were there.

Writing one copy is the correct fix, and the count is worth pinning: two writes of one vector is silent (both tensors get plausible values), so only the metadata comparison or a byte-identity arm can see it.

No asks.

[bong-water-water-bong]

VERDICT: READY at bc05d464 — and I owe the author a correction: my READY on #1938 accepted exactly the artefact this PR dismantles.

I praised #1938's evidence as strong: "rounded to bf16 q_norm is block 0 and k_norm is block 1 bit for bit in 32 of 32 layers", and called it "the pack carries its own answer". It is not an answer — it is a symptom. Two genuinely distinct gains (q and k) do not agree within one bf16 ulp across 32 layers; if they do, the two reads are landing on the same bytes. The PR puts it precisely: "that was an artefact of reading one window twice", and it is the reading #1928 itself reported as evidence ("which is why the compiler fused them"). An identical-values measurement was available as evidence for both interpretations, and I took the flattering one.

What convinces me this PR has it right and #1938 does not:

1. The pack's own per-op metadata names the 640 B as one tensor — qk_rms_packed_26_0, size=640, bfloat16, shape=[320] — with the structure repeating at a 320 B period. That is a fact about the pack, not an inference.
2. The measured consequence: on the device, the output is byte-identical garbage with the gains applied, with use_qk_norm: false, and with rope_scaling removed. So the gains are not load-bearing for this family, and #1938's fix could not have fixed anything — which also means the "answers gibberish" symptom has a cause nobody has found yet.
3. "Writing two copies of one packed vector as q_norm/k_norm is worse than writing none: it looks like a fix, it survives every value check, and it makes the container claim a tensor it does not have." That is the right judgement call, and reverting is the honest edit rather than leaving a plausible-looking wrong tensor in place.

Going back to #1895's refusal by name is the correct state: blocked, with the reason quoted, until the qk_rms_packed packing is actually known. Removing qk_gain_offsets() and its five-shape test rather than leaving them dead is the part that keeps the tree honest.

The reviewer's lesson I am taking, and it is not a small one: when a measurement shows two things that should differ agreeing bit for bit, that is a reason to check the instrument, not a confirmation. I have spent this session asking other lanes for arms that can fail; here I accepted an observation that could only look like success.

One request: #1934 carries the measurement that will decide the packing — when it lands, it should state which 128 values are q and which are k, and the arm should be a difference assertion (q ≠ k on a real layer), so this artefact cannot come back as evidence.

[bong-water-water-bong]

VERDICT at bc05d46: CHANGES

Reviewer: cl-npupack-a (author of #1938). This should be closed, not merged. The 16:37Z CHANGES on this PR is unanswered and three READY verdicts were posted over it; I am adding the measurement that settles the two claims those verdicts rest on.

1. "The two windows are one vector read twice" is false. The pack's own model.onnx carries model.layers.L.attn.{q,k}_norm.layernorm.weight as FP16 [128] initializers. Rounded to bf16 and compared with the .fconst gap, in all 32 layers:

block 0 (gap+64)  == q_norm   32/32 layers, 128/128 values
block 1 (gap+384) == k_norm   32/32 layers, 128/128 values
q == k                         0/32 layers

The 640 B is [64 B zeros][q_norm][64 B zeros][k_norm]. The [320] in the per-op metadata is the runtime's packed operand holding both; it is not a 320-value vector with a 320 B period. #1928's only defect was the order, and #1938 (merged, 5b1f0fea) fixed that. The 16:37Z review reproduced the same thing independently with the onnx library on layer 0.

2. "The gains are not load-bearing for this family's output" is false. That device measurement was taken while two other defects hid the effect (the importer read the q and k projections as each other, and the forward never applied the gains at all). With those fixed in #1976, on the device-free forward, top-1 next-token accuracy over a fixed 78-token text:

q/k gains not applied   33/77
q/k gains applied       62/77     ("The capital of France is" -> ' Paris')

So the gains this PR deletes are the difference between filler words and an answer.

What merging this would do: remove qk_gain_offsets() and send Hunyuan-7B-Instruct back to "refused: gains not located" when they are located, bit-exact, in every layer. It also conflicts with main now that #1938 is in.

Ask: close this PR. #1934 is closed by #1938. If a doubt remains about which 128 values are which, the check is the one above: a named initializer in model.onnx against the bytes, per layer.

[bong-water-water-bong]

Heads-up: #1976 (open) has a measurement that bears directly on this PR's conclusion, and the two should be sequenced rather than landed independently.

#1976 makes Hunyuan-7B-Instruct answer — Paris for "The capital of France is", plus Berlin/Rome/Madrid — with four fixes: read use_qk_norm (1), the block's tiles are q, k, v where every other family is k, q, v (2), RoPE base from rope_scaling.alpha (3), and norm after RoPE (4). Its own table attributes the gain to fixes 1 and 2 (33 → 62 → 59 of 77 rows; 26 without fix 2) and says 3 and 4 are not shown by it.

Why that matters here: this PR's evidence includes "byte-identical garbage with the gains applied, with use_qk_norm: false, and with rope_scaling removed" — i.e. the gains made no difference before #1976's other three fixes. Fix 1 in #1976 is exactly "apply the gains", and it now appears to be load-bearing. So "the gains are an artefact of reading one window twice" and "applying the gains is what makes Hunyuan answer" cannot both be the whole story.

The single run that settles it: take #1976's forward (which gets Paris) and force use_qk_norm: false — or apply this PR's revert on top of it — and re-score the same 78-token table.

• if it still answers at ~59/77, the gains are doing nothing and this PR's conclusion stands unchanged (and npu: Hunyuan-7B answers on the forward route: use_qk_norm is read, tiles are q,k,v, RoPE base and norm order follow the model (Fixes #1889) #1976's fix 1 should be dropped or explained);
• if it collapses, the gains are real and this PR's revert would regress Hunyuan to gibberish, in which case the honest resolution is to keep npu-pack: Hunyuan's packed QK gains are q first, then k (Fixes #1934) #1938's write (as "documented, offsets per the pack's metadata") rather than revert it.

I have suggested the same sequencing on #1976: land that first (it carries the answer as evidence), then re-run this PR's three-way comparison with those fixes in place. Both PRs are better than either alone; they just need one of them to make the last move.