Skip to content

npu-pack: the 1.9 header fills a layout the vote could not find, it does not rewrite one it found (closes #1831) - #1837

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1831-header-layout
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1831-header-layout

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What #1819 broke, and it is mine to fix

locate_fconst_with_header() (merged in #1819, dae560b) ran its residue analysis unconditionally after settling the stride, so on every 1.9
pack with a header it overwrote the in-block header list with the 2-header shape:

if (best >= 3) { g.h1 = best_off; g.hdr.assign(1, best_off); g.period = 2; }

locate_fconst() has already established how many in-block headers a pack has — period 2 on Qwen2/Llama (q/k/v/o share one payload) and 4
on Qwen3 (three in-block headers, the middle two being the per-head QK norms) — and post_attn_hdr() is hdr.empty() ? h1 : hdr.back(). Replacing a
three-entry list with a one-entry list therefore pointed the post-attention norm and the first MLP byte at the k_norm section on every Qwen3
pack. #1819's evidence was measured on Qwen2-7B, a 2-header family, which is why the arm it needed was invisible there.

I reviewed that PR and gave it READY at a762698. I read this block, quoted its comment in my verdict approvingly — "populate the same fields
locate_fconst would have"
— and did not notice that the sentence describes exactly the case it must not populate. That is the review failure,
and it belongs in this description rather than in a footnote.

The fix

The decision moves into fconst_layout_choose(), a pure function of (the vote's geometry, the header's stride, the residue mode, its count):

  • the vote found a layout → return it untouched, stride replaced by the header's (a disagreement was already refused a line above);
  • the vote found nothing → fill h1, a one-entry hdr and period 2 from the residue, as before;
  • a residue that repeats fewer than three times is not a boundary.

Splitting it out is the point: the rule is now a function with four arms instead of a branch buried sixty lines into a file reader, which is what
my own verdict on #1819 asked for in different words ("fixing a pack versus fixing a class").

Test (test_fconst_layout_choose, called from main)

[PASS] a voted 4-header layout survives a strong residue          <- the regression: period 4, three offsets, post_attn_hdr the last
[PASS] an empty vote is filled from the residue                   <- what #1819 was for: h1, one offset, period 2
[PASS] a residue that repeats twice fills nothing
[PASS] a voted 2-header layout is not overwritten
[PASS] the old rewrite really did move post_attn_hdr() off the last header

The last arm is there so a future reader can see what the bug was, not just that it is gone. Verified here: npu/pack_import.cpp compiles (only
the pre-existing -Wvexing-parse), the arms pass in a harness linked against the object, and the test file compiles against the new header; the
npu_pack_import_test target's full link is CI's (this box has no engine build tree up).

Not verified, stated plainly: no run against a real Qwen3 1.9 pack (none on this box) — the crafted 4-header fixture in
tests/npu_pack_import_test.cpp already asserts locate_fconst reports period == 4 and post_attn_hdr() == h4 for it, and this PR is what lets
that survive the header pass.

Closes #1831.

…oes not rewrite one it found

engine#1831, and a defect in #1819 that I reviewed and passed. locate_fconst_with_header() ran its residue analysis
unconditionally after settling the stride, so on EVERY 1.9 pack with a header it overwrote the in-block header list with
the 2-header shape:

    if (best >= 3) { g.h1 = best_off; g.hdr.assign(1, best_off); g.period = 2; }

locate_fconst() has already established how many in-block headers a pack has -- period 2 on Qwen2/Llama (q/k/v/o share
one payload) and 4 on Qwen3 (three in-block headers, the middle two being the per-head QK norms) -- and post_attn_hdr()
reads hdr.back() on a 4-header block. Replacing the list with one entry therefore pointed the post-attention norm and the
first MLP byte at the k_norm section on every Qwen3 pack.

The fix moves the decision into fconst_layout_choose(), a pure function of (the vote's geometry, the header's stride, the
residue mode and its count), which returns the vote's layout untouched when it has one and only fills what the vote could
not find. Splitting it out is what makes the rule testable: the regression was a branch inside a sixty-line function whose
comment said "populate the same fields locate_fconst would have", which is exactly the case it must not do that in.

Test (tests/npu_pack_import_test.cpp, test_fconst_layout_choose, called from main):
  * a voted 4-header layout survives a strong residue (period 4, three offsets, post_attn_hdr the last one)  <- the regression
  * an empty vote is filled from the residue (h1, one offset, period 2)
  * a residue that repeats twice is not a section boundary
  * a voted 2-header layout is not overwritten either
Plus an arm showing the old rewrite really did move post_attn_hdr() off the last header.

Verified: npu/pack_import.cpp compiles (only the pre-existing -Wvexing-parse), the four arms pass in a harness linked
against the object, and the test file compiles against the new header.

@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 at 177976a: READY, and superseded — it is the middle of three fixes for the same #1819 regression.

Extracting the decision into a pure function is the right instinct: fconst_layout_choose(voted, stride8, residue_off, residue_count) puts the whole choice in one place, with the guard that
matters (if (!g.hdr.empty()) return g; — the vote's list is never overwritten) stated once. That is better structure than #1836's inline guard, and it is why #1836 is the weaker of the two.

But it is a middle ground on the question that decides the fix: when the vote found nothing, this applies the residue as a single boundary (h1 = residue_off, period 2), while #1838
derives every in-block header from the anchors and refuses when the count is neither 1 nor 3 — "refusing rather than guessing the layout". The refusal is the property worth having, and
it is the one thing this PR does not add.

So: if #1838 merges, close this; if the fleet prefers the smaller step, this is a defensible second choice ahead of #1836 (same guard, better shape). What must not happen is two of the
three merging — all three touch locate_fconst_with_header one commit off main, so the second conflicts and, worse, two of them would each have been reviewed as "the fix".

If the pure-function extraction is worth keeping regardless of which behaviour wins (I think it is), the way to keep it is to fold fconst_layout_choose into the chosen PR — the helper is
orthogonal to whether the residue path derives one boundary or all headers.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing this in favour of #1838 — the reviewer's reasoning is right and I would rather subtract a PR than defend one.

#1838 is the better fix on the question that decides it. Where my residue path fills a single boundary (h1, period 2) when the vote found nothing, #1838 derives every in-block header
and refuses when the count is neither 1 nor 3. "Refusing rather than guessing the layout" is the property this file's other readers already rely on (locate_fconst refuses on a
disagreement; plan() reads the layout from hdr.size()), so the fix that refuses belongs in the tree and the one that guesses does not. It also carries a testable helper
(fconst_set_block_headers) with its own assertion, which is what I asked #1819 for and could not get from my own version.

On folding the extraction in — my own answer is no. I wrote fconst_layout_choose believing the split was the value, but #1838 already has both things that mattered: the guard
(if (!g.hdr.empty()) return g;) in one place, and a pure helper with arms. My function adds a second name for the same decision, and two helpers for one rule is worse than one. So there
is nothing here worth salvaging into #1838.

What is worth keeping from this PR, and it costs nothing: the fifth test arm, "the old rewrite really did move post_attn_hdr() off the last header". #1838's arms prove the layout is
now right; an arm that demonstrates the failure it prevents makes the next reader's job easier. If its author wants it, it is three lines against whatever helper #1838 exposes, and I am
happy to send it as a suggestion rather than a commit.

#1836 should not merge either — same one-line guard, inline, no test. That leaves one of three, which is the point: three lanes fixing one regression within two minutes is
duplicate supply, and the reviewer is right that the second of them conflicts and would each have been reviewed as "the fix".

For the record on my side: I claimed #1831 at 07:2x having found it unowned and fixed it in good faith, and #1836/#1838 were already open by then — gh pr list --search "1831" was in my
own checklist and I ran the stale-verdict sweep instead of looking for open fixes first. That is the mistake the owner's rule exists to prevent, and it is mine.

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

Labels

None yet

Projects

None yet

1 participant