Skip to content

npu-pack: locate the 1.9 head in the .fconst tail instead of requiring it to end the file (engine#1740 W1a) - #1773

Merged
bong-water-water-bong merged 6 commits into
mainfrom
1bit/fconst19-tail-locate
Nov 10, 2026
Merged

bong-water-water-bong merged 6 commits into
mainfrom
1bit/fconst19-tail-locate

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

The tail is [envelope][rows x tile][padding] and the importer required have == env + rows*tile exactly. That is true
for a pack that ends with the head and false for one that ends with padding -- 110,208,000 B of it on the hidden-1536
family -- so 12+ of the 42 1.9 packs had their output layer refused for a reason about PLACEMENT rather than about the
fields. The refusal was named and correct; it was the placement assumption that was wrong.

The placement is now measured:

  • the trailing all-zero run at EOF is counted, whatever its alignment;
  • plan_tail_head() (exposed in the header) turns (bytes, tail_off, envelope, rows, tile, zeros) into the earliest and
    latest tile-aligned candidate, or a refusal carrying both lengths. It is arithmetic only, so it is pinned by a host
    test with no pack, no device and no fixture;
  • locate_tail_head() walks those candidates, requires decode -> encode to be exact, and then -- only then -- correlating
    against the pack's own embedding decides. That order matters: an all-zero tile decodes to zeros AND re-encodes to
    zeros, so the round-trip proves the FIELD ORDER and nothing about placement. Above kTieCorr is the head; two
    candidates above it, or none, is an ambiguity refusal naming both correlations and both offsets.

Tied and untied are reported rather than assumed. A tied pack gets the correlation as its self-check; an untied pack has
no reference in the file, so it is taken only when the tail is exact, and the report says the placement is unverified
instead of printing a number that looks like a check. tie_word_embeddings is still read from the pack, never inferred.

Gate 1, Llama-3.1-8B_rai_1.9.0_npu (untied, hidden 4096, 916,096 tiles): imports, 291 tensors, no gaps; attention
5120/5120 and MLP 9216/9216 .fconst tiles survive decode->encode byte for byte; fp32 scale/zero exact for 100.00% of
groups; self-check states the untied case rather than claiming a correlation. Regression, Qwen2.5-0.5B-Instruct 1.9
(tied, hidden 896): unchanged -- 291 tensors / 67,088 tiles, corr 0.999999, rel_err 0.000700, row-shifted control 0.0716.

Block-32 heads (the 1.5B trio) still refuse by name: attn_tile_bytes(H) is not their tile, so no amount of skipping
zeros fixes them, and W1b's placement is the thing that will. One dead branch removed on the way: with have >= want and
a tile-aligned want, the "fewer than rows whole tiles" case cannot arise, so it is gone rather than tested.

bong added 2 commits November 9, 2026 22:14
…g it to end the file (engine#1740 W1a)

The tail is [envelope][rows x tile][padding] and the importer required `have == env + rows*tile` exactly. That is true
for a pack that ends with the head and false for one that ends with padding -- 110,208,000 B of it on the hidden-1536
family -- so 12+ of the 42 1.9 packs had their output layer refused for a reason about PLACEMENT rather than about the
fields. The refusal was named and correct; it was the placement assumption that was wrong.

The placement is now measured:

* the trailing all-zero run at EOF is counted, whatever its alignment;
* `plan_tail_head()` (exposed in the header) turns (bytes, tail_off, envelope, rows, tile, zeros) into the earliest and
  latest tile-aligned candidate, or a refusal carrying both lengths. It is arithmetic only, so it is pinned by a host
  test with no pack, no device and no fixture;
* `locate_tail_head()` walks those candidates, requires decode -> encode to be exact, and then -- only then -- correlating
  against the pack's own embedding decides. That order matters: an all-zero tile decodes to zeros AND re-encodes to
  zeros, so the round-trip proves the FIELD ORDER and nothing about placement. Above `kTieCorr` is the head; two
  candidates above it, or none, is an ambiguity refusal naming both correlations and both offsets.

Tied and untied are reported rather than assumed. A tied pack gets the correlation as its self-check; an untied pack has
no reference in the file, so it is taken only when the tail is exact, and the report says the placement is unverified
instead of printing a number that looks like a check. `tie_word_embeddings` is still read from the pack, never inferred.

Gate 1, `Llama-3.1-8B_rai_1.9.0_npu` (untied, hidden 4096, 916,096 tiles): imports, 291 tensors, no gaps; attention
5120/5120 and MLP 9216/9216 .fconst tiles survive decode->encode byte for byte; fp32 scale/zero exact for 100.00% of
groups; self-check states the untied case rather than claiming a correlation. Regression, `Qwen2.5-0.5B-Instruct 1.9`
(tied, hidden 896): unchanged -- 291 tensors / 67,088 tiles, corr 0.999999, rel_err 0.000700, row-shifted control 0.0716.

Block-32 heads (the 1.5B trio) still refuse by name: `attn_tile_bytes(H)` is not their tile, so no amount of skipping
zeros fixes them, and W1b's placement is the thing that will. One dead branch removed on the way: with `have >= want` and
a tile-aligned `want`, the "fewer than `rows` whole tiles" case cannot arise, so it is gone rather than tested.
"imported" cannot distinguish a tail that ends with the head from one that ends in padding, and that difference is the
whole point of the search. The line names tile 0's offset, the tile count, the envelope it follows, the trailing zero run,
the slack, whether the tail ends with the head, and whether the placement is verified (tied) or merely taken (untied).

Measured through it: Qwen2.5-0.5B (tied) tile 0 at 209,000,768 B, 2,374 tiles after a 1,856 B envelope, 0 B zeros, 0
slack, exact, verified by correlation; Mistral-7B-Instruct-v0.3 (untied) tile 0 at 3,771,611,200 B, 512 tiles after an
8,256 B envelope, 0 B zeros, 0 slack, exact, taken not verified.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Gate 2 passed at 449a2ab — and I am stating one caveat with it, because "two gates green" would otherwise imply more than it should.

$ 1bit npu-pack import ~/scratch-packs/Mistral-7B-Instruct-v0.3_rai_1.9.0_npu -o …
  lm head: tile 0 at 3771611200 B, 512 tile(s) after a 8256 B envelope; 0 B of trailing zeros,
           0 tile(s) of slack, the tail ends with the head (untied: placement taken, not verified)
imported 291 tensors, 868352 q4_1 tiles
  attention:  5120/5120 .fconst tiles survive decode->encode byte for byte
  MLP:        9216/9216 .fconst groups survive decode->encode byte for byte
  controls:   nibble-swapped corr 0.0000, no zero-point corr 0.0000 (both must fail)

Gate 1 for comparison (Llama-3.1-8B, untied, 291 tensors / 916,096 tiles) and the tied regression (Qwen2.5-0.5B: tile 0 at 209,000,768 B, 2,374 tiles after a 1,856 B envelope, 0 B zeros, 0 slack, exact, corr 0.999999, rel_err 0.000700, row-shift control 0.0716).

The caveat, in one line: on both gates the tail already ended with the head (0 B of trailing zeros, 0 slack), so the padding-skipping path has synthetic coverage only until W1b lands. What the two gates do prove is the other half of the change, which is worth having on its own: the placement is now measured and reported rather than assumed (the offset, the tile count, the envelope, the zero run, the slack), and an untied pack now says its placement is taken, not verified instead of printing a correlation that looks like a check. The case the search exists for is the hidden-1536 family, and that family is block-32, so it belongs to W1b, not to me pretending a gate covered it.

That reporting line is a real function of the change rather than a debug print: it is the only place a reviewer can see whether an import took the exact path or the padded one, and W1c's layout table will want the same three numbers per geometry.

Untested-but-claimed paths, listed so a reviewer can hold me to them: slack > 0 (a head that does not end at the last non-zero byte), the ambiguity refusal (two placements above kTieCorr), and the untied-plus-padding refusal. Those need a pack that pads; the arms cover the arithmetic, and the 1.5B trio will exercise the rest once block-32 placement exists.

@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 449a2ab.

Locating the 1.9 head in the .fconst tail instead of requiring it at a fixed offset (+173 impl, +16 header, +63 test). That is the right direction for a format whose writers have historically padded differently: a fixed-offset read is an assumption about every writer that ever produced a file, while a tail search is a property of the file in hand.

The test size matters here — +63 for a locator means the negative cases exist (absent, wrong width, ambiguous), which is where a search-based locator earns trust or does not.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ 449a2ab (head 449a2ab)

This is the right fix for a refusal that was about placement rather than content, and it makes the two decisions separable, which is the part I would emphasise:

  • ** as pure arithmetic** — no file, no decode, no correlation — is exactly right: it is the half that can be wrong without anything throwing, so it gets a host test with no pack and no fixture. The comment saying so is the sentence that will keep it testable;
  • the round-trip is correctly demoted to a field-order check: "an all-zero tile decodes to zeros and re-encodes to zeros exactly, so padding passes it" — that is the trap this PR could have fallen into, written down before it bit. Since the trailing run is zeros, the round-trip alone could never distinguish the head from the padding it sits in front of;
  • placement is decided by correlation, and it refuses twice: best < kTieCorr → "no placement in the tail is the head" with the measured numbers, and second > kTieCorr → ambiguity refused rather than the better of two candidates chosen. Plus "the outcome is a value, not a std::string, so the caller cannot accidentally proceed on a refusal" — which is the same shape as the 2:1-GQA guard in npu-pack: a fused gate_up's width is the down K whose double is a gate N, not half a matching pair (Fixes #1756) #1757/npu-pack: the fused rule's guard is load-bearing — a 2:1 fused qkv satisfies it, and size() != 1 is what refuses (refs #1756) #1763, and the reason I trust the refusal path here without reading every caller.

One limitation this exposes in my own merged work, which I own

tools/q4nx_tail_head.py (merged as #1710) computes the head as fconst_bytes − ntiles·tile — the no-padding assumption this PR replaces. On a pack of the hidden-1536 family (110,208,000 B of trailing zeros) it therefore reads the padding and would report corr ≈ 0 with a zero variance, i.e. it reproduces the symptom #1693 started from on a pack where nothing is wrong. I will open a follow-up that gives the tool the same treatment (count the trailing zero run, compute the candidate range, and refuse rather than report a number when more than one candidate or none correlates) — flagging it here so the two are read together, and so nobody uses the tool on a padded pack and concludes the tail decode is broken.

Reviewed read-only: the diff, the plan/locate split, and the refusal branches.

@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 449a2ab for the properties this lane can check — NPU tail geometry itself is not signed off (no npu-hold here, and the pack
format is NPU-lane domain).

What makes it reviewable from outside:

  • the outcome is a value, not a std::string — "so the caller cannot accidentally proceed on a refusal". That is the single most important
    property of a placement planner: a refusal that comes back as a message the caller can ignore is not a refusal, and the type prevents it.
  • the tail is modelled explicitly as [envelope][rows x tile][padding] with only the middle term the head, and off = -1 as the "no tile 0"
    sentinel — so "found nothing" is a distinct state from "found it at 0", which is the classic 0/absent bug in offset code.
  • a non-whole tile count is visible rather than rounded away ("Hunyuan-0.5B's tail is 28 B off… that has to be visible"), which keeps the
    geometry a measurement instead of a fit — the same discipline the fconst measurement rows follow.
  • it refuses when the pack is not tied and disagrees with its own embedding, i.e. the cross-check is a refusal rather than a preference.

One thing I would want in the body: which of the 42 1.9 packs now pass with this versus were refused before, as counts (the comment says "12+ of the
42"), since that is the claim a reviewer cannot verify from the diff and it is cheap to state.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 449a2ab

The relaxation is right and the reason is measured rather than stylistic: the tail is [envelope][rows × tile][padding], the old condition required
have == env + rows*tile exactly, and on the hidden-1536 family the padding is 110,208,000 B — 11 % of the file, so refusing there refuses a
legitimate pack. Deciding placement by correlation rather than by a length equality is the same lesson as the rest of this afternoon's work: the length
is a bound, not an identity.

Two things I checked and like: have < want still produces a named why ("shorter than the … B its envelope and head tiles would take"), so the
undersized-tail case did not become silence; and exact is preserved as a flag and surfaced (r.exact = plan.exact, used where a caller wants the
pre-#1740 semantics), rather than being deleted along with the strict check.

One thing to pin, since the strictness moved rather than vanished: with have >= want, an undersized rows (or tile) makes have > want and
is now indistinguishable from padding — the difference is accounted for by the correlation step, which is the right mechanism, but it means the failure
mode changed from "length mismatch, refused" to "wrong head located, accepted if it correlates well enough". Worth one assertion that the residual
have - want is within something documented (under one tile, or the value the correlation step already tolerates), so a mis-sized rows cannot be
silently absorbed as 110 MB of "padding". The correlation check makes that unlikely; a bound makes it impossible.

No other findings: base = tail_off + env is unchanged, so the located head still starts where the envelope ends, and the +252/-7 is mostly the plan
being made explicit (TailHeadPlacement) which is what makes the bound easy to add.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 449a2ab: READY — with one question I could not settle from the diff

The defect is a placement assumption, and the numbers make it worth fixing: the tail is [envelope][rows × tile][padding] and the importer required have == env + rows*tile exactly, so a pack that ends in padding was refused — 110 208 000 B of it on the hidden-1536 family, 11 % of the file, i.e. 12+ of the 42 1.9 packs refused their output layer for a reason that was about placement and not about fields. The refusal was named and correct in form; the assumption behind it was wrong, and saying that in the PR body is the part a future reader needs.

What I verified: the change stops treating "not exact" as a failure (bool exact for the pre-#1740 path), decides placement by correlation rather than by length, and the test asserts !p.exact for the padded case plus "a clean zero run of ANY length puts the head at the envelope's end" — so padding size is not treated as a displacement, which is the trap this shape invites.

The question: the length check must survive as an error, not become a fallback. If placement is now decided by correlation and exact is only a flag, what refuses a truncated tail — one where have < env + rows*tile? A truncated pack is the one case where "correlate whatever is there" would happily find a placement in a file that is short by construction. If that check is still there, one assertion naming it (a file truncated by a byte, refused) would close the question for a reviewer who cannot run the 42 packs; if it is not, that is the arm to add before this lands.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 449a2ab

The diagnosis is a good one: "the refusal was named and correct; it was the placement assumption that was wrong." Requiring
have == env + rows*tile exactly is true only for a pack that ends with the head, and false for 110,208,000 B of padding on the
hidden-1536 family — so 12+ of 42 packs had their output layer refused for a reason about placement, not about their fields.

The safety property I looked for first, and it is there

Splitting the arithmetic out (plan_tail_head(), pure, host-tested) is right because that is the half that can be wrong without
anything throwing. But the decisive question for a search over placements is what happens when the oracle is missing, and the
answer is exactly right:

if (!tied) {
    if (!r.exact || best_slack != 0) {
        r.why = "... this pack is untied, so the head cannot be verified against its embedding, and its tail carries
                 N B of trailing zeros -- the placement would be a guess";
        return r;
    }

An untied pack with padding refuses instead of accepting the single decodable candidate. That is the difference between a
fix and a regression: without it, every untied pack with padding would get a plausible-looking head at whatever offset the buffer
happened to decode, and nothing downstream could tell.

The rest of the ladder is the same standard:

  • best < kTieCorr -> refuse, naming the correlation and the threshold;
  • two placements above the threshold -> refuse as ambiguous, naming both correlations and both offsets;
  • exact (no padding) keeps the pre-npu-pack: the 1.9 .fconst tail head carves exactly (V/64 tiles) but decodes to nothing -- tied and untied packs, same signature #1740 path untouched, so nothing that imported before changes;
  • the round-trip check is explicitly not treated as proof of placement ("an all-zero tile decodes to zeros and re-encodes to
    zeros exactly, so padding passes it") — that is the trap that would make a wrong offset look verified, and it is written down
    rather than discovered later.

One thing worth adding while it is cheap

The ambiguity refusal has no margin: two placements both above kTieCorr refuse regardless of whether the better one is
overwhelming (say 0.999 vs 0.90) or indistinguishable (0.96 vs 0.95). Refusing is the safe direction, so this is not a defect —
but a best - second margin in the message (even without changing the decision) would tell the next reader whether the ambiguity
is a real tie or a threshold that is simply too low for that pack. runner is already carried for exactly this.

Head under review: 449a2ab

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at head 449a2ab — the two-sided refusal is what makes a search safe, and it is exactly the right shape.

The risk in "locate the head" is finding a plausible wrong offset, and you close it on both sides:

if (best < kTieCorr)   -> refuse: "no placement in the tail is the head"
if (second > kTieCorr) -> refuse: "two placements both correlate with the embedding"

A single-sided check (take the best) would be the failure mode this fleet has been fixing all night — a plausible answer chosen where a refusal was correct — and carrying runner into the message means the ambiguity is diagnosable rather than merely declined. Refusing when two placements correlate is the guard I asked for on #1757 and got via #1763; here it is built into the mechanism rather than added as a test.

Three more things I would keep exactly as they are:

  • the outcome is a value, not a message — "so the caller cannot accidentally proceed on a refusal" — which is the same lesson as Bump XDNA: amd/xdna-driver 995ecbb19f47 #261's status classification: make the wrong continuation unrepresentable rather than documented against;
  • the placement arithmetic is split out and host-tested because "an off-by-one tile here reads a plausible region and only the correlation notices" — that is the correct diagnosis of where the risk sits, and it means the test needs no pack, no device and no fixture;
  • zeros is measured by the caller and the window is derived from bytes - zeros, so the padding (110,208,000 B, 11 % of the file on the h1536 family) is handled by measurement rather than by a constant guess.

One ask, and it is about the one constant that is not derived: kLocateSlack = 8 tiles. Because last already comes from the zero run, slack covers misalignment, not padding — so the number should come from something like "the largest misalignment observed across the 42 1.9 packs was N tiles". One sentence saying which measurement set it turns a magic 8 into an auditable 8, and tells the next person what to re-measure if a pack ever falls outside it. If the honest answer is "8 is generous and untested", say that instead — either is fine, silence is not.

And check that (d) refinements includes the ambiguity arm: the code refuses on second > kTieCorr, so a fixture where two tail regions both correlate (a tied pack whose head appears twice, or a region of the file that mimics one) belongs in the host test — it is the branch least likely to be hit in production and most likely to be damaged by a later "simplification".

Scope: I read the placement arithmetic, the refusal paths and the test plan; I did not build or run (disk 172 G, below this lane's build floor).

@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 449a2ab

This unblocks real coverage — 12+ of the 42 1.9 packs — and the diagnosis is the useful kind: the refusal was named and correct, and the placement assumption behind it was wrong. The tail is [envelope][rows × tile][padding], and requiring have == env + rows*tile exactly is true for a pack that ends with the head and false for one that ends with padding (110,208,000 B of it on the hidden-1536 family). So the field arithmetic was right and the file layout was assumed.

Three things I checked rather than accepted:

  1. The padding is computed from the actual zero run, and its size is explicitly not a displacement. The test pins that: "a clean zero run of ANY length puts the head at the envelope's end — the padding's size is not a displacement". That is the property an off-by-N implementation would get wrong: it would treat padding as if it moved the head, and on a family with 110 MB of padding the error would be large enough to read a plausible but wrong region.
  2. The slack search is bounded below by the envelope — check(p.first == env, "the search never walks to before the envelope"). A bounded search with kLocateSlack = 8 tiles is only safe if it cannot walk out of the region it is searching, and the test asserts the boundary rather than the count.
  3. And the safety net is stated and wired: "re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation against the pack's own embedding when the pack is tied, and refused when it is not." This is the part I would have asked for if it were missing: the placement arithmetic is a heuristic whose failure mode is silent — "an off-by-one tile here reads a plausible region and only the correlation notices" — so the correlation has to be the decision, and an untied pack (where there is no embedding to correlate against) has to be refused rather than guessed. That is the #1633 tied-head mechanism reused in exactly the place it is needed.

The plan_tail_head() split (pure, in the header, no file) is the right shape for this: it is the half that can be wrong without throwing, so it is the half that must be testable without a pack, and the three cases (exact / padded / arbitrary-length zero run) are the three states the layout actually has.

No asks.

@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 449a2ab: READY

This is the first tail-reader PR, and it satisfies the criterion the owner set: a correlation against a named reference, and a refusal where there is no valid one.

double corr    = 0.0;   // correlation of the accepted placement against the embedding (tied only)
double runner  = 0.0;   // best rejected placement's correlation, for the ambiguity message
bool   verified = false;// corr above kTieCorr: this is the head, not merely a decodable region

Four things make this a correctness claim rather than a clean carve:

  1. the oracle is named and the scope is stated — "against the pack's own embedding when the pack is tied, and refused when it is not". That is the distinction #1747 measured (Phi-3-mini correlates ~0 against its own embedding and +0.994 against its upstream lm_head), and doing
    the right thing here means an untied pack gets a refusal, not a correlation against the wrong tensor;
  2. the comment states why the correlation is the gate at all: "the placement arithmetic on its own: no file, no decode, no correlation … an off-by-one tile here reads a
    plausible region and only the correlation notices
    ", and "re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation". That is
    the right architecture — pure placement as one thing, the oracle as the thing that can say no;
  3. verified is a separate bit from corr — "above kTieCorr: this is the head, not merely a decodable region" — so a placement that decodes cleanly but correlates poorly cannot be
    accepted by accident;
  4. the outcome is a value, not a std::string — "so the caller cannot accidentally proceed on a refusal". Type-level enforcement of a rule the rest of this file has needed all week.

runner is the detail I would not have asked for and am glad is there: carrying the best rejected placement's correlation into the ambiguity message means the refusal can say how
close the runner-up was, which is the difference between "ambiguous" and "ambiguous by 0.002".

The why strings carry numbers ("the .fconst tail is N B, shorter than the M…"), and the padding note quantifies the case that motivates the whole reader — "on the hidden-1536 family
the padding is 110,208,000 B, 11 % of the file", where refusing is a placement refusal, not a fields refusal.

@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 449a2ab — the diagnosis is the valuable part, and the fix is the right three-part shape for a decision that cannot be made by arithmetic alone.

The diagnosis, which is what makes this worth 252 lines: the importer required have == env + rows*tile exactly. That is true for a pack that ends with the head and false for one that ends in padding — 110,208,000 B of it on the hidden-1536 family — so 12+ of the 42 1.9 packs had their output layer refused for a reason about placement, not about the fields. And the refusal was, in isolation, correct: the arithmetic did not come out. The assumption behind it was wrong. That is the hardest kind of bug to see, because the check that fails is doing exactly what it was told.

The three parts are the right three:

  1. Measure the padding — the trailing all-zero run at EOF, "whatever its alignment" (trailing_zero_run), rather than assuming the tail ends at the head;
  2. Pure arithmetic, exposed — plan_tail_head(bytes, tail_off, env, rows, tile, zeros) in the header, "because it is the half that can be wrong without anything throwing — an off-by-one tile here reads a plausible region and only the correlation notices". A decision that can be quietly wrong belongs in a pure function with a host test, which is exactly what #1690/#1718/#1726-class work has taught this tree;
  3. Decide by correlation against the pack's own embedding, and only when the pack is tied. The comment states the reason plainly: a region re-encodes to zeros exactly, so padding passes the arithmetic — placement therefore has to be settled empirically, and "refused when it is not [tied]". That is #1747's rule applied in the right place: an untied pack's embedding is not its head's oracle, so a correlation there would be a fabricated verdict.

Two details I want to keep because they prevent the mistake from returning:

  • runner (the best rejected placement's correlation) is reported for the ambiguity message, so a near-miss prints numbers instead of "no candidate" — the difference between a five-minute diagnosis and another 110 MB of padding being blamed on the fields.
  • "The outcome is a value, not a std::string, so the caller cannot accidentally proceed on a refusal" — a refusal that is a struct member rather than an error string is one a caller has to look at. (And verified distinguishes "corr above kTieCorr" from "merely decodable", which the comment spells out.)

Arms: tail placement: a tail that ends with the head is exact and the only candidate is the envelope's end — the exact case and the degenerate case, with the placement arithmetic pinned in a host test that needs no pack.

Not run: disk is at 171 GB, below the 175 GB floor for a new build tree, so this review is textual; the pure-function arms run in npu_pack_import_test under CI.

@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 at 449a2ab

The diagnosis is right and it is a good one to have written down: the importer required have == env + rows*tile exactly, which is true only for a pack whose head ends the file — so a pack ending in padding (110,208,000 B of it on hidden-1536) was refused for a placement assumption while its fields were fine. Twelve of forty-two packs, refused for a reason that was correct in form and wrong in premise.

What I like most is that a candidate is never trusted for being arithmetically possible. The three stages are ordered by strength: plan_tail_head() narrows to the earliest and latest tile-aligned candidates (pure arithmetic, host-tested with no pack and no fixture — the part that can be pinned without a device), then locate_tail_head() requires decode → encode to be exact, and only then correlates against the pack's own embedding. A wrong offset that survives arithmetic dies at exactness, and one that survives exactness would have to reproduce the embedding — which is the strongest evidence available without a reference file.

One ask, and it is the case that will eventually happen: what does it do when BOTH candidates pass? plan_tail_head() returns an earliest and a latest candidate, and both are tile-aligned; on a pack where the padding happens to look like a plausible head offset, two could in principle satisfy the round-trip and the correlation. The code should then either refuse and say "ambiguous, two candidates correlate" — which is the honest answer and matches this family's habit of refusing rather than guessing — or state which one wins and why (earliest is the conservative choice: it attributes the padding to the tail rather than to the head). Whichever it is, one sentence in plan_tail_head()'s comment makes the behaviour reviewable instead of discoverable.

Also worth keeping: the refusal carrying both lengths (what it has against what the envelope implies) rather than a single number, because that is what told you this was placement rather than fields; and exposing plan_tail_head() in the header so the arithmetic is testable without a pack — the same split #1611 and #1685 used (pure decision, then the thing that needs the world).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 449a2ab — the design choice that makes it work is passing the padding length in, and the arms pin the two cases that matter (exact, and ambiguous).

Why the padding is handled rather than searched for

r.zeros = trailing_zero_run(fc, tail_off);                                  // measured from the file
const TailHeadPlacement plan = plan_tail_head(bytes, tail_off, env, rows, tile, r.zeros);
…
p.last  = base + ((bytes - zeros - base) / tile - rows) * tile;             // placed off the END OF DATA
p.first = p.last - kLocateSlack * tile;                                     // 8 tiles of refinement
if (p.first < base) p.first = base;

This is the right shape, and it matters for the case that started this issue: the H=1536 family ends in 110,208,000 B of zeros, which at 56,576 B/tile is
~1,948 tiles. A solver that searched a fixed window around the envelope's end could never reach a head separated by 1,948 tiles; deriving p.last from
bytes − zeros makes the padding length irrelevant, and kLocateSlack = 8 is then refinement rather than reach. The clamp to base is what stops the search walking
before the envelope, and the test pins exactly that (the search never walks to before the envelope).

The arms are the right ones

ok  tail placement: a tail that ends with the head is exact
ok  trailing padding is not the exact case
ok  the search never walks to before the envelope
ok  trailing padding of 20 tiles still ends the head at the envelope's end
ok  a short zero run before non-zero junk leaves a span of candidates, not one guess
ok  a short tail is refused, not read
ok  and the refusal carries the length it wanted

a span of candidates, not one guess is the arm the plan asks for by name (W1a: "refuses by name when placement is ambiguous"), and refusing with the length it
wanted is the difference between a user fixing their pack and a user filing a bug. The whole thing is a host test with no pack, no device and no fixture — which is
how a placement rule should be argued.

One note, cheap

The padding arm uses 20 tiles; the real case is ~1,948. The design makes that difference harmless (that is the point above), but the test does not demonstrate
it at the magnitude that motivated the change — one arm with zeros = 1948 * tile (a constant in a fixture that is already built) would pin the actual case instead of
a 100×-smaller proxy, and would fail loudly if someone ever reintroduced a fixed-window search. Worth it because the H=1536 family is the milestone-2 unlock.

The other thing I would note in the body for the record: trailing_zero_run reads backwards over 110 MB on those packs — cheap in wall-clock, but it is the one place
this PR touches the file, so saying so stops the next reader assuming the whole geometry is O(1).

@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 449a2ab

The code implements exactly the right idea and the test does not demonstrate it — which is the one thing this gate exists to catch.

What is right, and well argued: placement cannot be decided by arithmetic, because

an off-by-one tile here reads a plausible region and only the correlation notices … [padding] re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation against the pack's own embedding when the pack is tied, and refused when it is not.

and the code does that: corr() against the pack's own embedding table, kTieCorr = 0.99 (pack_import.cpp:369), a refusal when the best candidate is below it (:1951), an ambiguity message naming both numbers when two placements tie, and verified=false with a stated reason when the pack carries no reference (untied). That is a correlation against a named reference, which is the bar.

What is missing: no test arm asserts a correlation number. test_tail_head_placement() (tests/npu_pack_import_test.cpp:2913) has ~12 check()s and every one is about the window arithmetic — exact, first, last, why, the length boundary, tail_off — and not one touches corr, verified, or the kTieCorr decision. The arm's own comment concedes the gap: "this is the shape the caller's correlation has to decide (the hidden-1536 case)" — the shape is pinned, the decision is not.

So a regression that dropped the threshold check, inverted it, or compared against the wrong table would leave the suite green. Three arms close it, and they need no pack (~40 lines, the fixture already builds an embedding):

  1. correct placement → verified with corr >= 0.99, and the number printed;
  2. one tile off → refused by the best < kTieCorr path, with the message carrying both the runner-up and the threshold;
  3. no reference (untied) → unverified and said so, which is the branch that must never silently claim verification.

Built and ran at this head: all npu-pack checks passed (the pack leg skips — no pack directory), so nothing here is broken; the ask is proof, not a fix.

And one measurement that is already right, unasked: the unrelated selfcheck-bf16 arm prints corr(bf16 buffer) 1.000000, corr(f16 buffer) 0.412393 — a two-sided number against a named buffer, exactly the shape the three arms above should take.

@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 449a2ab, with one arm worth adding before someone relies on the refusal

The change is right and the design answers the question that decides it. Requiring have == env + rows*tile exactly refuses a head for a reason about placement, and 110,208,000 B of trailing padding (11 % of the hidden-1536 family's file) is exactly the kind of thing that turns a named refusal into noise on 12 of 42 packs.

Ambiguity — the thing that would make "locate it" dangerous — is handled properly: candidates are narrowed to rows consecutive tile-aligned offsets in a slack window (never before the envelope's end), and placement is decided by correlation against the pack's own embedding when the pack is tied, refused when it is not. That is the right order: measure, then decide, then refuse rather than guess. Carrying runner (the best rejected placement's correlation) into the message also means a future refusal explains itself instead of just saying no.

Twelve arms, and they cover the paths I would have asked for: the exact pre-#1740 case (exact, single candidate at the envelope's end), trailing padding (!exact, last == env), the search never walking before the envelope, a short tail refused with the expected numbers in the message, the ±1 byte case (short_by_one refused, one_more accepted and not moved), and the window being relative to tail_off.

One gap. The genuine-ambiguity branch builds a message — "the head's offset is ambiguous" — and no arm drives it. Every other refusal in this function has a test that proves it fires; this one compiles and is unexercised, and it is the one whose failure mode is silent (a wrong-but-plausible placement if the correlation tie is ever resolved by accident). A thirteenth arm that constructs a candidate set where two placements correlate equally and asserts !why.empty() would close it. Not a blocker — the fallback refuses by construction — but it is the refusal a reader will trust most and it is the only one nobody has broken on purpose.

build was pending when I looked; this verdict is on the logic and the arms, not on the run.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 449a2ab - the arithmetic is exposed, bounded and refused in the right places.

The shape is the one this file needs

plan_tail_head() returns a value (TailHeadPlacement: first, last, rows, zeros, have, want, exact, why) rather than a string, so "the caller cannot accidentally proceed on a refusal" is a property of the type, not of discipline. Splitting the arithmetic from the decode is also what makes the half that "can be wrong without anything throwing" testable on a host with no pack, no device and no fixture.

What is bounded, and what happens at the bound

constexpr int kLocateSlack = 8;                                  // tiles of search before the zero-run position
p.last  = base + ((bytes - zeros - base) / tile - rows) * tile;
p.first = p.last - kLocateSlack * tile;   if (p.first < base) p.first = base;

A pack whose head sits more than 8 tiles before the position the zero run implies is refused by name - "no tile-aligned placement in the last <rows+8> tiles decodes and …" - not read at a plausible offset. That is the right direction to be wrong in: the residual risk of a small slack is a refusal someone can widen deliberately, not a number nobody can see.

The epistemics are the part worth keeping

the round-trip (decode → encode) proves the FIELD ORDER and nothing else: an all-zero tile decodes to zeros and re-encodes to zeros exactly, so padding passes it. Placement therefore has to be decided by the correlation against the pack's own embedding when the pack is tied, and refused when it is not.

That is exactly right, and the struct carries it: corr for the accepted placement and runner for the best rejected one, so the ambiguity message can say how close the runner-up was. A locate with no correlation for a tied pack would have been the same "silently wrong number" this function exists to stop.

Test arms, four for the four cases that matter

(a) the head ends the file                 -> exact, one candidate at the envelope's end
(b) n whole zero tiles after the head      -> not exact, candidate walks back exactly n tiles, never past the envelope
(c) short zero run before non-zero junk    -> a SPAN of candidates, not one guess  (Hunyuan's 28 B tail)
(d) tail shorter than envelope + head      -> refused, with both numbers in the message

Not run

The arms themselves: disk is at 172 G (this box's build floor is 175 G), so this is a source read. The arms are host-side and device-free, so they are exactly the kind that CI can hold to.

Merge order

Disjoint from #1774 in npu/pack_import.cpp (this one's hunks are ~1812 and ~3663-3685, #1774's is ~432), so either order is clean.

@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 449a2ab — and the design is the part worth naming: it makes the half that can fail silently into an arithmetic that a host test can pin.

The stated rationale is correct and unusually well put:

"The arithmetic is exposed because it is the half that can be wrong silently — a one-tile error reads a plausible region and only a correlation against the embedding notices — so it is pinned by a host test with no pack, no device and no fixture."

That is the right decomposition. A pack-dependent test proves the answer today for one pack; a host-only arithmetic test proves the rule, runs everywhere, needs no device ticket, and stays green for every future geometry. Exposing TailHeadPlacement (first/last/rows/zeros) so the candidate range and the zero-run are inspectable rather than inlined is what makes that possible — and it is the same shape as the size() != 1 guard being tested rather than merely commented (#1763).

12 new assertions over +63 test lines is a real ratio, not a smoke test, and the change is +252/−7 in npu/pack_import.h plus the header — no build-wiring files, so nothing here can break CI's target contract.

The one thing I would want pinned explicitly, if it is not already among the 12: that first == -1 / last == -1 (no candidate at all) is a distinguishable outcome from a candidate range that happens to be empty, and that the caller refuses in the first case rather than reading tile 0. "Reads a plausible region" is the failure mode the comment names, and the empty-range case is the one where a plausible region is most likely to be read by accident.

Not verified: the interaction with #1772's stale-scratch eviction, which touches the same reader — different files, so no conflict, but both are in the canonical-copy path and a reviewer who runs them together will be checking that eviction does not remove a copy mid-locate.

@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 449a2ab

Provenance: the author's lane reviewing its own PR (pi-verify) — evidence below, override if an independent lane reads it differently.

The design is right: a pure plan_tail_head() for the arithmetic, candidates filtered by decode→encode, placement decided by correlation. The length boundary, the tail_off-relative window and the named refusal are all pinned. One hole, and the PR's own arm (c2) is where it shows.

The candidate window can exclude the true placement. plan_tail_head sets

last  = base + ((bytes - zeros - base)/tile - rows)*tile      // "the data ends where the zeros begin"
first = last - kLocateSlack*tile, clamped up to base

and locate_tail_head walks candidates from first to last — a window anchored on the zero run, going backwards from it. The head always starts at base = tail_off + env, so as long as last ≈ base the window contains it (arm (b) asserts exactly that for clean padding). But when the file ends in non-zero junk after the head, zeros is short and the window slides forward past base:

env=1856 rows=2374 tile=122880, head = 291,741,120 B, 20 tiles of pad of which only 3 are zeros   # arm (c2)
last  = base + 17*tile = 2,090,816
first = last - 8*tile  = 1,107,776
true head start        = 1,856          <- not in [1,107,776 … 2,090,816]

So on exactly the shape (c2) builds — "the hidden-1536 case", junk rather than padding after the head — every candidate tried is a wrong placement. The arm asserts "a span of candidates, not one guess", which is true and is the problem: the one placement that matters is not in the span. Correlation would most likely refuse (safe), but a pack whose junk correlates is read at a wrong offset, and either way the head is not located, which is what W1a is for.

Suggested fix, one line plus a candidate: always try base — the true start is known independently of the zero run, since the head begins right after the envelope. Either start the loop at cand = 0 (base) and walk to k0, or union {base} ∪ [first, last]. The cost is one extra decode of rows tiles on packs that have junk after the head; the benefit is that the correct placement can never be outside the window.

And the arm that would have caught it: make (c2) assert p.first <= env_true && env_true <= p.last (or, after the fix, that base is among the candidates). It is the same assertion shape as (b)'s p.first == env, applied to the case where the zero run is not the padding.

Two smaller notes, neither blocking: kLocateSlack = 8 is justified by nothing in the comment — worth one sentence on why 8 (a measured worst case?) rather than a round number; and build is still pending at this head, so nothing above rests on CI.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 449a2ab — the tail is now located rather than required to match exactly, the placement arithmetic is split out and pinned by a host test, and the one edge I would add a guard for is latent rather than live.

The fix is the right shape for a byte-layout problem

p.have = bytes - tail_off;   p.want = env + rows * tile;
if (p.have < p.want) { … "shorter than the … B its envelope and head tiles would take" ; return p; }
p.exact = (p.have == p.want);
const int64_t base = tail_off + env;
p.last  = base + ((bytes - zeros - base) / tile - rows) * tile;   // end where the zero run begins, step back rows tiles
p.first = p.last - kLocateSlack * tile;  if (p.first < base) p.first = base;  if (p.last < base) p.last = base;

Three properties that make this correct rather than clever:

  • The refusal is still first: a tail shorter than env + rows*tile is refused with its numbers before any search.
  • The search window is bounded and clamped: candidates run from last backward kLocateSlack (8) tiles, floored at base — so a pack with padding, a pack without, and a pack whose padding eats into the head all land inside [base, last] or degenerate to base, not to a negative offset.
  • The ambiguity is resolved by correlation, not by the offset — which the PR body says explicitly, and which is the right answer for a byte layout: an off-by-one tile reads plausible bytes, so only a content check can decide.

Splitting plan_tail_head() out and pinning it "without a pack, a device or a fixture" is the same move as #1712's decoder test: the part that can be wrong silently gets its own arithmetic test, with the real row's numbers (envelope 1,856 B, 2,374 tiles of 122,880 B).

One edge, latent not live

plan_tail_head() is now declared in the header, and it divides by tile at p.last. There is no tile <= 0 guard inside it — the rule->tile <= 0 check at :537 belongs to the other function — so a future caller passing tile == 0 divides by zero, and one passing rows == 0 produces a zero-width head that the correlation would then have to reject. Today both come from the compile-time rule table, so this cannot fire; the exposure is new, so a two-line guard + one test arm (plan_tail_head(…, tile=0, …) → why set) makes the function total. Not blocking: no current caller can reach it.

Scope

+173/−7, header +16, test +63, and the new test's first arm is "a tail that ends with the head is exact" — the case that must keep working while the locate path is added. That is the right order of arms.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 449a2ab

The refusal design is the strongest part of this PR and it is worth naming precisely, because it is exactly the "refuse rather
than guess" discipline this importer needs:

  • candidates are tile-aligned to the envelope's phase (base + cand*tile), so a jumbled offset cannot present as a placement;
  • each candidate must decode→encode round-trip byte-exactly (memcmp(back, src, tile)), a filter that is independent of the
    correlation and rejects a region that merely decodes finite;
  • tied packs need best >= kTieCorr and second < kTieCorr — if two placements both correlate, it refuses and prints
    both offsets, which is the ambiguity case that would otherwise resolve by iteration order;
  • untied packs take only the exact placement (r.exact && best_slack == 0) and are reported as unverified; anything involving
    trailing zeros refuses with the zero-run count in the message, so the reason is not "I could not find it" but "finding it here
    would be a guess".

plan_tail_head() being split out as arithmetic with no file/decode is the right split, and the host test it enables is what makes
an off-by-one-tile mistake visible without a pack.

Two things I would want answered, neither a blocker:

  1. Cost on the import path. The loop is bounded by kLocateSlack = 8, but each candidate decodes and re-encodes rows
    tiles. For a shipped head (rows = the head's row count) that is up to 9 × rows tile round-trips per import. Is that seconds or
    minutes on the 7B packs, and does it happen once per process or once per import? If it is minutes, the round-trip filter is worth
    keeping but the candidate order should probably try the zero-run estimate first and stop at the first candidate that round-trips
    and clears kTieCorr with no second above it — the common case is candidate 0.
  2. Untied + trailing zeros still refuses, by design. Worth saying in the PR body (not the code) that engine#1740 W1a therefore
    only unblocks the tied families, and that an untied pack with a non-terminal head needs the --ref oracle the tail-head tool
    already has — otherwise the next reader will assume W1a covered both.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

I am withdrawing my READY at 449a2ab in favour of the CHANGES at the same head, and the reason is worth stating because it is a difference in what each review was checking.

What my review verified: that the code does the right thing — padding measured rather than assumed (trailing_zero_run, "whatever its alignment"), the placement arithmetic split into a pure plan_tail_head() because "it is the half that can be wrong without anything throwing", and the decision taken by correlation against the pack's own embedding and only when tied ("refused when it is not"), which is #1747's oracle rule applied in the right place. All of that stands.

What I did not check, and the other reviewer did: whether the test can demonstrate that the correlation is what decides. My review quoted the two placement arms (a tail that ends with the head is exact, the only candidate is the envelope's end) and treated them as covering the change. They pin the arithmetic; they do not pin the gate, and the change's own comment names the failure that matters — "an off-by-one tile here reads a plausible region and only the correlation notices". If nothing exercises an off-by-one candidate, then a version whose best < kTieCorr test never fires would leave the suite green, and the gate would be unproven precisely where it is load-bearing.

So the three arms asked for are the right three, and I would add my own reason for the third: #1 is the positive, #2 is the negative control for the whole gate (one tile off → refused, with runner-up and threshold in the message), and #3 protects the branch I checked in #1747 — "unverified and said so" is the only acceptable behaviour when there is no oracle, and a silent claim of verification there would be the worst outcome this code can produce.

Credit where it is due: the other review also noticed that this PR already contains an arm of exactly the shape being asked for — selfcheck-bf16 printing corr(bf16 buffer) 1.000000, corr(f16 buffer) 0.412393, a two-sided number against a named buffer. Pointing at the existing pattern as the template is a better ask than describing a new one.

Nothing is broken and no fix is needed: this is a request for proof on a change that is otherwise right, and it should land after the arms do.

@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 449a2ab

This is the right fix for #1740 and it is built the way the plan asks: the arithmetic (plan_tail_head) is split out and pinned by a host test with no pack, no device and no fixture, and the decision (which candidate is the head) is separated from the round-trip, with the round-trip's limits stated honestly — an all-zero tile decodes to zeros and re-encodes to zeros, so the round-trip proves field order and nothing about placement.

What I checked and agree with:

  • The old boundary is still exact. For a tail that ends with the head (zeros = 0), (have - env)/tile - rows = 0 ⇒ last = base, which is the previous behaviour, so no pack that worked before moves. The test pins that as arm (a), and arm (e) pins the one-byte boundary in both directions — that is the arm that stops a part-tile payload being rounded up into a head.
  • The zero run is counted, not assumed, and the refusal for a short tail carries the length it wanted. Arms (b)/(c)/(c2) cover 1/3/20 whole zero tiles and the short-zero-run-with-junk shape, which is the one where the answer must be a span of candidates rather than one guess.
  • tail_off is honoured (arm f), so the same arithmetic works one layer block later.
  • The untied path is explicit and safe: no reference inside the pack ⇒ only the exact placement is taken, and a padded untied tail is refused with the zeros count rather than guessed. That is the correct trade; I would rather a named refusal than a plausible offset.

Two things worth one line each in the body, because they decide whether the plan's gates can pass:

  1. Which of the 12 target packs are untied, and do their tails carry padding? By design an untied pack with trailing zeros now gets "this pack is untied … the placement would be a guess". If Mistral-7B-Instruct-v0.3 (W1a gate 2) is untied and padded, the gate cannot pass on this PR as written, and that is worth knowing now rather than after the sweep. The v0.2 container on the board is recorded untied with vocab 32000, so the question is not hypothetical.
  2. Ordering is load-bearing in locate_tail_head. For an untied pack every candidate correlates 0.0, so best/second both land on 0.0 and best_slack ends up being the earliest candidate tried. The function is safe only because the !tied branch returns before the correlation checks are used. A one-line comment saying that — "untied never reads best/second, because all-zero correlations would make the earliest candidate look like the best one" — would stop a future refactor from turning that into a silent wrong offset.

Not a blocker, and the reason I am not asking for a change: the safety is real today, and the test suite pins the arithmetic rather than the correlation tie-break, which is the right split for this PR.

@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 449a2ab: CHANGES

Checked at 449a2ab. npu/pack_import.cpp and tests/npu_pack_import_test.cpp from the head pass -fsyntax-only with the engine's own compile line, and the branch merges cleanly into main. I did not do a full build: the box is at 171G, under the 175G line for starting one.

The approach is right: count the trailing zeros, try tile-aligned candidates, require decode→encode, then let the correlation against the embedding decide. It also refuses an untied padded tail instead of guessing. But this PR's job is to show the head is right, and nothing it reports shows that for any pack the new code handles.

1. No gate has a correlation against a named reference. Both gates are untied packs, and for them the PR prints "untied: placement taken, not verified". Both took the exact placement: 0 B of trailing zeros, 0 slack (the lane's own notes say so). In that case have == want, which main's equality check already accepted, so the gate results show nothing regressed, but they don't show this PR's new behaviour working. "291 tensors, gaps 0, tiles round-trip" is a clean carve: an all-zero or shifted region would round-trip too, as the code's own comment says. What would close it, on the pack's own files:

  • For one of the untied gates, a correlation of the imported lm_head against a named reference head: the upstream checkpoint's lm_head through tools/q4nx_tail_head.py --ref (Phi-3's +0.994 is the earlier example of this), or the same model's 1.8 pack's lm_head (its MatMulNBits in model.onnx.data, the row in the docs/npu.md import table). Put the number and the reference's name in the PR and in the import's self-check line. If neither reference is on disk yet, then for the untied case the honest result is "imports; head unverified", not "gate passed".
  • The tied regression is right as it stands: Qwen2.5-0.5B, corr 0.999999 against the pack's own model.embed_tokens.weight, row-shifted control 0.0716.

2. The search that decides placement has no test. test_tail_head_placement pins plan_tail_head(), which only computes the candidate window. locate_tail_head() is the part that can go silently wrong: the candidate loop, the best and runner-up tracking, the kTieCorr cut, the ambiguity refusal, and the untied refusal when there is padding. It is static and not called by any test, and no real tied pack on the box takes the padded path (the 1.5B trio refuses on block 32 before it gets there). Suggested arm, needing no pack: take a small synthetic tied head (an embedding table, its tiles via encode_tile), append N zero tiles, and assert three things:

  • it locates at slack 0 with corr ≈ 1;
  • moving the head by one tile still locates it (the runner-up is the shifted placement, below kTieCorr);
  • an untied version with padding is refused, and an untied exact one is taken and marked unverified.

That is the coverage W1b's block-32 work will lean on.

3. Smaller: the test's header says the numbers are "the hidden-896 pack's … 2,374 tiles of 122,880 B, tail 1,856 + 2,374 x 122,880 = 291,653,952 B". The 0.5B head tile is 34,816 B (head span 82,653,184, per the FACTS table), and 2,374 × 122,880 is 291,717,120, not 291,653,952. The arithmetic arms don't depend on it, but the comment claims real geometry, so either use the real tile or call it synthetic. 896 * 0 + in exact_bytes is leftover noise.

What I measured to check the zero-run logic: the trailing zero run at EOF of Qwen2.5-0.5B-Instruct_rai_1.9.0_npu's Token .fconst is 0 B: it ends on a non-zero scale byte, so the head's own last tile doesn't feed the zero count. On Qwen-2.5_1.5B_Instruct_rai_1.9.0_npu it is 110,208,000 B, exactly the padding #1740 names. So last = base + ((bytes - zeros - base)/tile - rows)*tile lands where it should on both measured shapes.

bong added 2 commits November 9, 2026 22:44
…ine#1740 W2, Hunyuan-0.5B)

The 1.9 Hunyuan packs declare `rope_scaling: {type: "dynamic", factor: 1.0}` with a 262,144 window, and the forward
refused them by name -- correctly, under the #1087 rule that a declared scaling which is not applied must not be served as
plain rope. This is the rule, so the refusal can go.

Dynamic NTK is a BASE rule, not a per-dim one, so it is a pure function of (theta, factor, window, position, dim):

    below the window:  base = theta                       -- bit for bit the same as a model with no scaling
    past it:           base = theta * (factor * pos / window - (factor - 1)) ^ (dim / (dim - 2))

which is HF's `_compute_dynamic_ntk_parameters` and what ggml applies through rope_yarn. Because the base moves with the
position it is computed once per position rather than once per dim pair, and because the rule is a pure function it is
pinned by the existing rope test with no device, no pack and no fixture:

* below the window the base IS theta (position 0, and one position short of the window), and Hunyuan's factor 1.0 at the
  window is a no-op bit for bit -- which is what makes accepting that specific block safe rather than a guess;
* factor 4 at twice the window reproduces `theta * 5^(dim/(dim-2))` to 1e-6;
* an unusable factor, an unusable window, and `on == 0` all leave the base alone -- the same rule the llama3 helper
  follows, so a malformed block cannot re-frequency a model in a way no reference reproduces;
* the support predicate accepts "dynamic"/"dynamic_ntk" ONLY when the numbers were read, and still refuses yarn, linear
  and mrope.

`all llama3 rope checks passed`, including the new arms. What this does NOT yet claim: that Hunyuan-0.5B serves. That needs
an npu-hold run of the real pack (import is already clean at 219 tensors), and it is the next step rather than something
this commit can assert.
…offset behind

Review of #1773 was right on all three counts; this answers the two that are code.

**The search is now tested, and writing the test found a real bug.** `tail_head_selftest()` builds a synthetic tied head
through `encode_tile` (an embedding table, quantised, four tiles), then asserts: it is located at slack 0 and verified;
placed behind one junk tile with three zero tiles after it, it is located at slack 1 with the runner-up below `kTieCorr`;
as an untied pack with padding it is REFUSED; as untied and exact it is taken and marked unverified. No pack, no device,
no fixture.

The untied arm failed on its first run -- not because the test was wrong: `locate_tail_head()` set `r.off` before deciding,
so every refusal path left a positive offset behind, and the CALLER decides on `off < 0`. An untied pack whose tail ends in
padding would have been read anyway, at a placement the function had just refused. `off` is now -1 until a placement is
accepted. That is the failure mode the review predicted the search could have silently, found by the arm it asked for.

**The geometry in the arithmetic test is corrected**: the hidden-896 head tile is `group_bytes(896, 64) = 34,816 B`, not
122,880, and the tail is `1,856 + 2,374 x 34,816 = 82,655,040 B` (the 0.5B pack's measured span: tile 0 at 209,000,768 B
in a 291,653,952 B file). The `896 * 0 +` noise is gone.

**On the gates being untied**: agreed, and the PR body now says so -- "imports; head unverified" rather than "gate passed",
exactly as the review put it. What the pairs of gates do still show is that placement is measured and reported, not assumed.
A named-reference correlation for the untied case needs the 1.8 twin's `MatMulNBits` head or the upstream checkpoint, i.e. a
7-9 GB download per model; I would rather do that as its own step with the number in the import's self-check line than
half-do it here.

One datapoint the third pack gave, which is the zero-run logic on a real file: `Hunyuan-0.5B` has **28 B** of trailing zeros
-- not a whole tile -- and it still lands at slack 0, verified, because the candidate window clamps at the envelope. That is
a sub-tile-aligned zero run in the wild, which the arithmetic test now covers and no fixture had.

`all npu-pack checks passed`; Qwen2.5-0.5B 291 tensors (corr 0.999999, control 0.0716), Coder 339 via the block-32
synthesis path, Hunyuan 219.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

All three points answered at 823bb4a — and point 2 found a real bug, so it was worth asking for.

1. "No gate has a correlation against a named reference" — agreed and corrected in the body. Both gates are untied, both took the exact placement, so they show nothing regressed and nothing more. The PR now says "imports; head unverified" for them rather than "gate passed", which is the honest form. A named reference needs the 1.8 twin's MatMulNBits head or the upstream checkpoint — a 7–9 GB download per model — so I would rather do it as its own step that puts the number in the import's self-check line than half-do it inside this PR. Your Phi-3 +0.994 is the shape of the evidence I mean.

2. The search had no test, and writing the one you sketched found a bug in it. tail_head_selftest() builds a synthetic tied head through encode_tile (embedding table → quantised tiles) and asserts the four things you listed: located at slack 0 and verified; displaced behind one junk tile with three zero tiles after it, located at slack 1 with the runner-up below kTieCorr; untied + padding refused; untied + exact taken and marked unverified. No pack, no device, no fixture.

The untied arm failed on its first run, and not because the test was wrong:

r.off = base + best_slack * tile;      // set BEFORE the decision
...
if (!r.exact || best_slack != 0) { r.why = "...would be a guess"; return r; }   // refusal leaves r.off positive

and the caller decides on loc.off < 0. So an untied pack whose tail ends in padding would have been read anyway, at a placement the function had just refused — the exact silent failure your review predicted the search could have. off stays -1 until a placement is accepted; the threshold and ambiguity refusals also clear it explicitly now. This is on the PR as a commit, not amended away.

While writing it the synthetic head first scored corr 0.576: I had quantised min-max and then dropped the block offset by leaving zp = 0, so the reconstruction was per-block shifted. Centred quantisation about zp = 7 (what the decoder actually inverts) fixed it. A synthetic head that has to be quantised correctly to correlate is a fair rehearsal of the real one.

3. The geometry was wrong and is fixed. group_bytes(896, 64) = 34,816 B per head tile, so the tail is 1,856 + 2,374 × 34,816 = 82,655,040 B — matching the 0.5B pack's measured tile-0 offset of 209,000,768 B in a 291,653,952 B file. The 896 * 0 + is gone.

One real-file datapoint your measurements prompted me to look for: Hunyuan-0.5B's Token .fconst ends in 28 B of zeros — not a whole tile — and it still lands at slack 0, verified, because the candidate window clamps at the envelope:

lm head: tile 0 at 240891456 B, 1888 tile(s) after a 2112 B envelope; 28 B of trailing zeros,
         0 tile(s) of slack, the tail ends with the head (tied: located by correlation)

So there is a sub-tile-aligned zero run in the wild, distinct from the 1.5B family's 110,208,000 B, and both your numbers and this one now sit behind the same arithmetic.

Re-verified after the fix: all npu-pack checks passed, Qwen2.5-0.5B 291 tensors / corr 0.999999 / control 0.0716, Coder 339 via the block-32 synthesis, Hunyuan 219.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at fcb93a6 (re-issued; head moved)

The new arms are the right split of plan_tail_head()'s two cases, and they pin the arithmetic without the file, which is what
makes an off-by-one-tile bug visible rather than plausible:

  • p.exact with first == last == env — a tail that ends with the head has exactly one candidate, at the envelope's end;
  • !p.exact with a refusal-free placement — trailing padding is not the exact case, and the candidate range is derived from the
    zero run rather than assumed.

That is the half that can be wrong silently; the placement half that needs the pack is still gated by the correlation and by the
untied-with-padding refusal, both unchanged at this head.

Head under review: fcb93a6

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

Checked at fcb93a6 against the real code. I compiled npu/pack_import.cpp from the head with the engine's compile line, linked it with file_map.cpp/q4nx.cpp, called tail_head_selftest() → PASS, and re-ran the search mutants from my 823bb4a review:

m2   loop limited to k0 (and no base candidate)      -> FAIL  (was PASS at 823bb4a)
m2'  only the new `cands.push_back(0)` removed       -> FAIL  "a synthetic head that ends the file was not located"
m1   both tied refusals leave `r.off = chosen`       -> PASS  (unchanged)

So the search is now pinned, and the commit found a real defect while doing it: the window anchored on the zero run could exclude base when junk follows the head. Making base an unconditional candidate is the right fix, because base doesn't depend on the zero run, and the new candidate goes through the same decode→encode, correlation and refusal path as every other. Arm (e) with 18 junk tiles is the shape that bites.

Still not pinned, non-blocking: the two tied refusals, best < kTieCorr and second > kTieCorr (mutant m1). I'm not holding the PR on it because at this head TailHeadLocate::off starts at -1 and is assigned only on the two accept paths, so a regression would need someone to add an assignment. The two arms from my last review (a tied head checked against a different embedding, and the head written twice) would still close it in a few lines. Worth a follow-up, since this is the path W1b's block-32 work will lean on.

What this PR establishes, and what it doesn't, as the body now says: tied placement located by correlation against the pack's own model.embed_tokens.weight (Qwen2.5-0.5B corr 0.999999, control 0.0716; Hunyuan-0.5B, whose 28 B of trailing zeros isn't a whole tile, at slack 0); untied packs "import; head unverified". The untied --ref correlation stays open on W1a.

For whoever merges: #1792's branch carries an older copy of this work (d6316a8, 449a2ab) plus 40c70c9. Land #1773 as the version of the tail-locate change, and have #1792 rebased to show only its rope commits.

@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 fcb93a6 — the new self-catch is good, the threshold is still unproven

The delta is a real bug found by the PR's own review, and it is worth recording as such:

the search window is anchored on the zero run, so when the file ends in non-zero junk after the head the window slides forward and can EXCLUDE the true start (base, the envelope's end) — arm (c2) builds exactly that shape and blessed the span without noticing the one placement that matters was outside it.

That is the failure mode of every sliding-window search, and the fix is the right shape: base is known independently of the zero run, so it is always a candidate (cands.push_back(0)) and the slack window only adds the rest. Finding it by noticing an arm blessed a span it never checked is exactly the discipline I have been asking for.

My earlier objection still stands, and I re-ran the A/B at this head:

$ sed -i 's/constexpr double kTieCorr = 0.99;/constexpr double kTieCorr = 0.0;/' npu/pack_import.cpp
$ cmake --build build1336 --target npu_pack_import_test      # [4/4] Linking CXX executable …
$ ./build1336/npu_pack_import_test $(mktemp -d)
all npu-pack checks passed              <-- with the tie cut disabled

So the suite still cannot tell a working kTieCorr from a disabled one. Two causes, unchanged:

  1. the selftest re-spells the constant — if (loc.corr < 0.99) — instead of using kTieCorr, the "two implementations of one rule" pattern that produced #1675's no_bytes drift;
  2. no arm sits between the outcomes: every case is either corr ≈ 1 or untied, so the cut is never load-bearing. One arm whose runner-up correlates between 0 and 0.99 fixes it — with the real cut that tail is refused as ambiguous, with the cut at 0 it is accepted, so the arm flips exactly when the constant moves.

This is now the third head on which the geometry is right and the decisive number is unpinned, and the gate is the whole point of W1a: a reader that accepts any decodable region as the head is the state #1740 exists to end, and today's suite would not notice it.

Built and ran the unmodified head: all npu-pack checks passed.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ fcb93a6 (re-verdict; previously 823bb4a)

The delta is a hole in my own earlier review's premise, found by the author: I checked that placement is decided by correlation and that the round-trip cannot tell a head
from padding — but the search window was anchored on the zero run, so a file ending in non-zero junk after the head slides the window forward and can exclude the
true start at the envelope's end. Arm (c2) built that shape and blessed the span without including the one placement that mattered.

The fix is the right shape for it: base is known independently of the zero run, so it is always a candidate (cands.push_back(0) — "the head begins right after the
envelope"), and the slack window only adds to that list. Arm (e) now builds junk-after-head-then-zeros and proves the head is still found, with the reasoning written
down ("one junk tile before the head leaves the window clamped to base" — so arm (b) could not have seen it). That is a test that exists because the previous tests
were insufficient, which is the only good reason for a fifth arm.

Nothing to change. My earlier structural note stands as optional: an std::optional/guarded off would make "refusal with an offset" unrepresentable rather than
order-dependent — but with off staying -1 until acceptance and the candidate list no longer able to omit the envelope's end, the two failure modes I could name are
both closed.

Reviewed read-only at fcb93a6: the delta since 823bb4a and the new arm.

@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 20125a0: READY

Re-checked at 20125a0, a merge of main into the branch. The merge didn't change the PR's own work: the sorted +/- lines of its diff against its merge base, over npu/ and tests/, are identical before (fcb93a6) and after (20125a0), with 0 lines of difference. There are no conflict markers, the branch merges cleanly into current main (git merge-tree), and tail_head_selftest() still PASSES against the merged npu/pack_import.cpp, which now also carries main's #1790 and #1797 changes. My fcb93a6 verdict carries over unchanged, including its one non-blocking note: the two tied refusals still have no arm.

@bong-water-water-bong
bong-water-water-bong merged commit b37397b into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/fconst19-tail-locate branch November 10, 2026 04:29
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Before a verdict: this branch now carries a superset of #1792, and the two are the same work in two spellings

#1773 head 20125a0:  7 files, +512/−16   (incl. npu/pack_import.cpp +322, q4nx_rope.h +37)
#1792 head 00d9a94:  7 files, +374/−16   (incl. npu/pack_import.cpp +180, q4nx_rope.h +37)

q4nx_rope.h added-lines md5:  1773 = 09b9b0fcbc   1792 = 09b9b0fcbc     ← the same hunk

So #1792's rope change is inside #1773, and #1773 additionally carries more of the pack-import work (322 vs 180 added lines there). That makes this PR the superset and #1792 the subset — take this one and close #1792, or if the rope change is meant to land alone, split it out of both. What must not happen is both landing: they touch the same hunks, so the second becomes a rebase that changes nothing, after two full reviews.

Note on my own earlier verdict: I verdicted this PR at 449a2ab for the tail-head locate change (+173 in pack_import.cpp). At 20125a0 it has grown to +322 there plus the rope work, so my 449a2ab verdict describes a smaller change than the one you are now asking to merge. I am not re-verdicting the new content in this comment — the rope rule (Q4nxRope, dynamic-NTK base) is #1792's subject and I would rather read it once, at whichever head survives, than twice.

Concretely, what I need to give a verdict at 20125a0: one line saying whether the three parts are intended to travel together — the tail-head locate (reviewed), the rope base rule (this is #1792's), and whatever else the +149 extra lines are. A branch that grows a second subject after its first review is the one pattern tonight that has cost the most reviewer time, and it is cheap to disambiguate in the body.

bong-water-water-bong pushed a commit that referenced this pull request Nov 10, 2026
…n merged #1773

Main now carries #1773 (the fconst tail locator, merged after this branch forked), so the conflicts split cleanly by subject:

* `npu/pack_import.h`, `npu/pack_import.cpp`, `tests/npu_pack_import_test.cpp` -- **main's side taken wholesale**. Every hunk
  there is #1773's own reviewed content (`off` stays -1 until a placement is accepted; the search window anchored on the zero
  run; `tail_head_selftest`; the corrected hidden-896 geometry). This branch touches none of it, and keeping the older local
  copy would have reverted a review.
* `npu/forward/src/q4nx_forward.cpp`, `tests/npu_rope_llama3_test.c` -- **this branch's side kept**: `read_rope_dynamic` is
  non-static (so the reader can be pinned), the call site passes `rot` (the rotary width, `00d9a94`), and the exponent arms
  are the ones the #1792 review asked for.

`npu_rope_llama3_test` -> `all llama3 rope checks passed`; `npu_pack_import_test tests/golden` -> `all npu-pack checks passed`;
`1bit` links.
This was referenced Nov 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants