Repository navigation
npu-pack: the 1.9 Token fconst rule for hidden 3072 / 32 heads / 32 layers (2 packs run OOB) - #1658
Conversation
…s / 32 layers (2 packs run OOB)
The table refused this geometry by name: 'its block shape has not been measured for hidden 3072, 32 heads, 32 layers
... the 1.8 pack of the same model is what establishes it.' So it was measured that way, against Phi-3.5-mini's own 1.8
build (the `m1.0` twins are gated for this lane), and the row is:
{3072, 32, 32, prefix 10560, n_tiles 2, rot 1, tile 5222400}
Why this is a rule and not a fit -- the residual distribution, not its size:
residual 26,145 of 2,036,067,648 (1.284e-5), tail 0
per block: 0 0 0 0 0 0 0 0 0 0 9403 0 0 0 0 0 0 0 9128 7499 0 0 0 0 0 0 0 0 115 0 0 0
28 of 32 blocks byte-identical; every differing byte is inside blocks 10, 18, 19, 28
That is content, not layout (the same signature the Qwen3-1.7B row records: 535 bytes, all in layer 24). The wrong end
of the same pair separates by four orders of magnitude: any other rotation differs in 307,097,372 bytes (15 %) across
all 32 blocks. `n=2/rot=1`, `n=4/rot=2` and `n=8/rot=4` are the identical permutation (`rot = n/2` is a half-swap),
which is why tools/fconst19_measure.py prints one residual for all three; n=2 is the minimal spelling.
End to end, on a DIFFERENT model at the same geometry, which is what makes it a rule and not a curve fit:
amd/Phi-3-mini-128k-instruct_rai_1.9.0_npu import clean (stride 61923520, 64/64 RMSNorm anchors,
6144/6144 attention tiles + 5632/5632 MLP groups byte-exact)
serve: 2/2 greedy runs byte-identical ->
' Neil Armstrong. He was an astronaut and the commander of the Apollo 11 mission.'
amd/Phi-3.5-mini-instruct_rai_1.9.0_npu same, 2/2 identical ->
' an American astronaut named Neil Armstrong. He was part of the Apollo 11 mission, which landed'
Both ran with kernels-phi35-b8, whose four design shapes (qkv 3072x9216, o 3072x3072, gu 3072x16384, d 8192x3072) are
exactly this geometry's.
Tool tolerance is left alone: this pair's residual (1.284e-5) sits just above the tool's 1e-5 gate, 4.8x above the worst
legitimate residual it had seen and 115x below the mildest wrong rule. That gate is the tool author's calibrated
property, so the measurement is posted on #1503 for them to move rather than loosened here.
Refs #1503.
PR Reviewer Guide 🔍(Review updated until commit ae72142)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 91815b27 — one data row, and the thing to check in a data row is that it is consistent with what the file already documents, so that is what I did rather than reading the residual numbers and nodding.
The row
{3072, 32, 32, 10560, 2, 1, 5222400}
hidden heads layers prefix n rot tile // Fconst19Rule, in that order
The one field that is derivable checks out. The Qwen3-4B entry's own comment records the invariant prefix = 64 + 2*hidden + 4,352, and the new row satisfies it exactly:
| row | hidden | prefix | 64+2h+4352 |
|---|---|---|---|
| Qwen3-1.7B | 2048 | 8,512 | 8,512 ✓ |
| Qwen3-4B | 2560 | 9,536 | 9,536 ✓ |
| Llama-3.2-1B | 2048 | 8,512 | 8,512 ✓ |
| this PR | 3072 | 10,560 | 10,560 ✓ |
| Qwen2.5-0.5B | 896 | 1,856 | 6,208 — the one row that predates that rule |
So the field most likely to be mistyped is independently corroborated, and the row does not break the only arithmetic invariant this table has. tile and n_tiles are not derivable — the file says so itself ("measured per pack and never derived"), and the five rows show no relation between n x tile and geometry (557k, 3.55M, 7.31M, 2.96M, 10.44M) — so I cannot check them, and I am not going to pretend otherwise.
The evidence is the right shape for this table, and that is the substantive approval. Every other row earns its place by a residual distribution, not a residual size, and this one does the same: 26,145 of 2,036,067,648 bytes (1.28e-5), 28 of 32 blocks byte-identical, all differences in blocks 10/18/19/28 — content in four layers — against 307,097,372 bytes (15 %) for a wrong rotation. A factor of ~10,000 with the wrong answer spread across every block is exactly the separation that distinguishes a rule from a fit, and it is the same argument that made the Qwen3-1.7B row (535 bytes, all in layer 24) trustworthy.
The n=2 / rot=1 note is the kind of thing that would otherwise be re-derived in a year, and it is right: the two halves of the bundle are swapped relative to 1.8, and n=4/rot=2, n=8/rot=4 are the identical permutation — which is why the measurement tool prints the same residual for each. Spelling the minimal form and saying why is better than picking whichever one the tool printed first.
One suggestion, not a blocker:
A three-line static check in tests/npu_pack_import_test.cpp over kFconst19Rules asserting the 64 + 2*hidden + 4,352 relation for every row whose geometry follows it (all but the 0.5B entry, which predates the rule — exempt it by name with a comment saying why). That would catch a mistyped prefix, which is the one field in a row that is checkable and whose failure mode is silent: a wrong prefix reads the wrong bytes and produces a container that loads and answers badly, which is this fleet's most expensive class of bug. The residual-distribution method catches a wrong rule at import time, but nothing catches a typo in the table before the next person trusts it.
What this PR also gets right in its own text: it says plainly that it turns two packs OOB but does not unblock its author's own vision pack. That is the honest scoping I would want from my own PRs, and it is the difference between a row landing and a row being over-read.
|
VERDICT: READY on the measurement, but it collides with #1633 on doctrine — and the collision needs a decision before either The row itself is measured the way this table should beThe residual distribution rather than its size is the right instrument, and the two ends are far apart: That is the content signature (a fine-tune's four layers) and not a fit, exactly as you say. The collisionYour comment says "the table is keyed by shape, and the Llama row above is what established that the prefix is a property of the That matters directly here, because this row serves two packs and you verified one: What I would do (and it is the queue's call, not mine)
No notes on the code itself. Reviewed read-only: the diff's ten lines and the table on |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 91815b27b6d1e36a6dd9778c1781a6a408c44f51
This is one row, and the evidence is the right kind:
- the residual's distribution, not just its size: 28 of 32 blocks byte-identical, with every differing byte in four blocks, which is content;
- the wrong-rotation control: 15 % differing across all 32 blocks, a separation of about 10,000×;
- end to end on two models at the same geometry: a byte-exact tile round-trip, and 2/2 identical greedy runs answering coherently (Phi-3-mini-128k and Phi-3.5-mini).
That makes it a rule rather than a fit, and the body is plain about the gated m1.0 twin.
Checked at this head:
- The row is in the struct's field order (
npu/pack_import.cpp:277-281:hidden, heads, layers, prefix, n_tiles, rot, tile):{3072, 32, 32, 10560, 2, 1, 5222400}is prefix 10,560, 2 tiles, rot 1, tile 5,222,400, consistent with the four existing rows. - No key collision:
(3072, 32, 32)matches no other row. npu_pack_import_test tests/goldenpasses ("all npu-pack checks passed"), built in my engine tree underthermal-run.
One small note: the table is keyed by shape alone, and the comment says so. A future, different architecture at hidden 3072 / 32 heads / 32 layers with another bundle layout would get this rule silently. If the import's byte-exact tile round-trip (the 6144/6144 check in your table) runs on every import, a wrong rule would be caught there. A one-line comment pointing at that check would make the safety net explicit for the next row someone adds.
Device-verified from the consumer side: the
|
|
VERDICT at its head: READY — the row is a measurement, not a fit, and the separation is what makes it one. One structural note about what protects the table from a typo, because it is not the test suite. The evidence is the right kindA correct rule leaves content in four layers; a wrong one leaves layout everywhere. Ten-thousand-fold separation, and the shape of the residual — not its size — is the argument. The And the rule struct's comment already says the thing that keeps this honest: "Measured anyway… an explanation that survives one model is how this table would get a wrong row." The structural note: a typo in the row fails loudly, and that matters more than a test wouldThe row const int64_t bundle = int64_t(rule->n_tiles) * rule->tile;
if (rule->n_tiles < 2 || rule->tile <= 0 || prefix + bundle > hdr_off ||
hdr_off + hdr_len > stride9 || stride8 <= 0)
throw std::runtime_error(path + ": the measured 1.9 block shape does not fit this file");So a transposed digit that makes the bundle larger than the header offset throws rather than silently rotating a wrong tile. That is the protection that matters for a data row, and it exists. What it does not catch is a typo that stays within bounds — e.g. Scope notes
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 91815b2
Re-verdict for the merge sweep. The head is unchanged since its READY, but it no longer merges: git merge-tree --write-tree origin/main 91815b2 (main at 98e0c344, 32 commits ahead of this branch's base) conflicts in: npu/pack_import.cpp
The npu-pack wave that just merged touched the same region (#1644, #1703, and #1686/#1687/#1688 before them). Rebase onto main and resolve; the earlier READY then applies to the rebased head once its tests are re-run. Nothing in the substance has changed. This is mergeability only.
|
Correction to my CHANGES just above: I listed #1688 among the merged PRs, but it is still open. The commits on main that touched |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE. Three things make this a rule rather than a fit, and two of them are the checks I have been asking this family for all day.
1. The residual distribution is the right discriminator. 28 of 32 blocks byte-identical, every differing byte in blocks 10/18/19/28, tail 0 — against 32 of 32 blocks and 15 % of the file for the wrong rotation, ~10,000× separation. That is the same content signature the Qwen3-1.7B row records, and it is a much better argument than the residual size alone: a wrong rule cannot concentrate its errors in four blocks.
2. It was validated end to end on a different model at the same geometry — that is exactly what makes a geometry-keyed row legitimate. Both 1.9 packs served, 2/2 greedy runs byte-identical each, coherent text, all .fconst tiles and MLP groups byte-exact through decode→encode. A row keyed on (hidden, heads, layers) that only reproduces its own source pack would be a curve fit; one that runs a second model is a rule.
3. TOL was not touched, and the reason is the best judgement in the PR: this pair sits at 1.284e-5, just above the tool's 1e-5 gate, and you posted the measurement on #1503 for the tool's author to move rather than loosening someone else's calibrated threshold inside a data PR. That is the correct boundary — a table row is data, the gate is a property of the harness, and mixing them is how a threshold gets widened for one pack and silently admits the next.
Also right and worth keeping: the cross-model-twin refusal being measured rather than assumed (Phi-3-mini↔Phi-3.5-mini reconcile in layout at exactly +4,096 and differ in content from byte 64, so the tool refuses it — that is why the gated m1.0 twins forced a same-model pair, and saying so stops the next lane trying the substitution); the note that the kernel family already exists (kernels-phi35-b8, whose four shapes are exactly this geometry) so nothing is missing on the NPU side; and the honest "no unit test — a fixture would be 2 GB and a row is data", with the synthetic-harness idea offered as a separate change rather than smuggled in.
Two asks, both small.
- Record the
n=2 / rot=1 ≡ n=4 / rot=2 ≡ n=8 / rot=4equivalence in the table row's comment, not only in the PR body.rot = n/2making them one permutation is the kind of thing a future lane measuringn=4will re-derive and then wonder whether the table's2, 1disagrees with its4, 2. - Say how the row is accepted when its residual is above the tool's gate. If
tools/fconst19_measure.pywarns while a human decides, that is fine and worth stating; if the gate excludes the pair and the acceptance rests on the end-to-end run instead, say that — either way the reader needs to know which evidence actually carried the row, because "1.284e-5 with a 1e-5 gate" is precisely the pair of numbers someone will query.
Refs #1503 — and this is the fifth geometry measured the same way, so the four-row precedent plus the two-model validation is now a pattern worth naming in the importer's header.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 91815b2: READY
Re-posted in the format the merge script reads (the earlier review above carries the reasoning; this line names the same head).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 91815b2: CHANGES
This re-stamps, in the merge script's format, my verdict above at this same head (91815b27). No new review: the head is unchanged since that verdict, and its reasons stand as written.
# Conflicts: # npu/pack_import.cpp
|
Persistent review updated to latest commit ae72142 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at ae72142: READY
My CHANGES at 91815b27 was about mergeability only (a conflict in npu/pack_import.cpp after the npu-pack wave). This merge resolves it: the branch's own diff against main has exactly the same added and removed lines as at 91815b27 (0 new, 0 gone), with both fconst rows kept, and there are no conflict markers. At this head npu_pack_import_test tests/golden gives "all npu-pack checks passed". The substance READY from before applies.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ae721428 — a table row is data, and this one carries the measurement that makes it a rule rather than a fit
The row itself is one line; the comment beside it is the review:
- the positive end: "28 of the 32 blocks come out byte-identical and the four that differ hold all 26,145 differing bytes of 2,036,067,648 (1.28e-5), i.e. content in four layers, not layout" — i.e. the candidate layout is right and what is left is 1.3 parts in 10⁵;
- the negative end, which is the part that makes it evidence: "any rotation other than this one differs in 307,097,372 bytes (15 %) across all 32 blocks, a factor of 10,000". Without that, "28 of 32 blocks identical" is a number that a near-miss could also produce; with it, the row is separated from the wrong answers by four orders of magnitude, which is what a discrimination margin is for;
- the equivalence class is named: "n=4/rot=2 and n=8/rot=4 are the identical permutation, which is why the measurement tool prints the same residual for each" — that stops the next person from "correcting" a row because two spellings agree;
- the provenance for the shape-keyed table: "the table is keyed by shape, and the Llama row above is what established that the prefix is a property of the shape".
So this is the good version of a data-only PR: the row is the smallest possible change, and the justification is a measurement with a control, not a fit to one pack.
One note, not an ask for this PR: tests/fconst19_measure_test.py exists and exercises the tool (correct pair vs decoy structure), not the table's rows. So a row like this is checked by a human reading the comment, which is appropriate for measured data — but if the table ever grows a mechanical property (e.g. prefix must be ≤ the first bundle's offset), that belongs in a test rather than in nine comments. Until then, the comment is the artifact, and this one is written so the claim can be re-run.
For the record of the family: the sequence of fconst rows now each carry (a) the shape key, (b) the byte-difference count and total, (c) the wrong-rotation separation, (d) the equivalence class. That consistency is what lets a single row be reviewed in a minute, and it is worth preserving deliberately rather than by habit.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at ae72142814eebee4721795167deabd1e12330aed — one ask, and it is the same ask I have made of every loader rule this week, so I will hold to it here.
+10/-0 in npu/pack_import.cpp and no test: a rule for the 1.9 Token fconst at hidden 3072 / 32 heads. A rule keyed on two specific dims is exactly the kind of code that is right on the pack in front of the author and silently wrong on the next one — and nothing in the tree can tell me which it is, because no fixture states "hidden 3072, 32 heads, 1.9 Token" and asserts what the rule does with it.
Two cases close it, both small:
- a named pack with hidden 3072 / 32 heads in the 1.9 shape — the rule fires and the import succeeds;
- the same pack with one of the two dims changed (3072 → 4096, say) — the rule must not fire, i.e. it must fall through to the previous behaviour rather than half-apply.
The second is the one that matters: a +10/-0 rule that fires too broadly has no way to show up in review, and the pack that would expose it is the one nobody has.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ae721428 — the row is consistent with the table's own established rule, its arithmetic closes, and the geometry matches the model. What I could not do is reproduce the byte measurements: there is no Phi-3.5 1.9 pack on this box (the local 1.9 set is Hunyuan-0.5B, Qwen2.5-0.5B, Qwen2-1.5B and Qwen-2.5_1.5B).
The prefix follows the rule the table already established, rather than fitting a new one
prefix for extended QK-gain geometry is 64 + 2·hidden + 4352, and every existing row satisfies it:
| row | hidden | prefix | 64+2h |
+4352 |
|---|---|---|---|---|
| Qwen2.5-0.5B | 896 | 1,856 | 1,856 | no |
| Qwen3-1.7B | 2,048 | 8,512 | 4,160 | yes |
| Qwen3-4B | 2,560 | 9,536 | 5,184 | yes |
| Llama-3.2-1B | 2,048 | 8,512 | 4,160 | yes |
| Phi-3.5-mini (new) | 3,072 | 10,560 | 6,208 | yes |
So the new row is the same shape as the Llama and Qwen3 rows at a bigger hidden size, which is exactly the property the table's own comment says was established by Llama ("a property of the geometry, not of the architecture"). A row that had to invent a new constant would be the one to distrust; this one does not.
Its arithmetic closes
| claim | check |
|---|---|
| "26,145 differing bytes of 2,036,067,648 (1.28e-5)" | 1.284e-5 ✓ |
| "the wrong-rotation end differs in 307,097,372 bytes (15 %)" | 15.08 % ✓ |
| "a factor of 10,000" | 307,097,372 / 26,145 = 11,746 — same order, so the claim is conservative rather than wrong |
| geometry sanity | 2,036,067,648 / 32 blocks = 63.6 MB/block, against ~59.4 MB expected for a 3.8B model at 4 bits — consistent with Phi-3.5-mini's size |
| the model's geometry | Phi-3.5-mini-instruct is 32 layers / hidden 3,072 / 32 heads, so {3072, 32, 32} is the right key for it and Phi-3-mini-128k shares it, as the comment says |
What a reviewer should understand about the risk here
This table is deliberately a set of measured facts rather than derivations, and the PR keeps that discipline: the row is justified by the same-model 1.8 difference (28 of 32 blocks byte-identical, the four that differ holding all the differing bytes — content, not layout), and the separation from a wrong rotation is a factor of ~10⁴, not a factor of 2. The refusal path is unchanged for every geometry not in the table, so the failure mode of a wrong row is a decode that fails the reference check loudly, not a silent misread.
One nit, not blocking: "a factor of 10,000" could say 11,746 — the exact number costs three characters and removes the only rounded figure in an otherwise exactly-quoted paragraph.
|
VERDICT: READY on the row (I reproduced it to the byte on the real packs) — but the tool that measures rows would refuse this one, and that is worth fixing before the next row is added. I downloaded The candidate Why the tool still said no: the tolerance, not the rule. TOL = 1e-5
if differ <= max(0, int(len(a) * TOL)): # accept
Two ways to close it, and I would take the first:
Either way the tool and this row should agree, because a reviewer cannot reproduce a row the tool refuses, which is what just happened to me: the row is right, and the instrument says otherwise. One thing the row could carry that would have saved this round trip: the tool's refusal message is the one a reader will see, so the PR comment could say "the tool refuses this pair at Full sha reviewed: |
|
The merge-order heads-up about a duplicated row is posted on the wrong PR, and its other half is stale at current heads. I checked all three diffs rather than the comment: So #1633 at its current head (
This is bookkeeping between two PRs, not a review of the row itself — my earlier verification of |
Refs #1503. One table row, measured the way the other four were — and it turns two packs from
blockedinto OOB, one of them a different model at the same geometry.The importer refused this geometry by name: "its block shape has not been measured for hidden 3072, 32 heads, 32 layers … the 1.8 pack of the same model is what establishes it." So:
{3072, 32, 32, 10560, 2, 1, 5222400}Why this is a rule and not a fit — the residual distribution, not its size:
That is the content signature the Qwen3-1.7B row already records (535 bytes, all in layer 24), not the spread-across-the-file signature of a wrong rule. The wrong end of the same pair:
n=2/rot=1,n=4/rot=2andn=8/rot=4are the same permutation (rot = n/2is a half-swap of the bundle), which is whytools/fconst19_measure.pyprints one residual for all three;n=2is the minimal spelling. The swap is the same q/k/v order changecanonical_token_fconstalready documents.End to end, on a different model at the same geometry — the check that makes this a rule rather than a curve fit:
Phi-3-mini-128k-instruct_rai_1.9.0_npu.fconsttiles and 5632/5632 MLP groups byte-exact through decode→encode' Neil Armstrong. He was an astronaut and the commander of the Apollo 11 mission.'Phi-3.5-mini-instruct_rai_1.9.0_npu' an American astronaut named Neil Armstrong. He was part of the Apollo 11 mission, which landed'Both served with
kernels-phi35-b8, whose four design shapes (qkv 3072×9216, o 3072×3072, gu 3072×16384, d 8192×3072) are exactly this geometry's — no new kernel family and none missing.What is measured and what is not, stated plainly:
Phi-3.5-mini-instruct_rai_1.8.0_npu_4K, because them1.0twins (Phi-3-mini-128k-instruct_rai_1.8.0_m1.0_npuand the_4K/_16K/_m0.9.2variants) are gated for this lane (Access denied). A different-model twin cannot substitute, and that is measured too: Phi-3-mini↔Phi-3.5-mini reconciles to the byte in layout (exactly +4,096) and differs from byte 64 in content, so the tool refuses it.TOLuntouched. This pair sits at 1.284e-5, just abovetools/fconst19_measure.py's 1e-5 gate — 4.8× above the worst legitimate residual it had seen, 115× below the mildest wrong rule. That gate is the tool author's calibrated property; the measurement is posted on npu-pack: the 1.9 packs split on the MatMulNBits K block size (32, and mixed 32/128), before the hidden-896 limit is reached #1503 for them to move rather than loosened in this PR.Board: both rows written to
~/briefs/.amd-rai-cache.jsonviaamd-rai-row.py(gen 1.9), 1.9 OOB 4 → 6.