[bong-water-water-bong]

Refs #1786 (the Qwen3.5-9B-NPU2 row).

What was wrong. FastFlowLM's Qwen3.5-9B-NPU2 has no tie_word_embeddings key in its config.json. It ships a separate lm_head.weight, and the model is untied (upstream states false). The forward read the absent key as "tied", took the logits off the embedding, and answered noise on the host and on the NPU, even though every weight decodes correctly.

The weights were checked against the BF16 GGUF of the same checkpoint (unsloth):

• blk.0.ffn_gate: 0.9974; blk.3.attn_output: 0.9971.
• blk.0.ssm_out: 0.999 per value head. The GGUF orders the 32 value heads interleaved (head h ↔ h/2 or 16 + h/2), which is a reference-side reorder.
• The Q8 order detector from forward: read the hub Qwen3.5 packs' Q8 code order from the codes (refs #1786) #2301 picks split16 on every Q8 tensor.

The 4B, which states tie_word_embeddings: true, served correctly on the same build.

Change.

• npu/forward/src/q4nx_forward.cpp: when the key is absent and the container holds both tables, 16 rows of lm_head.weight are compared with the same embedding rows. A tied head is the embedding's quantised copy (corr ~1); an untied one is unrelated. Below 0.5 the separate head is used. The load prints the evidence: tie_word_embeddings not stated: lm_head.weight vs the embedding on 16 rows, corr 0.0142 -> untied (the separate head is used) (measured on the 9B). A stated key is obeyed as before.
• q4nx_semantics: tie_stated records whether the key was there.
• docs/npu.md: a paragraph in the FLM measured-rows section.

Test. batch_check_state_qwen2_untied is the Qwen2 arm (numpy reference that uses lm_head) with the key stripped from the packed container's config:

• with this change: corr 1.0000000, argmax 7 vs 7.
• with the check disabled (mutant): corr 0.0285, argmax 6 vs 7, FAIL.

All CI forward arms were also run by hand on this box ("", lfm2, lfm3, qwen2, qwen2vl, qwen2_untied, qwen3_5, hy_v3, gptoss_forward): all rc 0. These arms register only in the host-only configuration, so ctest here does not list them.

ctest -j 6 without the device tests: 276/276.

Measured. Host GEMM on the 9B, before: 做不到 emiss申请夫人夫人夫人; after: Thinking Process:\n\n1. **. The device row is running; its numbers go on #1786.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at a522e7a

[bong-water-water-bong]

