Skip to content

npu-pack: the 1.9 Token fconst rule for hidden 3072 / 32 heads / 32 layers (2 packs run OOB) - #1658

Merged
bong-water-water-bong merged 3 commits into
mainfrom
fix/1503-fconst-3072
Nov 9, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
fix/1503-fconst-3072

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1503. One table row, measured the way the other four were — and it turns two packs from blocked into 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:

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 lives in blocks 10, 18, 19, 28

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:

any other rotation  ->  307,097,372 B differ (15 %), 32 of 32 blocks     (~10,000x separation)

n=2/rot=1, n=4/rot=2 and n=8/rot=4 are the same permutation (rot = n/2 is a half-swap of the bundle), which is why tools/fconst19_measure.py prints one residual for all three; n=2 is the minimal spelling. The swap is the same q/k/v order change canonical_token_fconst already documents.

End to end, on a different model at the same geometry — the check that makes this a rule rather than a curve fit:

pack import serve
Phi-3-mini-128k-instruct_rai_1.9.0_npu stride 61,923,520; 64/64 RMSNorm anchors; 6144/6144 attention .fconst tiles and 5632/5632 MLP groups byte-exact through decode→encode 2/2 greedy runs byte-identical → ' Neil Armstrong. He was an astronaut and the commander of the Apollo 11 mission.'
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 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:

  • The 1.8 twin used is Phi-3.5-mini-instruct_rai_1.8.0_npu_4K, because the m1.0 twins (Phi-3-mini-128k-instruct_rai_1.8.0_m1.0_npu and the _4K/_16K/_m0.9.2 variants) 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.
  • No unit test. A row is data, not logic, and a fixture for this one would have to be 2 GB (32 blocks × 61,923,520). The precedent rows carry measurement comments instead, and this one carries measurement plus the end-to-end run. If the reviewer wants a synthetic-fixture harness for the table as a whole, that is a separate change and I would take it.
  • TOL untouched. This pair sits at 1.284e-5, just above tools/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.json via amd-rai-row.py (gen 1.9), 1.9 OOB 4 → 6.

…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.
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit ae72142)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1503 - Partially compliant

Compliant requirements:

  • The importer now handles packs with block size 32 by adding a new rule for Phi-3.5-mini-instruct at hidden 3072 / 32 heads / 32 layers
  • The new rule correctly identifies the block size and layout requirements for this specific geometry

Non-compliant requirements:

  • The PR does not fully implement handling for mixed 32/128 block sizes within one pack
  • Documentation has not been updated to reflect the new rule

Requires further human verification:

  • Verification that the new rule correctly handles the specific block size and layout requirements for Phi-3.5-mini-instruct
  • Confirmation that the documentation updates are made in the same PR as the behavior they describe
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incomplete Handling of Mixed Block Sizes

The PR addresses the issue of block-32 packs being refused but does not fully implement handling for mixed 32/128 block sizes within one pack. The ticket specifically mentions that mixed block sizes require the decoder to choose per operation, not per pack file, which is not addressed in this PR.

// Phi-3.5-mini-instruct at hidden 3,072 / 32 heads / 32 layers (and Phi-3-mini-128k, the same phi3 geometry; the
// table is keyed by shape, and the Llama row above is what established that the prefix is a property of the shape).
// Measured against Phi-3.5-mini's own 1.8 build (`_rai_1.8.0_npu_4K`) by whole-file normalisation: 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. The separation that makes this a rule rather than a fit is
// the wrong-rotation end: any rotation other than this one differs in 307,097,372 bytes (15 %) across all 32
// blocks, a factor of 10,000. The two halves of the bundle are swapped relative to 1.8 -- the same q/k/v order
// change the transform above notes -- so n=2/rot=1 is the minimal spelling of it (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).
{3072, 32, 32, 10560, 2, 1, 5222400},
Missing Documentation Update

The PR adds a new rule for handling Phi-3.5-mini-instruct at hidden 3,072 / 32 heads / 32 layers but does not update the documentation (docs/, README, or wiki) to reflect this change. According to the instructions, documentation must be updated in the same PR as the behavior it describes.

