Repository navigation
npu-pack: the 1.9 header fills a layout the vote could not find, it does not rewrite one it found (closes #1831) - #1837
bong-water-water-bong wants to merge 1 commit into
Conversation
…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
left a comment
There was a problem hiding this comment.
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.
|
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 ( On folding the extraction in — my own answer is no. I wrote What is worth keeping from this PR, and it costs nothing: the fifth test arm, "the old rewrite really did move #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 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 — |
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.9pack with a header it overwrote the in-block header list with the 2-header shape:
locate_fconst()has already established how many in-block headers a pack has —period2 on Qwen2/Llama (q/k/v/o share one payload) and 4on Qwen3 (three in-block headers, the middle two being the per-head QK norms) — and
post_attn_hdr()ishdr.empty() ? h1 : hdr.back(). Replacing athree-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 fieldslocate_fconstwould 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):h1, a one-entryhdrandperiod 2from the residue, as before;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 frommain)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.cppcompiles (onlythe pre-existing
-Wvexing-parse), the arms pass in a harness linked against the object, and the test file compiles against the new header; thenpu_pack_import_testtarget'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.cppalready assertslocate_fconstreportsperiod == 4andpost_attn_hdr() == h4for it, and this PR is what letsthat survive the header pass.
Closes #1831.