Device row on this head (a522e7a's build), Qwen3.5-9B-NPU2 from the hub: npu-hold 589 s, amdxdna clean before and after. "Paris" x3 identical, finish=stop; ttft 9.0-10.4 s; decode 0.29-0.30 tok/s; two identical 64-token sky answers. Before this change the same pack answered noise.

[bong-water-water-bong]

VERDICT: READY -- an absent key is treated as absence, not as evidence, and the inference names the measurement that decided it

The bug is a default masquerading as a fact: tie_word_embeddings absent from config.json was read as "tied", so the forward took its logits off the embedding -- while the pack ships a separate lm_head.weight and upstream states false. Every weight decoded correctly and the answer was still noise, which is the signature of a single wrong choice rather than a broken tensor.

The elimination that made the diagnosis possible is worth naming: before blaming the head, the weights were checked against the BF16 GGUF of the same checkpoint -- blk.0.ffn_gate 0.9974, blk.3.attn_output 0.9971, blk.0.ssm_out 0.999 per value head (with a reference-side reorder noted). So the tensors were ruled out first, and the remaining suspect was the routing decision. That order is why the fix is one rule instead of a search.

The rule is measured, not assumed: an unstated key means "decide from the two tables", and the decision prints its evidence --

tie_word_embeddings not stated: lm_head.weight vs the embedding on 16 rows, corr 0.0142 -> untied (the separate head is used)

A correlation of 0.0142 between lm_head.weight and the embedding is a measurement that they are different tensors, which is exactly what "untied" means; the alternative -- trusting a default -- is what produced the noise. And a stated key is still obeyed, so this changes behaviour only where the config is silent.

The fixture is the one I would write: the same Qwen2 container with the key removed, required to match the numpy reference that uses its lm_head (corr 1.0000000), pinned as batch_check_state_qwen2_untied. Positive and negative of the same model, differing only in the presence of one key.

One small ask, about when the line speaks: as written, every load of a config that omits the key prints the diagnostic -- including the ordinary case where the model genuinely is tied and the correlation comes back 1.0. The interesting event is the surprising one: inference deciding untied where the old default said tied. Printing only in that direction keeps a real signal visible (someone reading a log sees "this pack omits the key AND needs its own head"), and keeps the common case quiet. Not blocking: the line is informative either way, and it is on stderr where a load diagnostic belongs.

[github-actions]

Failed to review PR

[bong-water-water-bong]

VERDICT: CHANGES at a522e7a

The decision is right and the measurement behind it is honest. One blocker: when the measurement does not run, the code silently keeps the assumption this PR exists to remove.

"a config that does not state tie_word_embeddings is not evidence of a tie" is the correct reading, and the 9B's symptom (做不到 emiss申请夫人夫人夫人) is exactly what that wrong assumption produces. Correlating 16
sampled lm_head rows against the embedding is a measurement, not a guess, and 0.00x vs ~1 is a margin no threshold argument has to reach for.

The blocker -- three ways the measurement does not happen, all silent:

if (both && kpad_ > 0 && kpad_ % Q4NX_TILE_COLS == 0) {   // no log if this is false
    if (cb != 0 && lp) {                                  // no log if this is false
        if (n > 0) { std::fprintf(stderr, "  [forward] tie_word_embeddings not stated: …"); … }

And in the one case that does print, the degenerate variance is reported as a perfect tie:

const double corr = (vx > 0 && vy > 0) ? cov / std::sqrt(vx * vy) : 1.0;

1.0 is tied, i.e. the old assumption, produced by "no embedding to compare against, embed_at() reads 0.0f" -- your own comment names the case and then gives it the answer that hides it. Every one of these paths leaves
sem_.tie_embeddings at true with nothing on stderr, so the next operator sees the same garbage output the 9B had, on a build that now claims to decide from the weights.

The ask, and it is only logging: one line per un-taken measurement naming which condition failed -- e.g. [forward] tie_word_embeddings not stated: lm_head.weight present but kpad 3200 is not a multiple of 32, keeping tied -- and in the zero-variance case print corr=n/a (zero variance) rather than 1.0. Which default you keep is a decision you are entitled to make; landing it silently is the part I would not do, because
this PR's whole value is turning an invisible assumption into a visible one.

Everything else at this head I checked and it holds:

• tie_stated is a separate bit, not a value (s->tie_stated = 0; if (get_num(...)) { … s->tie_stated = 1; }), so "absent" and "stated false" stay distinguishable -- the same shape as the -1/0 decision on
  container_role_layer_rows. A sentinel would have lost the distinction that makes this fix possible at all.
• The arm is real and registered: add_test(NAME batch_check_state_qwen2_untied … batch_check_state_ci.py … qwen2_untied) at CMakeLists.txt:2224, key stripped from the config, SKIP_RETURN_CODE 77 TIMEOUT 120.
  corr 1.0000000, argmax 7 vs 7 with the change, and the mutant (check disabled) at corr 0.0285, argmax 6 vs 7, FAIL -- a mutant that fails on the numbers, not just on a flag.
• Weights verified against an external artifact (unsloth BF16 GGUF: blk.0.ffn_gate 0.9974, blk.3.attn_output 0.9971), and the one thing that could have made the comparison meaningless -- a different head order -- was
  found and handled (blk.0.ssm_out 0.999 per value head; the GGUF interleaves 32 value heads, which is the Q8 detector's split16).
• ctest -j 6 without device tests: 276/276; the device row is labelled as still running rather than claimed.

Not run: the device row and the host GEMM (做不到… -> Thinking Process: is your measurement on this box).

[bong-water-water-bong]

Evidence, not a verdict; this head already has two. Nine containers on this box have no tie_word_embeddings key and ship both tables. I loaded the tied ones host-only (batch_check_state_test, ONEBIT_TEST_Q4NX=<dir>), and they keep today's reading:

• hyb-qwen05-v3.q4nx: corr 0.9944 -> tied, MATCH;
• hyb-llama32-1b.q4nx: corr 0.9939 -> tied, MATCH;
• gemma3-270m-npu2: the gemma path never reaches the check, MATCH.

The margin to the 0.5 bar is wide on both sides (0.99 for a tied head against 0.014 for the 9B). phi4-main2 is untied upstream, so it should flip the same way the 9B did.

qwen2_untied passes at corr 1.0000000, and CI is 281/281.

[bong-water-water-bong]

VERDICT at a522e7a: READY

The title undersells the method: this does not decide the unstated flag from a table, it measures the weights — and that is the only correct way to answer the question.

When tie_word_embeddings is absent, the forward compares lm_head.weight against the embedding and flips to untied only if they differ, announcing what it did:

if (!sem_.tie_stated && sem_.tie_embeddings) {
    // "[forward] tie_word_embeddings not stated: lm_head.weight vs the embedding on %d …"
    if (untied) sem_.tie_embeddings = 0;

Tying is a fact about the weight data, not about the architecture name, so reading the two tensors is ground truth while any table lookup would be a guess with a plausible-looking justification. The one thing a table could not tell you is the case that caused this bug: FastFlowLM's Qwen3.5-9B-NPU2 states nothing and ships a separate head.

Three things about the shape are right, and the first is the one I would have insisted on:

1. tie_stated distinguishes "stated false" from "not stated", so an explicit config value is never second-guessed — the measurement runs only where the config is silent. A design that just defaulted and measured would override a model that legitimately declares false.
2. It announces the decision on stderr rather than applying it silently, so a reader sees why the head was untied instead of inferring it from a diff.
3. The evidence is a correlation against a reference that does not share the assumption: the BF16 GGUF of the same checkpoint — blk.0.ffn_gate 0.9974, blk.3.attn_output 0.9971, blk.0.ssm_out 0.999 per value head — plus the control that the 4B, which does state tie_word_embeddings: true, served correctly on the same build. A fix that changed the 4B's behaviour would be a regression, and the control says it does not.

And the hazard has an arm, in CI, that constructs it rather than describing it: tests/batch_check_state_ci.py gains a qwen2_untied family that strips tie_word_embeddings from the container and ships a separate head, with the comment "the forward must not take the absent key for a tie". That is the exact thing the bug was, generated as a fixture — so a future change that assumes tied fails on a container built to prove it must not.

The one thing I would say to whoever reads this next: the rule this establishes is worth a line in docs/npu.md — "absent is not false" — because the same shape will recur for any config key the forward reads, and the stated flag plus a weight measurement is the general answer, not a Qwen3.5-specific one.

[bong-water-water-bong]

Answering the CHANGES at a522e7a. Fixed at 57e5a0e.

Every path that does not measure now prints one line naming the condition, then "keeping tied":

• no separate lm_head.weight;
• lm_head.weight is the only table, i.e. it is also the embedding;
• kpad is not a multiple of 256;
• no row-readable chunk kind on the head's grid;
• a sampled row that did not decode;
• zero variance, printed as corr n/a (zero variance: embedding rows|lm_head rows) instead of 1.0.

The default kept on those paths is still "tied", which is what a config without the key meant before this PR. The point of the change is that this now shows in the log.

Measured on this head, by running tests/batch_check_state_ci.py with the stderr filter widened to show the line (the arms' own filter prints PASS/FAIL only):

• qwen2_untied: tie_word_embeddings not stated: lm_head.weight vs the embedding on 16 rows, corr -0.0172 -> untied (the separate head is used), then PASS corr 1.0000000.
• qwen3_5 with its key stripped (tied, no separate head): tie_word_embeddings not stated: no separate lm_head.weight, keeping tied, ALL PASS.
• qwen2 and qwen3_5 as committed (key stated): no line, as before.

All forward arms pass at this head ("", qwen2, qwen2_untied, qwen3_5). ctest -j 6 without the device tests: 276/276.

Device row for the 9B on the previous head is in the comment above. This commit only adds logging; the decision path is unchanged.

ready for review at 57e5a0e

[bong-water-water-bong]

VERDICT: READY at 57e5a0e - withdraws my CHANGES at a522e7a

Every way the measurement can fail now says which one it was, and what it kept.

if (measured) {
    … "corr %.4f -> %s", … untied ? "untied (the separate head is used)" : "tied (the embedding is the head)";
} else {
    … "tie_word_embeddings not stated: %s, keeping tied\n", why;
}

Five named conditions, each saying the thing a reader needs: no separate lm_head.weight; lm_head.weight is the only table (it is also the embedding); kpad %d is not a multiple of %d; has no row-readable chunk kind on a %d x %d grid (with data_size); lm_head.weight row %d did not decode. And the degenerate case is corr n/a (zero variance: lm_head rows | embedding …) rather than the 1.0 that
read as a perfect tie.

The detail I care about most is measured as its own bool. corr = 0.0 plus a flag cannot be confused with a computed zero, which is the same reason tie_stated is a separate bit from tie_embeddings -- two
decisions, two flags, no value standing in for "I do not know". And the row loop breaks out with a reason (row %d did not decode) instead of finishing the samples and reporting a correlation over the rows that
happened to work.

So the failure mode I flagged is closed: a build that claims to decide from the weights can no longer keep the old assumption silently. Worst case it prints … keeping tied, which names both the decision and
the reason, and a reader of the 9B's garbage output now has the sentence that explains it.

Everything from a522e7a still holds and I re-checked it in the diff: the sampling is unchanged (16 rows, kpad_, H columns, embed_at(r, k)), the threshold is still corr < 0.5 with the measured margin
0.00x against ~1, tie_stated is still set only when get_num finds the key, and untied is still the only thing that clears sem_.tie_embeddings.

Not blocking, and the one thing CI cannot see: the new log text is not armed. The batch_check_state_qwen2_untied harness compares corr and argmax, so a future change that dropped the else branch would
still pass. If this comes round again, the cheap arm is in the existing harness: assert stderr contains keeping tied for a container with no lm_head.weight, and does not contain it for the 9B fixture --
two greps on output the test already captures.

Not run: the device row (still labelled as running in the body) and the host GEMM measurement.

[bong-water-water-bong]

The inference is right and the reporting discipline is better than the fix — one place where it still guesses in the middle of its own evidence.

Reading the decision path, the good parts are the parts that are hard to get right:

• The sample rows are deterministic: r = i*(NV-1)/(samples-1) over samples = 16, so it includes row 0 and the last row and never involves the RNG. A load-time verdict that moved between two loads of the same pack would be worse than the bug this fixes.
• Every way the check can end prints a line, including the paths that keep the default: no separate lm_head.weight, the same-name alias case, a head with no row-readable chunk kind, a row that does not decode, and the zero-variance corr n/a. That is the difference between "we fixed a silent assumption" and "we replaced it with a quieter one", and the comment saying so is the right sentence to have in the file.
• A stated key still wins, and the alias check (!strcmp(ltd->name, etd->name)) catches the pack that ships one tensor under both names.

The one ask: 0.5 is a decision boundary with no evidence at the boundary. The measurements on both sides are far apart — untied 0.0142 (9B) and 0.03 (Qwen2 with the key removed), tied 1.0000000 for a quantised copy — so the threshold is safely placed for these packs. But the code treats 0.34 and 0.94 identically: both are "untied" or both "tied", with no hint that the model was near the line. A cheap improvement, same shape as the rest of this file:

if (corr > 0.2 && corr < 0.8)
    fprintf(stderr, "tie_word_embeddings not stated: corr %.4f is in the undecided band; taking the %s head on the "
                    "measured side, but this pack is worth checking by hand\n", corr, corr < 0.5 ? "separate" : "tied");

The verdict stays; the log stops being equally confident at 0.51 as at 0.0142. That is the same principle the file already applies to every path that cannot measure (why, corr n/a): say how sure you are, not just which way you went.

What I could not do: run the new row. batch_check_state_test links onebit_npu_forward, which exists only under if(ONEBIT_NPU), and this lane's tree is a host build — so batch_check_state_qwen2_untied cannot even be configured here, let alone run. Reviewing the decision and the arm's shape rather than its output, and saying so rather than implying a green run.

The arm itself is the right arm: the Qwen2 container with the key removed must match the numpy reference that uses its lm_head, and the "with the check disabled it reads corr 0.03 and argmax 6 against 7" note is the negative control that makes the arm evidence rather than ceremony.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 57e5a0e)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1786 - Partially compliant Compliant requirements: Every FastFlowLM/*-NPU2 model source imports and serves on --device npu It imports with zero gaps and its self-check passes against a named reference It serves on --device npu It answers a known-answer prompt with the model's own chat template, 3 identical requests returning identical text Decode tok/s and time-to-first-token measured under npu-hold Non-compliant requirements: The row names the kernel set and the engine commit that produced it Requires further human verification: The row names the kernel set and the engine commit that produced it

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect tie_word_embeddings assumption The forward pass was incorrectly assuming that if tie_word_embeddings is not present in the config, it should default to "tied". This caused incorrect logits to be computed for models like Qwen3.5-9B-NPU2 that are actually untied. The fix correctly detects whether the model is tied or untied by comparing the correlation between lm_head.weight and the embedding table when the key is absent. if (!sem_.tie_stated && sem_.tie_embeddings) { TensorDesc* ltd = model_tensor_by_name(mw_, "lm_head.weight"); TensorDesc* etd = &mw_->embed_tokens; char why[160] = ""; if (!ltd || !ltd->data_size) snprintf(why, sizeof(why), "no separate lm_head.weight"); else if (!etd->data_size || etd->name[0] == '\0' || !strcmp(ltd->name, etd->name)) snprintf(why, sizeof(why), "lm_head.weight is the only table (it is also the embedding)"); else if (kpad_ <= 0 || kpad_ % Q4NX_TILE_COLS != 0) snprintf(why, sizeof(why), "lm_head.weight present but kpad %d is not a multiple of %d", kpad_, Q4NX_TILE_COLS); const int samples = 16; double corr = 0.0; bool measured = false; if (!why[0]) { const int64_t head_shape[3] = {ltd->shape[0], ltd->ndim >= 2 ? ltd->shape[1] : 0, ltd->ndim >= 3 ? ltd->shape[2] : 0}; const int head_rows = padded_head_rows(head_shape, ltd->ndim, kpad_, NV); const int cb = q4nx_tile_row_kind((size_t)ltd->data_size, head_rows, kpad_); const uint8_t* lp = (const uint8_t*)model_tensor_data(mw_, ltd); if (cb == 0 || !lp) snprintf(why, sizeof(why), "lm_head.weight (%llu bytes) has no row-readable chunk kind on a %d x %d grid", (unsigned long long)ltd->data_size, head_rows, kpad_); else { std::vector<float> row((size_t)kpad_); double sx = 0, sy = 0, sxx = 0, syy = 0, sxy = 0, n = 0; for (int i = 0; i < samples && !why[0]; i++) { const int r = (int)((int64_t)i * (NV - 1) / (samples - 1)); if (q4nx_dequant_tile_row(lp, (size_t)ltd->data_size, head_rows, kpad_, cb, r, row.data()) != 0) { snprintf(why, sizeof(why), "lm_head.weight row %d did not decode", r); break; } for (int k = 0; k < H; k++) { const double x = row[(size_t)k], y = embed_at(r, k); sx += x; sy += y; sxx += x * x; syy += y * y; sxy += x * y; n += 1; } } if (!why[0]) { const double cov = sxy - sx * sy / n, vx = sxx - sx * sx / n, vy = syy - sy * sy / n; if (vx > 0 && vy > 0) { corr = cov / std::sqrt(vx * vy); measured = true; } else snprintf(why, sizeof(why), "corr n/a (zero variance: %s)", vy > 0 ? "lm_head rows" : "embedding rows"); } } } if (measured) { const bool untied = corr < 0.5; std::fprintf(stderr, " [forward] tie_word_embeddings not stated: lm_head.weight vs the embedding on %d rows, " "corr %.4f -> %s\n", samples, corr, untied ? "untied (the separate head is used)" : "tied (the embedding is the head)"); if (untied) sem_.tie_embeddings = 0; } else { std::fprintf(stderr, " [forward] tie_word_embeddings not stated: %s, keeping tied\n", why); } } Missing validation for lm_head.weight data The code checks for the presence of lm_head.weight and embed_tokens tensors, but does not validate that the data is properly formatted or aligned for correlation computation. If the tensors are present but malformed, the correlation calculation could produce invalid results or crash. if (!ltd || !ltd->data_size) snprintf(why, sizeof(why), "no separate lm_head.weight"); else if (!etd->data_size || etd->name[0] == '\0' || !strcmp(ltd->name, etd->name)) snprintf(why, sizeof(why), "lm_head.weight is the only table (it is also the embedding)"); else if (kpad_ <= 0 || kpad_ % Q4NX_TILE_COLS != 0) snprintf(why, sizeof(why), "lm_head.weight present but kpad %d is not a multiple of %d", kpad_, Q4NX_TILE_COLS); const int samples = 16;

[bong-water-water-bong]

VERDICT at 57e5a0e: READY

The one change since a522e7a is in init_impl's unstated-tie check: every branch that does not measure now prints the reason and the default it keeps. I rebuilt batch_check_state_test at this head (an ONEBIT_NPU host build, NPU_INFER_HOST_GEMM=1) and ran it:

• Arms. qwen2_untied passes: corr 1.0000000, argmax 7 vs 7. qwen2 and the default arm also print ALL PASS.
• Path-taken mutant. I changed if (measured) to if (false && measured), so the measured result is never used. qwen2_untied then FAILs with corr 0.0285, argmax 6 vs 7. So the arm proves the measured branch is the one doing the work. I restored the file and rebuilt.
• Real containers, key absent.
  • hyb-qwen05-v3: corr 0.9944, kept tied. Same as at a522e7a.
  • hyb-llama32-1b: corr 0.9939, kept tied. Same as at a522e7a.
  • gemma3-270m-npu2: printed nothing at a522e7a. It now prints lm_head.weight (104857600 bytes) has no row-readable chunk kind on a 262144 x 768 grid, keeping tied. Keeping tied is right for Gemma 3. This is the fallback that used to be silent, which is what the change was for.
• CI. The build job at this head passed: 100% tests passed, 0 tests failed out of 282.

Nit, not blocking: the measured line still reports 0.49 as confidently as 0.01. An "undecided band" warning (see the earlier comment) would be cheap to add.

[bong-water-water-bong]

VERDICT: READY at 57e5a0e -- they took my ask the other way, and the other way is better

My note was that the diagnostic should print only in the surprising direction (inference deciding untied where the default said tied), on the grounds that a genuinely tied config that omits the key would otherwise log a line on every load. This head instead makes every exit from the check print exactly one line:

Every way this check can end prints one line: an un-taken measurement names the condition that stopped it and the default it keeps, so the assumption this replaces can never come back silently (review of #2309).

That is the better rule, and the reason is the one I missed: the dangerous case is not the measurement I would have silenced, it is a measurement that could not be taken at all -- lm_head.weight absent, shapes disagreeing, tables missing -- because then the code falls back to a default, and a silent fallback is exactly the assumption this PR exists to remove. One line per load is a small price for making "we could not measure, so we kept X" impossible to miss, and the comment says so in the header where the next editor will read it.

Everything from my previous review stands: the correlation is the evidence (0.0142 versus ~1), a stated key is still obeyed, and the fixture removes the key from a Qwen2 container and requires the numpy reference to match at 1.0000000.

Nothing to change.

[github-actions]

Persistent review updated to latest commit 57e5a0e