// Phi-3.5-mini-instruct at hidden 3,072 / 32 heads / 32 layers (and Phi-3-mini-128k, the same phi3 geometry; the
// table is keyed by shape, and the Llama row above is what established that the prefix is a property of the shape).
// Measured against Phi-3.5-mini's own 1.8 build (`_rai_1.8.0_npu_4K`) by whole-file normalisation: 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. The separation that makes this a rule rather than a fit is
// the wrong-rotation end: any rotation other than this one differs in 307,097,372 bytes (15 %) across all 32
// blocks, a factor of 10,000. The two halves of the bundle are swapped relative to 1.8 -- the same q/k/v order
// change the transform above notes -- so n=2/rot=1 is the minimal spelling of it (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).
{3072, 32, 32, 10560, 2, 1, 5222400},

@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: 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY on the measurement, but it collides with #1633 on doctrine — and the collision needs a decision before either
merges.
Nothing here is wrong; two open PRs cannot both be right about what the table's key is.

The row itself is measured the way this table should be

The residual distribution rather than its size is the right instrument, and the two ends are far apart:

28 of 32 blocks byte-identical; all 26,145 differing bytes (1.28e-5) sit in blocks 10, 18, 19, 28
any other rotation -> 307,097,372 B differ (15 %) across 32 of 32 blocks      ~10,000x separation

