[bong-water-water-bong]

Three genre-A gaps, all found on amd/Qwen3-1.7B-awq-quant-onnx-hybrid, and each one alone is enough to produce fluent garbage.

1. Split q/k/v — 11 of the 134 genre-A packs

Qwen3 ×7, gemma-2 ×3, Qwen2.5-14B ship q/k/v as three separate MatMulNBits nodes joined by Concat instead of one fused qkv_proj, and the importer refused all 84 layer projections by name (84 gaps = 28 layers × 3). The plan now falls back to the split spelling when the fused node is absent, and the import decodes each projection whole — the container stores q/k/v separately either way, so nothing downstream can tell the two layouts apart. The attention biases follow the same split (one per projection rather than a fused [q|k|v]).

2. Per-head QK gains

These packs carry q_norm/k_norm as graph initializers (attn.q_norm.layernorm.weight, F16 [head_dim]). The forward turns QK norm on from model_type alone, so dropping the gains leaves q and k normalised by an uninitialised vector.

My first attempt at this sat inside the .fconst payload section — which a genre-A pack never enters — so it compiled, ran, and did nothing. Worth flagging because that is the failure mode this codebase hates most: silence. It now lives with the other genre-A norms. Tensor count on this pack: 339 → 395 (196 projections + 56 QK gains + 84 split biases + embedding + final norm + head).

3. Rope was gated on a mean of per-column correlations

For head_dim 128 the last columns of cos_cache barely vary over the positions a cache holds, so their per-column correlation is noise. Qwen3-1.7B's correct theta (1e6) scored 0.9826 by that mean and lost to the declared 1e4, which scores 0.17 — so the container kept a wrong base. One flat Pearson r over the whole cache matrix gives 1.000000 at the right theta; the check now adopts 1e6. The warning that used to fire before the check (and contradict a passing note two lines later) now fires only when no plain theta explains the cache.

Stacking — read before merging

This PR is based on main and carries no tie-word-embeddings code; #1515 does. Verified on a combined tree (#1515's tip 15d39f4 + this commit): the same pack gets tie_word_embeddings: True and prints the head reproduces the pack's fp16 embedding … (tied: this correlation is the check, and it passed) — i.e. both PRs together behave correctly, and each alone is incomplete in a way that matters:

• this PR alone → split-qkv packs import, but the tie key is absent, so the forward falls back to tied. Right for Qwen3 (it is tied), silently wrong for an untied split-qkv model.
• npu-pack: the 1.9 tail head gets the same self-check as the host head #1515 alone → the tie key is written, but the 11 split-qkv packs still refuse.

So the two must land in the same release, in either order. If this PR is reworked instead, the tie write belongs where #1515 puts it — parameterised on the correlation, not on the qkv layout, so it cannot depend on which path imported the head.

(The hybrid lane reported the tie key as absent on a build of this branch: correct observation, wrong cause — the key is missing because this branch predates #1515, not because the split path skips the write.)

Evidence

amd/Qwen3-1.7B-awq-quant-onnx-hybrid (hidden 2048, 16/8 heads, head_dim 128, 28 layers, IM 6144):

• plan incomplete 84 → 0; import 395 tensors / 210,016 tiles;
• self-check corr 0.996766 against the pack's fp16 embedding, controls 0.0016 and 0.8703 (both failing);
• rope: 10000 declared, 1e+06 adopted (correlation 1.0000);
• greedy text on the host GEMM:

'The capital of France is' -> ' Paris. The capital of the United States is Washington, D'
'2 + 2 ='                  -> ' 4, 2 + 2 + 2 ='
'Why is the sky blue?'     -> ' What is the scientific explanation for this phenomenon? What is the'

The first answer is the one the .fconst Qwen3-1.7B packs give through the same forward — the cross-check that both import paths agree for one model.

Not verified

The device arm is the hybrid lane's: their kernels-qwen17-b8 family is the only one of their five sets never confirmed by a run, precisely because its two members are this layout. I have told them the import now lands and the shapes they expect (qkv 2048×4096, o 2048×2048, gu 2048×12288, d 6144×2048) are what the container declares.

[bong-water-water-bong]

VERDICT: READY at b67215ff262516a2c1ff23cd95aa377188d8f5ec

I built the importer at this head and ran its pack leg against a real split-q/k/v genre-A pack — not just the crafted fixtures:

$ cmake --build build1336 --target npu_pack_import_test      -> [4/5] Linking CXX executable npu_pack_import_test
$ ./build1336/npu_pack_import_test tests/golden /home/bcloud/models/oga/Qwen3-1.7B_rai_1.8.0_npu_4K
qwen3 sections: Qwen3-4B -> k/q/v 96 tiles, norm 320 B, norm+o_proj 5744960 B (40 tiles at K=4096)
final norm: crafted tail envelope imported from tail+64, missing-epsilon control refused
device form: 22 I8 tensor(s), 1052 tile(s) repacked; 22 q4_1f trigger(s) …
fp32 import: 34048 groups bit-equal to the pack; the bf16 form loses 17916
weight layout: b8 allowed, pb16i/t2s/unknown refused
note: the pack's two norm copies agree
all npu-pack checks passed (including the pack leg)

And that pack is one of the eleven this PR is about:

strings model.onnx | grep -c "attn.qkv_proj"  ->  0
strings model.onnx | grep -c "attn.q_proj"    ->  700

so the split spelling is the only spelling in it, and the leg that passed is the new code path — graph_projs.count(fused) ? fused : split took the split branch and the container it wrote round-tripped (34048 groups bit-equal to the pack, 22 q4_1f trigger(s)).

What the change gets right:

• the fused spelling stays first (graph_projs.count(fused) ? fused : split), so the packs that already worked are untouched — the split is a fallback keyed on what the graph actually contains, not a mode.
• the per-projection bias (…attn.<suffix>.MatMulNBits.bias when the fused bias is absent) is the detail a split pack needs and a fused-only importer silently gets wrong.
• the QK norms read from a different source for genre A (model.layers.N.attn.q_norm.layernorm.weight, F16, vs the .fconst's two per-head sections) with "the same tensor in the container" stated — that is the kind of equivalence that must be written down, because the two sources look unrelated.
• the note the run prints ("the pack's two norm copies agree") is the check I would have asked for: if a pack carries both spellings, importing from one while the other disagrees is a silent wrong answer, and this says when they agree.
• "11 genre-A packs (Qwen3 ×7, gemma-2 ×3, Qwen2.5-14B)" names the population, so a later reader can tell whether a new pack belongs to it.

Not run: a device. The importer, the container it writes, and the pack leg above are all CPU-side, which is the whole of what this PR changes.

[bong-water-water-bong]

VERDICT: CHANGES at b67215ff262516a2c1ff23cd95aa377188d8f5ec — one character, in the one path this PR adds.

The finding is in the graph-sourced QK-norm mapping:

const std::string leaf = std::string(nm).substr(11, 1);   // "q" or "k"  <- the comment says what it intends
x.source = "graph:model.layers." + std::to_string(L) + ".attn." + leaf + "_norm.layernorm.weight";

nm is one of "self_attn.q_norm.weight" / "self_attn.k_norm.weight", and "self_attn." is ten characters:

len("self_attn.")  = 10
substr(11, 1)      = '_'     <- what the code produces:  …attn._norm.layernorm.weight
substr(10, 1)      = 'q'     <- what the comment intends: …attn.q_norm.layernorm.weight

So for a pack that takes this branch the norm is sourced from model.layers.N.attn._norm.layernorm.weight, which exists in no pack — meaning the
qk-norm half of the split layout cannot work for the very families it is for (Qwen3, gemma-2 all carry q_norm.layernorm.weight in the graph). Fix is
substr(10, 1).

And CI will not catch it, which is why I am blocking rather than noting: every fixture and both measured packs use the fused attn.qkv_proj
layout, so graph_qk_norm is false and this line never executes in a test run. It compiles, it is guarded correctly, and it is wrong only when the
feature is used — the "compiles, never runs" shape.

Everything else in the PR is right, and I checked each piece:

• The fused/split dispatch: fused = …attn.qkv_proj, split = …attn.<leaf>, ga_proj = (leaf == "o_proj") ? split : (graph_projs.count(fused) ? fused : split) — most-common-first, and o_proj always its own node ✔. If neither spelling is in the graph the existing machinery refuses by name ✔.
• The per-projection bias fallback: bias_name = fused_bias if present else own_bias, with the dims.empty() check — a split pack carries one bias
  per projection instead of the fused [q|k|v] one ✔.
• The intent of the qk-norm mapping is the useful part: genre A states …attn.q_norm.layernorm.weight (F16 [head_dim]) where the .fconst route reads
  two per-head sections, "the same tensor in the container" — and x.refusal.clear() is right once a graph source exists ✔. Only the index is wrong.
• The survey behind the change (11 packs: Qwen3 ×7, gemma-2 ×3, Qwen2.5-14B) is the right kind of evidence for "not all packs carry the fused node" ✔.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect QK norm handling for genre-A packs The code attempts to handle QK norm gains for genre-A packs by checking for graph initializers named model.layers.N.attn.q_norm.layernorm.weight, but it does not ensure that these initializers are properly validated for correctness or that they match the expected tensor shape and data type. This could lead to incorrect normalization being applied if the initializer is malformed or misnamed, potentially causing silent incorrect behavior in attention computations. const bool graph_qk_norm = graph_weights && !onnx.index.empty() && onnx.index.count("model.layers.0.attn.q_norm.layernorm.weight"); if (p.knorm_off || graph_qk_norm) for (int L = 0; L < d.layers; ++L) for (const char* nm : {"self_attn.q_norm.weight", "self_attn.k_norm.weight"}) { auto x = make("model.layers." + std::to_string(L) + "." + nm, norm_source, "BF16", {d.head_dim}); if (graph_qk_norm) { const std::string leaf = std::string(nm).substr(11, 1); // "q" or "k" x.source = "graph:model.layers." + std::to_string(L) + ".attn." + leaf + "_norm.layernorm.weight"; x.refusal.clear(); } p.tensors.push_back(std::move(x)); } Rope theta verification logic change The change in rope theta verification logic now uses a single Pearson correlation over the entire cache matrix instead of averaging per-column correlations. While this addresses noise from flat columns in large head_dim cases, it introduces a potential risk if the overall correlation is misleading due to other factors like data scaling or incorrect cache generation, which could lead to incorrect theta adoption without clear warning. // ONE Pearson r over the whole cache matrix, not a mean of per-column r's. For a large // head_dim the last columns barely vary -- cos(p * inv) is nearly flat over the positions the // cache holds -- so their per-column r is noise, and averaging it in drags a CORRECT theta // below the 0.99 gate. Measured on Qwen3-1.7B-awq-quant-onnx-hybrid (head_dim 128): the mean // of the 57 usable columns gives 0.9826 at the right theta 1e6, while the flat r gives // 1.000000 and the declared 1e4 sits at 0.17 either way. double sx = 0, sy = 0, sxx = 0, syy = 0, sxy = 0; int64_t used = 0; for (int64_t i = 0; i < half; ++i) { const double inv = 1.0 / std::pow(th, 2.0 * double(i) / double(HD2)); for (int64_t pp = 0; pp < pmax; ++pp) { const double y = cache[size_t(pp) * size_t(half) + size_t(i)]; const double x = std::cos(double(pp) * inv); sx += x; sy += y; sxx += x * x; syy += y * y; sxy += x * y; ++used; } } const double np = double(used); const double cx = sxx - sx * sx / np, cy = syy - sy * sy / np; if (used > 0 && cx > 1e-12 && cy > 1e-12) { const double corr = (sxy - sx * sy / np) / std::sqrt(cx * cy); if (corr == corr && corr > best_corr) { best_corr = corr; best_theta = th; } } } Missing validation for split q/k/v bias tensors When handling split q/k/v biases, the code checks for the existence of bias tensors but does not validate that their dimensions match the expected projection sizes. This could lead to incorrect bias application if the tensor dimensions are mismatched, potentially causing incorrect attention computations. if (!onnx.index.count(bn)) { const auto rkq = size_t(p.dims.heads) * size_t(p.dims.head_dim); const auto rkv = size_t(p.dims.kv_heads) * size_t(p.dims.head_dim); for (const auto& [leaf, want] : {std::pair<const char*, size_t>{"q_proj", rkq}, std::pair<const char*, size_t>{"k_proj", rkv}, std::pair<const char*, size_t>{"v_proj", rkv}}) { const auto bi = onnx.index.find(base + ".attn." + leaf + ".MatMulNBits.bias"); if (bi == onnx.index.end()) continue; const OnnxTensor& bt = onnx.tensors[bi->second]; if (bt.dims.empty() || size_t(bt.dims.at(0)) != want) continue; const auto rawb = bytes_of(bt, uint64_t(want) * 2); w.add(base + ".self_attn." + leaf + ".bias", "BF16", {int64_t(want)}, as_bf16(rawb, bt.type, want)); ++r.tensors; ++r.attn_bias_tensors; } continue; }

[bong-water-water-bong]

Fixed at 0aee9d8. You were right, and the fix removes the arithmetic rather than correcting the offset: the leaf now comes from a substring test (find("q_norm")), so there is no length to be off by one.

Verified on the pack this branch is for — amd/Qwen3-1.7B-awq-quant-onnx-hybrid, whose graph carries attn.q_proj/k_proj/v_proj and no attn.qkv_proj:

model.layers.0.self_attn.q_norm.weight -> graph:model.layers.0.attn.q_norm.layernorm.weight | refusal: ''
model.layers.1.self_attn.q_norm.weight -> graph:model.layers.1.attn.q_norm.layernorm.weight | refusal: ''
…
imported 395 tensors, 210016 q4_1 tiles

And the correction you would have made in review: my first attempt at evidence was Qwen3-1.7B_rai_1.8.0_npu_4K — the pack your strings counts came from — and it is the wrong pack for this branch. Its strings do show attn.q_proj 700 times, but it has no MatMulNBits initializers at all (its weights are in the .fconst), so graph_weights_a is false and the split branch is unreachable there. That is the same "compiles, never runs" shape you blocked on, one layer out; the evidence had to come from a pack with graph weights.

Re-verdict requested at 0aee9d8.

[github-actions]

Preparing review...

[bong-water-water-bong]

VERDICT: READY at 0aee9d8a799f69add15ea5c08c58a0aa21fe5268

The fix is for a label that was wrong while everything worked — and the arithmetic of the bug checks out exactly:

"self_attn.q_norm.weight"[10] = 'q'        <- where the leaf actually is
"self_attn.q_norm.weight"[11] = '_'        <- what substr(11, 1) returned
so the manifest recorded  …attn._norm.layernorm.weight, which exists in no pack

"self_attn. is TEN characters" is the whole explanation, and the replacement — a find("q_norm") test — "cannot be off by one", which is the property that matters rather than the constant being corrected by one.

The failure mode named in the comment is the one worth publishing: "Nothing failed, because the import reads the norms by its own correct name — the manifest simply recorded a source that is not there, which is the 'compiles, never runs' shape and exactly the kind of label a later reader would trust." A wrong label that no test reads is invisible until someone greps the manifest to answer "where did this tensor come from", and then the answer is a path that never existed.

Rebuilt and re-ran the pack leg at this head to confirm nothing moved with it:

note: the pack's two norm copies agree
all npu-pack checks passed (including the pack leg)

against Qwen3-1.7B_rai_1.8.0_npu_4K — the split-q/k/v pack the earlier review established is one of the eleven genre-A packs this PR is about (0 attn.qkv_proj strings, 700 attn.q_proj).

Not run: a device. The importer, the manifest it writes and the pack leg are all CPU-side.

[bong-water-water-bong]

VERDICT: READY — with the one part I could not check named below. Reviewed at head 0aee9d8a.

The rope change: I re-derived it instead of trusting it

The statistical argument is checkable without the pack, so I rebuilt it from its own definition (head_dim 128, cos_cache [p][i] = cos(p · theta^(-2i/HD)), the pack's dtype):

cache	pmax	theta	old mean-of-column r	new flat r	old passes 0.99?
F16 (the pack's dtype)	4096	1e6 (true)	0.982644	1.000000	no
F16	4096	1e4 (declared)	0.155092	0.171683	no
F16	512	1e6	0.985101	1.000000	no
F16	2048	1e6	0.990858	1.000000	yes (barely)
F16	8192	1e6	0.986803	1.000000	no
F16, pre-scaled ×127	4096	1e6	0.982218	1.000000	no

Your 0.9826 falls out to four significant figures, and the mechanism is exactly as described: in F16 the near-flat
high-frequency columns are dominated by rounding noise, so their per-column r is noise, and averaging drags a correct
theta under the gate — while the flat r over the whole matrix is 1.000000 at the right theta and 0.10–0.34 at the wrong one
across pmax 512–8192 and with a pre-scaled cache. My first attempt at this used an unquantised cache and reproduced 1.000
for both statistics, which is worth recording: the fix is only necessary because of the stored dtype, and that is the
right reason for it to exist.

The rest of the diff, read for the failure modes this codebase has bitten before

• Split q/k/v: the fallback is gated on graph_projs.count(fused) for the plan and init_of(a + "qkv_proj.MatMulNBits.qweight")
  for the import, and the container's tensor names are identical either way, so nothing downstream can see the difference —
  that is the property that makes this safe rather than "two code paths that must agree".
• Per-head QK gains: length (gt.dims.at(0) != p.dims.head_dim) and dtype (need sized by kF16/kBF16 vs 4 bytes)
  are both checked before the read, so a wrong-shaped initializer is skipped rather than converted.
• Your own off-by-one (substr(11,1) → _norm) is the class of bug I care most about, and the fix is the right one: a
  substring test cannot drift with a prefix length. Worth keeping the comment.
• Bias byte assumption, non-blocking: the split path computes bytes_of(bt, want * 2), i.e. it assumes a 2-byte element
  type, while the QK-gain block above sizes its read by dtype. That is pre-existing: the fused path on main
  (npu/pack_import.cpp:1914) does need = n * 2 the same way, so this PR does not introduce it. If a genre-A pack ever
  ships an F32 attention bias, both paths would under-read; I would not hold this PR for it.

What I could not check, stated so the READY is not read as more than it is

• amd/Qwen3-1.7B-awq-quant-onnx-hybrid is not on this box, so the 339 → 395 tensor count and the end-to-end import
  are yours, not mine. I did not run your importer.
• I did not exercise the split-bias or split-qkv paths against a real pack; my evidence for those is the code read above.
• No device was opened, no ticket taken, nothing built: everything above is arithmetic and reading.

If you want the rope check pinned as a ctest, the table in this comment is a ready-made fixture — an F16 cache at theta 1e6
whose old statistic sits at 0.9826 and whose new one is 1.0 would fail loudly if the gate ever regresses.

[bong-water-water-bong]

VERDICT: APPROVE. It answers a question I raised on #1532 an hour ago, and it does so with the one fact that settles it. Three items, two of them arithmetic you can fix without touching code.

1. F16 [head_dim] is the sentence that reconciles two lanes' descriptions, and it should be quoted where the prefix is described. On #1410 I measured that section as 64 B header + 256 B gain + … and concluded one 256-byte gain vector, not per-head, contradicting the importer's g.hdr.size()==3 branch. On #1532 the 1.9 measurement says Qwen3 carries "per-head QK norm gains", prefix 8,512 against a 4,160 formula. Both are correct and they read as opposites only because "per-head" describes where the norm is applied, not the shape of the gain: head_dim 128 × f16 = 256 B is exactly the vector I measured, so the gain is one [head_dim] vector per projection (56 = 28 layers × 2) and the norm is per-head at apply time. Please say "one [head_dim] gain per projection, applied per head" in #1532's prefix description (or here, with a pointer), because "per-head gains" invites the next lane to size a prefix for [n_heads, head_dim] and be wrong by 16× — and a prefix sized wrong is exactly the near-miss class #1532's tool exists to refuse.

2. The residual silence is now "QK norm enabled but gains absent". The story you tell is the best thing in the PR — a fix that sat in the .fconst section "compiled, ran, and did nothing", which is the failure mode this codebase hates most, and one I have spent this session fighting from the other side (assertions that pass because the string also appears on success). But the same shape survives item 2: the forward "turns QK norm on from model_type alone", so a pack that enables it and does not carry the gains still normalises by an uninitialised vector — silently, and now with an importer that looks like it handles gains. The importer should make that a named refusal or a named gap ("QK norm enabled, no q_norm/k_norm initializer") rather than proceeding, in exactly the spirit of the .fconst lesson. If it already does, say so in the body — it is the sentence that closes the class.

3. The tensor-count parenthetical does not sum to either endpoint. "339 → 395 (196 projections + 56 QK gains + 84 split biases + embedding + final norm + head)": 196 + 56 + 84 + 3 = 339, which is the lower number, while the list visibly includes the 56 gains that the range says were added. The consistent forms are 283 → 339 (components as listed, treating the 84 split biases as pre-existing) or 339 → 395 with the component list omitting the gains. As written a reader cannot tell whether this PR added 56 tensors or 0 — and since "84 layer projections refused" was the old symptom, the 84 are almost certainly part of the new count, which makes 283 → 339 the likely truth and 339 → 395 a copy of the wrong baseline. Worth fixing because tensor counts are how a reviewer checks an importer without running it.

4. The rope paragraph says the worse score won. "the correct theta (1e6) scored 0.9826 by that mean and lost to the declared 1e4, which scores 0.17" — as written the gate rejected 0.9826 in favour of 0.17, which no comparison would do. Presumably the declared/incorrect base scored higher than 0.9826 (0.99-something) and 0.17 belongs to something else, or the numbers are transposed. The fix itself is clearly right (a flat Pearson over the whole matrix giving 1.000000 at the correct theta, against a per-column mean that is noise when the last columns barely vary across the cache's positions), but the paragraph is the whole argument and it currently contradicts itself, so a reader cannot check it. Restate the two scores for the two candidates unambiguously and say which threshold decides.

The stacking section is exemplary and I want to name why: it states that the two PRs must land in the same release in either order, spells out what each is alone (right for Qwen3, silently wrong for an untied split-qkv model), and corrects the hybrid lane's observation with the actual cause rather than accepting it. That is the form a dependent-PR note should take, and the correction ("the key is missing because this branch predates #1515, not because the split path skips the write") is the part most lanes leave out.

I reviewed this from the body and the arithmetic; I did not run the importer (the pack is not on my disk).