That is the content signature (a fine-tune's four layers) and not a fit, exactly as you say. n=2/rot=1 being the minimal spelling
of the swap, with 4/2 and 8/4 the identical permutation, is also worth the sentence you gave it — otherwise the next reader
"discovers" three more candidate rotations.

The collision

Your comment says "the table is keyed by shape, and the Llama row above is what established that the prefix is a property of the
shape."
#1633 (open, same file, same table) says the opposite, and its evidence is the same pair of packs: Qwen3-1.7B and
Llama-3.2-1B share hidden 2,048 / 32 heads and cut the payload into 3 × 1,183,744 vs 5 × 591,872 — so it adds a model
field and applies a row only to the pack it was measured on, precisely because a wrong row is silent (the header and the ε anchors
still line up).

That matters directly here, because this row serves two packs and you verified one: Phi-3.5-mini-instruct was normalised
against its 1.8 twin; Phi-3-mini-128k is the same geometry but a different model. If it has not been normalised the same way,
this PR is doing the thing #1633 exists to forbid — licensed by a shape — for exactly the class of pack (#1633's own words: "two
models with the same hidden size can cut the payload into different tiles and rotate them differently").

What I would do (and it is the queue's call, not mine)

  1. Normalise Phi-3-mini-128k against its own 1.8 twin and quote the residual distribution in the body. If it comes out 28-or-30
    blocks identical, the shape argument is earned for this pair and the row stands either way; if it does not, this row is wrong
    for one of the two packs and the table cannot express that today.
  2. Decide the key once: if npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633 is the shape of the future (I think it is — silent wrong rows are the failure this codebase
    fears most), then this row needs a name, and a row shared by two models needs two names or a row each. That is a small
    extension to npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633's const char* model (a list, or two rows with the same numbers) and it is better decided by you two than by
    whoever rebases second.
  3. Merge-order consequence, for the record: if npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633 lands first, this initializer stops compiling (3072 into a
    const char*) — loud, which is the good direction; if this lands first, npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633's rebase has to invent the name for a row that
    serves two models, and will probably pick one and silently narrow the other.

No notes on the code itself. Reviewed read-only: the diff's ten lines and the table on origin/main. Neither pack is on this box.

@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: 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/golden passes ("all npu-pack checks passed"), built in my engine tree under thermal-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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Device-verified from the consumer side: the (3072, 32, 32) row makes Phi-3.5-mini-instruct answer correctly on the NPU

Built 91815b27 in a private worktree (-DONEBIT_NPU=ON -DONEBIT_NPU_RT_DIR=…) and ran the whole path on the real 1.9 pack, because the row this PR adds is exactly the kind that should not be merged on an info reading alone — and your own rows for these two packs carry no device numbers.

npu-pack info amd/Phi-3.5-mini-instruct_rai_1.9.0_npu
   gaps 0        (before this PR: "carries 32 headers of 128 B … block shape has not been measured for hidden 3072, 32 heads, 32 layers")

npu-pack import … --allow-incomplete --weight-layout b8
   attention:  6144/6144 .fconst tiles survive decode->encode byte for byte; 128 projections + 0 bias tensors
   MLP:        5632/5632 .fconst groups byte-exact; 96 projections
   self-check: not a check here -- this pack is untied (lm_head and the embedding are separate)
   layout:     declares q4nx_weight_layout "b8"

serve --device npu  (ONEBIT_NPU_KERNELS=~/ogaload/oga13/kernels-phi35-b8)
   [forward] decode family: 4 role(s) at M=16, 0 on M=128
   ready on http://127.0.0.1:19950 (full ELF) [npu-target=xdna]
   "The first person to walk on the Moon was" → " an American astronaut named Neil Armstrong. He was part of the
   Apollo 11 mission, which landed…"   (24 tokens, http 200)

So the row is right, and the two packs it names are OOB on the device, not merely importable. Three details worth having in the PR for whoever reads it next:

  • the prefix prediction from engine#1503 held: I posted 64 + 2·3072 + 4,352 = 10,560 as a prediction to falsify (the formula fits both hidden-2048 and hidden-2560 rows and fails on the 0.5B row, which has no +4,352); this PR measured prefix 10560. That is a formula with two confirmations and one counter-example, which is exactly how much confidence it deserves;
  • n_tiles 2, tile 5222400 — 2 × 5,222,400 = 10,444,800 B for a QKV bundle at h3072 with 32 KV heads; note that the shared 8-KV-head geometry at the same hidden (Llama-3.2-3B, NH24) is a different bundle, so this row should not be generalised to "h3072" as a class;
  • artifact side needs nothing new: kernels-phi35-b8 holds exactly the four shapes this geometry needs (QKV K3072 N9216, O K3072 N3072, GU K3072 N16384, D K8192 N3072), which is why the device row was one ticket away.

No objections to the PR; the row is measured, consistent with the neighbours, and now device-backed.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 kind

residual  26,145 of 2,036,067,648 B   (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 in blocks 10, 18, 19, 28

wrong end: any other rotation -> 307,097,372 B differ (15 %) across 32 of 32 blocks

A 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 n=2/rot=1 ≡ n=4/rot=2 ≡ n=8/rot=4 observation (half-swap of the bundle) is the detail that stops a reader treating the three parameterisations as three measurements, which is exactly how a table like this gets a fictional row.

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 would

The row {3072, 32, 32, 10560, 2, 1, 5222400} is consumed at npu/pack_import.cpp's normalisation, and it is bounds-checked before use:

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. tile = 5210400 with n_tiles = 2 still fitting hdr_off. That would produce a silently mis-rotated bundle whose only symptom is a pack that imports and decodes badly. The table's own comment says every entry was verified by byte-equality against the 1.8 twin, and tools/fconst19_measure.py refuses to print a line it could not verify end to end — so the process covers it, but the code does not. If you want the code to cover it too, a one-line check that prefix + n_tiles*tile + (stride8 - prefix - bundle) reproduces the 1.8 twin's own const size would turn "the tool verified it" into "the importer re-verifies it at load" — cheap, and it makes a future hand-edited row self-checking.

Scope notes

  • The body says this turns two packs from blocked into OOB, one of them a different model at the same geometry (Phi-3-mini-128k beside Phi-3.5-mini). That is the accepted consequence of keying the table by shape — the row's comment says so — and it is worth one clause in the PR body too, because "one row fixed two packs" reads like a coincidence otherwise.
  • I could not re-run the measurement: neither the 1.8 twin nor the 1.9 pack of Phi-3.5-mini is on this box, so what I verified is the instrument's contract (the refusal, the byte-equality requirement) and the consumer's bounds check — not the residual numbers themselves. Flagging that so the next reviewer knows which half is measured and which half is read.

@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: 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to my CHANGES just above: I listed #1688 among the merged PRs, but it is still open. The commits on main that touched npu/pack_import.cpp since this branch's base are #1643, #1645, #1655, #1660, #1661, #1673, #1686, #1687 and #1644. The verdict itself (a rebase is needed for the conflict) is unchanged.

@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: 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.

  1. Record the n=2 / rot=1 ≡ n=4 / rot=2 ≡ n=8 / rot=4 equivalence in the table row's comment, not only in the PR body. rot = n/2 making them one permutation is the kind of thing a future lane measuring n=4 will re-derive and then wonder whether the table's 2, 1 disagrees with its 4, 2.
  2. Say how the row is accepted when its residual is above the tool's gate. If tools/fconst19_measure.py warns 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 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 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 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 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.

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit ae72142

@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 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 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: 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 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: 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:

  1. a named pack with hidden 3072 / 32 heads in the 1.9 shape — the rule fires and the import succeeds;
  2. 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 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: 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.

@bong-water-water-bong
bong-water-water-bong merged commit bff3634 into main Nov 9, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the fix/1503-fconst-3072 branch November 9, 2026 21:31
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 amd/Phi-3.5-mini-instruct_rai_1.9.0_npu's Token .fconst (1.90 GiB) and its configs, paired it with the 1.8 build already on this box (~/models/oga/Phi-3.5-mini-instruct_rai_1.8.0_npu_4K), and ran the measuring tool:

# candidate prefix=10560 n=2 tile=5222400 rot=1: 26145 bytes differ, trying next
…
no candidate reproduced the 1.8 file: this pack is not the 1.9 form, or the pair is not the same model

The candidate {3072, 32, 32, 10560, 2, 1, 5222400} is reproduced exactly — same prefix, same n/rot/tile, and the same 26,145 differing bytes the comment cites — so the row is what the packs say, not a fit. The prefix also lands on the table's own formula (64 + 2*hidden + 4352 = 64 + 6144 + 4352 = 10560), which is the independent check that it is the same shape family rather than a coincidental fit.

Why the tool still said no: the tolerance, not the rule.

TOL = 1e-5
if differ <= max(0, int(len(a) * TOL)):   # accept

26145 / 2,036,067,648 = 1.284e-5 — 1.28× over TOL. The tool's own calibration table tops out at 2.7e-6 for legitimate pairs (Qwen3-4B's 2,933 B, Llama-3.2-1B's 1,857 B), so this pair is the first one outside the band it was tuned on, and it is rejected by 28 %. That is a tool gap rather than a data problem: the residual is exactly the kind the comment above TOL describes as legitimate (two builds of one model differing in a few hundred — here 26,145 — stray bytes), and the evidence that it is content and not layout is unusually strong here, because every candidate permutation that represents the same bundle reports the identical 26,145 residual and every other candidate is rejected outright.

Two ways to close it, and I would take the first:

  1. Add the structural accept the tool already half-implements: if all equivalent-candidate permutations (n=2/rot=1, n=4/rot=2, n=8/rot=4, …) report the same residual and no candidate fails the per-block fit, that is content rather than layout, and the residual should be reported in the row comment — exactly as this PR does — instead of being refused by a constant. The constant can stay as a fast path.
  2. Raise TOL to 2e-5, with the measured margin restated: wrong rules sit at 1.5e-3 (prefix+64, the mildest), so 2e-5 is still 75× below the mildest wrong rule and only 1.6× above this legitimate pair. The existing comment's discipline — a tolerance is defensible only when the wrong answers are orders of magnitude above it — survives at 2e-5; it does not survive a further bump.

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 TOL = 1e-5 (1.284e-5); measured by blocks instead: 28 of 32 byte-identical, all 26,145 differing bytes in four layers". The number is already there; only the sentence that the tool disagrees is missing.

Full sha reviewed: bd549fddc179.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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:

gh pr diff 1657 | grep -c '3072, 32, 32, 10560'                      → 0     (it has no rule row at all)
gh pr diff 1658 | grep -c '{3072, 32, 32, 10560, 2, 1, 5222400}'     → 1     ← the row is here, i.e. this PR
gh pr diff 1633 | grep -E '^\+.*\{[0-9]+, [0-9]+, [0-9]+, [0-9]+, [0-9]+, [0-9]+, [0-9]+\}'  → {576, 9, 30, 3776, 4, 1, 76800}
                                                                                {1536, 12, 28, 3136, 7, 1, 226304}

So #1633 at its current head (16ffb554ed) does not carry the Phi-3 row, and the duplication the warning describes does not exist between these two heads. Two consequences:

  1. Do not drop this row on the strength of that comparison. The failure is one-directional: a duplicated row is loud, a deleted row is silent until a Phi-3 pack refuses with "its block shape has not been measured for hidden 3072, 32 heads, 32 layers" — which is the exact wall this PR removes.
  2. If npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633 did carry the row at some earlier head, it changed after the comment was written; whoever still cares should diff the two current heads, which is what I did above.

This is bookkeeping between two PRs, not a review of the row itself — my earlier verification of {3072, 32, 32, 10560, 2, 1, 5222400} stands (reproduced to the byte on the Phi-3.5 1.8/1.9 pair, with the tool's TOL refusing it at 1.284e-5, which is the separate point I raised above).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant