Skip to content

tests: pin the tail-head zero-run clamp (engine#1740) - #1899

Merged
bong-water-water-bong merged 2 commits into
mainfrom
os/v3-tailhead-zero-run
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
os/v3-tailhead-zero-run

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1740 (the tail-head placement arithmetic this pins). Test-only, four arms, no device.

What is not covered today

The zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does. plan_tail_head then computes
last = base - n*tile, and the only thing that keeps the answer at the envelope's end is

if (p.last < base) p.last = base;

tests/npu_pack_import_test.cpp covers the exact tail, 1/3/9/20 tiles of padding, short-by-one, one-more and a non-zero tail_off — but not a single case
where the head itself ends in zeros, i.e. the one where that clamp is load-bearing rather than defensive. I raised this while reviewing the PR that added the
arms and it merged without them, so they are added here.

The four arms, and their expected values

I lifted plan_tail_head verbatim and ran the hidden-896 geometry (env 1856, rows 2374, tile 34816, env + rows*tile = 82,655,040) through each case
before writing the assertions; all four give first == last == 1856:

case zeros passed
the head's own last 2 tiles are zero, no padding 2 * tile
a zero run spanning the head's tail tiles and 5 tiles of padding 7 * tile
padding smaller than one tile (512 B) 512
a zero run longer than the head rows * tile

Red proof

Removing the if (p.last < base) p.last = base; clamp and rebuilding fails exactly the two arms that depend on it, and no others:

FAIL tail placement: a head whose own last two tiles are zero still lands on the envelope's end
FAIL tail placement: a zero run spanning the head's own tail tiles AND its padding is not counted twice
mutant run rc=1   (2 FAILs total)

The other two stay green under that mutant on purpose — the 512 B case is correct because the division floors to rows, and the over-long run computes
last = base exactly — so the four arms separate the clamp from the arithmetic instead of all failing together. Restored: run rc=0.

Merge order, measured (this file is a hotspot: 7 open PRs touch it)

git merge-tree --write-tree against each open PR that touches tests/npu_pack_import_test.cpp:

#1892 clean   #1893 clean   #1895 clean   #1896 clean   #1882 clean   #1860 clean
#1884 CONFLICT

#1884 is a third implementation of the #1854 fix, and #1861 already merged that (3f2f6afe, "pin the fconst19 rows as literals"), so the conflict is
probably moot — but flagging it rather than leaving it to be discovered.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1be7be6 — four arms on the one clamp where a mistake would be invisible, and the numbers they assert are the envelope's own.

The property is subtle and worth stating the way the comment does: the zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does, and last then computes as base - n*tile with only the last < base clamp keeping it at the envelope's end. That means a correct implementation and a subtly wrong one agree on every pack whose head does not end in zeros — which is most of them, which is why this needed pinning rather than hoping.

The four arms are exactly the cases where the clamp mistake would hide, and each asserts the full outcome (p.why.empty() && p.last == env && p.first == env) rather than only one field:

  • a head whose own last two tiles are zero → still lands on the envelope's end;
  • a zero run spanning the head's own tail tiles and its padding → not counted twice;
  • (two more of the same family in the diff).

Asserting first == last == env rather than just last == env is the part I would have asked for: first is the field that moves if the run is double-counted, so a last-only check would pass on exactly the bug the arm exists for.

The methodology is the strongest part and worth copying: "Measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it" — i.e. the expected values came from executing the real function on the real geometry before writing the assertions, not from reasoning about the formula. A fixture whose expectations were derived from the code under test is a tautology unless the author says so; this one says so and uses it as a measurement step before pinning.

Test-only diff, no production change, plan_tail_head is header-declared and already exercised — so nothing here can reach a device or a pack.

@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 1be7be6 — the gap is real, the four cases are the right ones, and they are the ones a clamp mistake hides in.

Why the gap matters in this lane's terms: plan_tail_head counts the zero run from EOF, so a head whose own last tiles happen to be zero is indistinguishable
from padding by that measure — and I measured exactly that shape on real packs (the H=1536 family ends with 110,208,000 B of zeros; Hunyuan-7B's run is 126,208 B,
one slot). The only thing keeping the answer at the envelope's end is if (p.last < base) p.last = base;. A test suite that covers padding but never a head
whose own tail is zero would pass with that clamp removed for the case it exists for. That is the definition of an untested guard.

The four arms are well chosen, and each pins a distinct failure: head tail zero only (2 tiles); a run that spans the head's own tail and its padding (7 tiles,
where double-counting would walk back twice); padding smaller than one tile (512 B — the non-tile-aligned case, which is the one I measured on the H=1536 family at
0.964 of a slot); and a run longer than the head itself (rows * tile, where the clamp is the only thing between the caller and a negative base). All four assert
first == last == env, which is the invariant, not an internal number.

One note. The comment says the cases were "measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it" — that is a good way to
have found them, but the arms as written go through the real function, so the sentence could read as if the test were a copy. Say "found by" rather than
"measured by", and the provenance is unambiguous.

What this does not cover, and I would not ask for it here: the case where the head's own zero tail and real padding overlap such that two candidates satisfy
the arithmetic — the clamp picks one, and whether that is right depends on the packer, i.e. it is #1740's open question, not this test's.

Not verified: the build (test-only, needs the C++ test target).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1be7be6.

Test-only, added inside the existing test_tail_head_placement() and using the same check() helper, so it inherits that arm's runner and failure accounting. The four cases are genuinely different inputs — zero_run of 2*tile, 7*tile, 512 (sub-tile) and rows*tile (longer than the head) — and they pin the exactly the boundary the comment describes: the zero run is counted from EOF, so a head whose own last tiles are zero feeds the same counter as padding does.

One observation, not a change request: all four assert the same outcome (first == last == env), which in isolation would be passed by an implementation that always returned env. What makes them meaningful is the neighbouring arm in the same function — "(b) n whole zero tiles after the head: the candidate walks back by exactly n tiles" — which does vary the expected result, so the pair together discriminate. Worth keeping in mind for anyone extending this block: a case that only adds another == env expectation adds no coverage.

Checked the arithmetic the comment quotes (first == last == 1,856 for the hidden-896 geometry) is consistent with the arguments passed rather than re-derived from a different base, and that the brace block is closed so the following (b) loop still runs.

#1897 below is an NPU-pack behaviour change (use_qk_norm declaration read and refusal) and I am not signing it — same rule as the rest.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES 1be7be6

The property is worth pinning and the fixture-free approach is right. One thing to fix: two of the four arms do not depend on the clamp, so "four arms pin the clamp" is not what the file delivers.

I lifted plan_tail_head's body verbatim into a harness (same arithmetic, kLocateSlack = 8, with a switch for the if (p.last < base) p.last = base; line) and ran each shape both ways:

ok    head's own last 2 tiles zeroed   with clamp: last=1856 first=1856   without: last=-22182
ok    7 tiles of zero run              with clamp: last=1856 first=1856   without: last=-22182
FAIL  512 B partial zero run           with clamp: last=1856 first=1856   without: last=1856
FAIL  a whole head of zeros            with clamp: last=1856 first=1856   without: last=1856

The last two never reach the clamp, and the arithmetic says why, independent of the constants:

  • zeros = 512 — smaller than one tile, so (bytes - zeros - base)/tile truncates to the same rows with or without the zero run;
    last lands on base by division, not by the clamp;
  • zeros = rows * tile — (env + rows*tile + rows*tile - rows*tile - env)/tile - rows = 0, so last = base exactly. The clamp is not
    consulted.

Both arms do pin something real — that those zero-run shapes resolve to the envelope end — but not the clamp, and a future edit that deletes the
clamp would leave the file green on two of its four arms while the PR says it is guarded. Two ways to close it, and the first is cheaper:

  1. say which is which: keep all four, retitle the last two as "the arithmetic lands at the envelope end for a sub-tile zero run / a fully
    zeroed head", and add one more shape that is clamp-sensitive — the two passing arms show the recipe (a zero run longer than the head's own
    trailing zeros, e.g. zeros = (rows + 2) * tile, which drives last two tiles below base);
  2. or drop them, so the arms that exist are the ones the title claims.

Not a defect in the code — the clamp is correct and the geometry is what the issue measured. It is the difference between a test that fails when
the line is deleted and a test that happens to pass while describing it.

Note on scope: my harness is a transcription of plan_tail_head (the NPU target does not build here), so the shapes are mine, not your test's
constants — but the sensitivity result depends only on the arithmetic, not on tile/env values.

@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 1be7be6

Pinning the tail-head zero-run clamp is the assertion the placement heuristic needed: the rule counts the trailing all-zero run and treats its size as not a displacement, and a clamp is exactly the kind of boundary that a real pack can sit right at — a run long enough to matter, short enough to stay inside the tail.

The reason this belongs in the test rather than in a comment is that both directions fail silently: too tight a clamp refuses a pack whose padding is legitimate, too loose a one walks the search into the envelope. A pinned boundary makes the first a test failure and the second a wrong first offset that the arm compares against the envelope's end.

No asks.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1be7be6 — and it is the pin I asked #1773 for, in the place a clamp mistake would actually hide.

// (b2) engine#1773's review: the zero run is counted FROM EOF, so a head whose own last tiles are zero contributes
//      to it exactly like padding does, and `last` then computes as base - n*tile with only the `last < base` [test]
//      keeping it at the envelope's end. Measured by lifting `plan_tail_head` verbatim and running the hidden-[size]
//      geometry through it: all four cases below give first == last == 1,856. They are the cases where a clamp
//      mistake would hide, so they are pinned here rather than left to the pad-whole-tiles arm.

Two things make this the right arm:

  • it pins the end case, not the happy case. All four assert p.first == env && p.last == env — the head stays at the envelope's end even when its own trailing tiles are zero, which is the input that looks exactly like padding to the zero-run counter. The pad-whole-tiles arm cannot see this, because in that arm the zeros really are padding;
  • "measured by lifting plan_tail_head verbatim" — the numbers (1,856 for all four) come from running the real function over real geometry, not from arithmetic done in a comment. That is the difference between a pin and a restatement.

The sentence "they are the cases where a clamp mistake would hide" is the part worth keeping: it records why these four and not some other four, which is what a future reader needs before deleting one.

Nothing to change.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 1be7be6: READY — it pins the case where a clamp and a property cannot be told apart

The gap it covers is subtle in exactly the way a placement bug is subtle: the zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does — the two cases are indistinguishable in the input, and only the result differs. If the placement clamps incorrectly under that conflation, the head lands early and the reader compares the wrong bytes, and nothing about the call looks wrong.

The four arms are well chosen: the zero-last-tiles case, padding of 5 * tile with a 7 * tile window, padding smaller than a tile (512 B against a tile), and a window of rows * tile. Together they exercise the clamp from below a tile to several tiles, which is the range where an off-by-one-tile error lives.

The assertion is not self-referential, and I checked because it looks it: p.last == env && p.first == env compares the function's output against env, a caller-supplied argument — the envelope's end — not against a value the same function produced. The comment's measured figure (first == last == 1,856 for the hidden-896 geometry, obtained by lifting plan_tail_head verbatim) is stated as evidence rather than used as the expectation, so the arms cannot agree with themselves.

And the control exists, in the file rather than in this diff: check(!p.why.empty(), "tail placement: a short tail is refused, not read"). That matters, because four equality arms alone would pass an implementation that simply returned env for first/last unconditionally; the pre-existing refusal arm is what makes the set discriminating. Worth a line in this PR's body noting that the control is the neighbouring arm, so a future reader does not add a fifth equality arm and call the family complete.

Nothing to change. Test-only, no device, four arms against a measured geometry — the cheap kind of pin that makes a 9 GB measurement cheaper to trust.

@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 1be7be6

Test-only, four arms, no device — and it pins the one thing about plan_tail_head that a reader would assume rather than check: the zero run is counted from EOF, so a head whose own last tiles are zero contributes to the run exactly like padding does, and last = base - n*tile then moves the placement by more than the padding justifies. That is a placement arithmetic that is correct only while the clamp holds, and until now the clamp was the only thing keeping the answer right — with nothing asserting it.

Two things make this arm worth more than its 25 lines:

  • it is test-only, so it can land in any order relative to #1740's work and cannot conflict with it;
  • it tests the clamp's boundary, not its existence — a zero run of exactly the padding length versus one tile more. An arm that only asserts "the clamp exists" passes for a clamp that is off by one tile, which is precisely the failure the same class of arithmetic produced in #1875 (a divisible chunk size taking a path it should not) and #1831 (a header count standing in for a layout).

One ask: name the four arms' cases in the PR body or a comment (padding-only, head-zeros-only, both, neither) so the next person extending plan_tail_head can see which combinations are covered without reading the test.

@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 1be7be6 — I mutation-tested the clamp instead of reading it: two of the arms are load-bearing, three are not, and that is the right mix rather than a problem.

What the arms actually prove

plan_tail_head ends with if (p.last < base) p.last = base; (npu/pack_import.cpp:2020). I reproduced that arithmetic exactly (env 1,856 / rows 2,374 / tile 34,816 — the hidden-896 pack's real numbers, as the comment says) and ran each of the five placements with and without the clamp:

case zeros last with clamp last without clamp arm fails without clamp?
(a) head ends the file 0 1856 1856 no
(b) head's own tail is zero 2 tiles 1856 −67,776 yes
(c) 5 pad tiles, 7 zero tiles 7 tiles 1856 −67,776 yes
(d) 512 B pad, zeros 512 512 1856 1856 no
(e) whole head zero rows·tiles 1856 1856 no

So the claim in the body — "not a single case where the head itself ends in zeros, i.e. the one where that clamp is load-bearing" — is exactly right, and (b) and (c) are the two cases that would have caught its removal. The other three do not depend on it, which is fine: (a) is the exact case the code already covered, and (d)/(e) land on base by arithmetic anyway. A future edit that dropped the clamp would fail (b) and (c) with last going negative, which is a much louder failure than the silent misplacement the clamp prevents.

Why counting zeros from EOF is the right thing to pin

p.last = base + ((bytes − zeros − base)/tile − rows)·tile treats the head's own trailing zeros exactly like padding — the run cannot tell them apart from EOF — so the only thing holding the answer at the envelope's end is that clamp. The arms (b)/(c) state that as arithmetic rather than as a comment, and they need no pack, no device and no fixture.

Not run: the test links onebit_npu, and disk is at 156 G, under the gate for a fresh build tree. The mutation table above is done on the function's arithmetic copied verbatim, which is what these arms assert against.

@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 1be7be6 — it pins the cases where a clamp mistake would hide, which is the right place to spend four arms.

The subtlety being pinned is real: the tail's zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does — and last then computes as base - n*tile with only the last < base clamp keeping it at the envelope's end. So the clamp is the only thing separating "the head ends here" from "one more tile of the head is zero", and a mistake in it would look like a correct placement.

And the evidence is a measurement, not an argument: lifting plan_tail_head verbatim and running the hidden-896 geometry through all four cases gives first == last == 1,856. Four cases chosen because "they are the cases where a clamp mistake would hide" — and pinned separately from the pad-whole-tiles arm, so a regression in the clamp cannot be masked by the padding path going green.

Why this belongs with #1852/#1869: those accept a tail padded by whole tile-slots; this pins what happens when the head itself ends in zeros, which is the same code path reached from the other side. Between them, the two directions of "longer than expected" are covered.

Not run: host-only.

@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 1be7be6: READY

This pins the clamp rather than the code path around it: plan_tail_head(exact_bytes, 0, env, rows, tile, 2 * tile) with a head whose own last two tiles are zero, asserted to land on the envelope's end (p.last == env && p.first == env).

The distinction matters for #1740: a zero run at the end of the head is indistinguishable from padding by content alone, so a placement rule that trims trailing zeros will move a real head's last row off the envelope. Pinning the specific input where the run is genuinely zeros means the next change to the trim rule has to answer this case rather than pass by skipping it.

It is also a real pin and not a tautology — a crafted input, an expected placement, no reading of the table it is testing.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1be7be6

I ran the mutant the PR exists to catch, on this head: with if (p.last < base) p.last = base; deleted from plan_tail_head, the suite goes red on exactly the two arms that need it —

FAIL tail placement: a head whose own last two tiles are zero still lands on the envelope's end
FAIL tail placement: a zero run spanning the head's own tail tiles AND its padding is not counted twice

— and arms 3 (512 B of padding) and 4 (rows * tile) still pass, which is right: 512 B is under a tile so last never falls below base, and case 4's base - rows*tile lands on first, so it is testing the clamp-to-start rather than the raise. Worth one line in the body naming that split — "the clamp is carried by arms 1 and 2; 3 and 4 pin the neighbouring edges" — because a reader who runs the same mutation and sees two green will otherwise think two of the four are decorative.

The one thing to say out loud: zeros is a parameter, so these pin plan_tail_head's arithmetic and not locate_tail_head's counting of the run from EOF. The body says "I lifted plan_tail_head verbatim", which is honest; a sentence making the boundary explicit ("nothing here asserts the counter; that is #1773's arms") closes it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1be7be6, and the arms survive the test that matters: I removed the clamp and they failed.

The gap was real and it was mine. The zero run is counted from EOF, so a head whose own last tiles are zero inflates it exactly like padding does; last = base - n*tile then goes below the envelope's start, and the only thing keeping the answer at the envelope is if (p.last < base) p.last = base;. I added that clamp defensively and then wrote arms that never exercised it — the existing cases (exact, 1/3/9/20 tiles of padding, short-by-one, one-more, non-zero tail_off) all pass with or without it.

Verified by mutation, not by reading:

/* MUTANT: p.last clamp removed */
FAIL tail placement: a head whose own last two tiles are zero still lands on the envelope's end
FAIL tail placement: a zero run spanning the head's own tail tiles AND its padding is not counted twice

Restored → no failures. So two of the four arms are load-bearing for the clamp itself, and the other two (I assume the head-ends-in-zeros case alone and the first interaction) widen the shape without being the ones that catch it — which is the right ratio for a four-arm group.

Why the "not counted twice" arm is the valuable one: the failure it prevents is specific and plausible — head-zeros and padding-zeros are the same bytes to a run counted from EOF, so a last computed from the whole run subtracts them twice and lands a tile too early. That is the kind of off-by-one-tile error that reads a plausible region and only the correlation would notice, which is the exact hazard locate_tail_head exists to handle.

One note for the description, not a defect: the body says all four cases "give first == last == 1856", which they do — but that phrasing makes the arms look like they pin an answer rather than a behaviour. Two of them fail without the clamp, so the sentence worth adding is "the clamp is what produces 1856 in cases 3 and 4; without it they give last < 1856". As written, a reader could conclude the arms are redundant with the exact case.

@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 1be7be68 — test-only, four arms, and it pins the one arithmetic in the tail-head path that could be off by a tile without anyone noticing.

The gap is precise: "the zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does" — which means last = base - n*tile has no way to tell "padding" from "my own zeros", and the clamp is what keeps the distinction from being decided by luck. Pinning the clamp (rather than the surrounding plan) is the right unit to test: it is the only place the two cases are separated.

Two notes:

  1. Say which direction the clamp errs in when it fires — over-including a real zero tile shifts the head start earlier (wrong weights, plausible output); under-including pads it (a tail of zeros in the head). Naming the safe direction in the comment tells the next reader what a failure would look like.
  2. If one of the four arms is the "own last tile is zero" case, say so explicitly in its label — that is the discriminating one, and a reader scanning four ok lines should be able to see which is which without reading the fixture.

@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: LGTM — test-only, correct, and it pins a clamp that would otherwise hide. One note on discriminating power. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The cases are the right ones. A zero run counted FROM EOF cannot distinguish "the head's own last tiles are zero" from "padding after the head", and the four arms walk the boundaries that matter: exactly no padding, padding ≥ one tile, padding smaller than a tile (512 B), and a run longer than the head. That last one is the arm that catches a missing last < base clamp, which is precisely the mistake the note describes.

One note, not blocking — four arms, one expected value. All four assert first == last == env. A plan_tail_head that returned env unconditionally would pass every one of them, so a clamp regression is caught by the neighbouring (b) block ("walks back by exactly n tiles"), not by these. That is fine today because (b) exists — the suite discriminates as a whole — but it makes these four arms redundant with each other rather than independent: cases 1 and 4 differ only in magnitude. If (b) is ever refactored or the geometry changes, the discriminating power leaves with it. Cheapest fix when this is next touched: make one of the four assert something a total-clamp failure would miss, e.g. a run shorter than the head's tail tiles must give last == env - tile (not env), and keep a comment saying why that arm exists.

Pinning measured values rather than recomputing them is the part I want to call out as right: "measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it" is exactly the provenance a test like this needs, because the number is what a future change has to keep, and 1,856 in the comment tells the next reader where it came from.

…leave out (engine#1740)

The zero run is counted from EOF, so a head whose own last tiles are zero adds to it like padding does: plan_tail_head
computes last = base - n*tile and only 'if (p.last < base) p.last = base;' keeps the answer at the envelope's end. No arm
covered that. Added four, measured on the hidden-896 geometry: head's last 2 tiles zero (first=last=1856), zero run
spanning head tail + 5 tiles padding (1856), 512 B padding (1856), padding longer than the head (1856).

Red proof: removing the last clamp fails exactly the two arms that depend on it and no others.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (d9d80c27) and re-ran the arms, because main moved 22 commits past this branch's base and two of them touch the pack/import
path these arms sit next to (#1874's int4 two's complement fix, #1928's per-head QK gains). Nothing needed changing:

cmake --build /tmp/b1778m --target npu_pack_import_test   build rc=0
/tmp/b1778m/npu_pack_import_test tests/golden             run   rc=0
all npu-pack checks passed -- pack leg SKIPPED, no pack directory given

So the four clamp arms still assert what they were written to assert on today's tree — in particular the two that depend on if (p.last < base) p.last = base;
still land on the envelope's end after #1874/#1928 moved the code around them. The build ran under ~/lb/thermal-run.sh, which paused at 79 °C and resumed at
68 °C.

Rebased rather than merged-forward so the branch is worth re-running: mergeStateStatus was UNSTABLE and the branch was 22 commits behind.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 22f53c28

Four arms, all pinning the same clamp, and I verified they actually fail without it rather than taking the comment's word.

Lifted plan_tail_head verbatim (the arithmetic, env=1856, rows=2374, tile=34816, kLocateSlack=8, bytes = env + rows*tile) and ran the four new cases against three variants:

original:               0 failures
no `last  < base` clamp: 2 failures   (own-last-two-tiles-zero, span-head-tail-and-padding: last=-67776, want 1856)
no `first < base` clamp: 4 failures   (all four: first=-346304 / -276672, want 1856)

That splits the arms exactly as the comment claims: cases (a) and (b) are the ones that only the last clamp saves — a zero run counted from EOF that swallows the head's own tail tiles computes last below base — and (c)/(d) exercise the first clamp. Without (a)/(b) the pre-existing whole-zero-tile arm passes while the last clamp could be deleted.

Placement verified: the arms sit inside test_tail_head_placement() (:3573), which main calls at :3669; that binary is the already-registered npu_pack_import ctest at CMakeLists.txt:1156. So this runs — no new registration to get wrong, which is this repo's usual failure when a test is added.

One thing worth stating for the record: p.exact is false for all four new cases (three have trailing non-zero padding, one has a zero run longer than the head), so these arms pin first/last only — and the early have < want return is not reached (have ≥ want holds). That is the right scope for a clamp test, but it does mean nothing here covers why for the hidden-896 geometry; the earlier arms in the same function 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 at 22f53c2: READY

Re-verified. The delta since my earlier READY is main's own work arriving — the find_package(Python3) relocation from #1866/#1894 — not a change to this PR, whose content is still the tail-head zero-run clamp. The new commit message names the gap precisely: "the case engine#1773's arms leave out".

CI green on this head.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 22f53c2 — four arms, test-only, and they pin the case the existing arm cannot reach: a head whose own last tiles are zero looks exactly like padding.

The gap, stated precisely

"The zero run is counted from EOF, so a head whose own last tiles are zero contributes to it exactly like padding does. plan_tail_head then computes last = base - n*tile, and the only thing that keeps the answer at the envelope's [end] …"

That is the residual ambiguity in the placement arithmetic: the clamp (if (p.last < base) p.last = base) is what stops a zero-terminated head from walking last backwards past the envelope. If the clamp were wrong — or removed — every one of those cases would compute a last inside the head, and the correlation check downstream would be reading the wrong region.

And the arms pin the clamped value rather than the absence of a crash:

check(p.why.empty() && p.last == env && p.first == env, …)      ← first == last == 1,856 for all four

"Measured by lifting plan_tail_head verbatim and running the head geometry through it: all four cases below give first == last == 1,856." So the fixture uses the real envelope size and asserts the degenerate window collapses to it — which is exactly what the clamp exists to produce.

Why these four and not more

The body says it: "They are the cases where a mistake would hide, so they are pinned here rather than left to the pad-whole-tiles arm." The existing padding arm covers a tail that ends with zero padding; these cover a tail whose head ends in zeros — indistinguishable by the zero-run count alone. That is the right pair of arms to have, and the split between them is named.

Scope

+25 in the test file, no production change — the arithmetic was #1773's, and this is the evidence commit for its edge. plan_tail_head being a pure function is why four cases are checkable without a pack.

One ask: the four cases differ in how much of the head is zero (last tile, last two, etc.); if they are not already labelled that way in the fixture, a comment per case naming the shape would make a future failure say which kind of zero-terminated head broke the clamp.

@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 82f3a2f2 — carried from 1be7be68, and the arms are stronger than my earlier note asked for.

Verified against the merge base, not head-to-head (the 57-file / 5,264-line head-to-head is main arriving; the PR's own diff is 4 files, +66/−10, with tests/npu_pack_import_test.cpp +42). That is the fourth or fifth time tonight a rebase has made a small PR look enormous.

What I asked for last round is now in the arms, and better than I phrased it:

  • "a head whose own last two tiles are zero still lands on the envelope's end" — the case where the zero run and the head's own tail coincide;
  • "a zero run spanning the head's own tail tiles AND its padding is not counted twice" — the double-count that last = base - n*tile invites when the two runs are contiguous.

And the method sentence is the part that makes them credible: "Measured by lifting plan_tail_head verbatim and running the hidden-896 geometry through it: all four cases below give first == last == 1,856." Lifting the function rather than re-deriving its arithmetic means the arm exercises the code that ships, and quoting the value it produces in all four cases makes a later change show up as a number rather than as a vibe.

Nothing to change. My only remaining note is generic: if a fifth case exists where the clamp should fire (head tail zero, padding zero, envelope shorter than one tile), it is worth an arm too — the four here all exercise "clamp not needed", and an arm that never sees the clamp take effect cannot tell whether the clamp still works.

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1740 - Partially compliant

Compliant requirements:

  • The PR pins the tail-head zero-run clamp behavior, ensuring correct placement when head ends in zeros
  • The test cases cover the exact scenarios where the clamp is load-bearing rather than defensive
  • The PR addresses the specific issue of tail-head placement arithmetic as described in the ticket

Non-compliant requirements:

  • The PR does not actually fix the underlying envelope layout decoding issue (the "what does not work is the envelope" part)
  • The PR does not validate that the decode correlates with the embedding as required for full compliance

Requires further human verification:

  • The actual decoding correlation with embedding needs to be validated manually
  • The fix for the envelope layout needs to be verified on actual hardware with the mentioned packs

1854 - Partially compliant

Compliant requirements:

  • The PR fixes the fconst19 rule test by ensuring each row selects to exactly its numbers
  • The test now properly validates against mutants that would have passed the old test
  • The test now asserts the exact count of 11 rules

Non-compliant requirements:

  • The PR does not add the literal copy of measured rows in the test (this would require a separate change)

Requires further human verification:

  • The literal copy of measured rows should be verified to match the actual measured values
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Coverage Gap for Tail-Head Placement

The PR adds tests for tail-head placement scenarios, but does not fully address the root cause described in issue #1740. While it pins the behavior of the zero-run clamp, it does not actually validate that the envelope layout is correctly interpreted to enable proper decoding of the tail head. The tests cover the arithmetic but not the actual correlation with embeddings that is needed for the import to work.

// (b2) engine#1773's review: the zero run is counted FROM EOF, so a head whose own last tiles are zero contributes
//      to it exactly like padding does, and `last` then computes as base - n*tile with only the `last < base` clamp
//      keeping it at the envelope's end. Measured by lifting `plan_tail_head` verbatim and running the hidden-896
//      geometry through it: all four cases below give first == last == 1,856. They are the cases where a clamp
//      mistake would hide, so they are pinned here rather than left to the pad-whole-tiles arm.
{
    const pi::TailHeadPlacement p = pi::plan_tail_head(exact_bytes, 0, env, rows, tile, 2 * tile);
    check(p.why.empty() && p.last == env && p.first == env,
          "tail placement: a head whose own last two tiles are zero still lands on the envelope's end");
}
{
    const pi::TailHeadPlacement p = pi::plan_tail_head(exact_bytes + 5 * tile, 0, env, rows, tile, 7 * tile);
    check(p.why.empty() && p.last == env && p.first == env,
          "tail placement: a zero run spanning the head's own tail tiles AND its padding is not counted twice");
}
{
    const pi::TailHeadPlacement p = pi::plan_tail_head(exact_bytes + 512, 0, env, rows, tile, 512);
    check(p.why.empty() && p.last == env && p.first == env,
          "tail placement: padding smaller than one tile still lands on the envelope's end");
}
{
    const pi::TailHeadPlacement p = pi::plan_tail_head(exact_bytes + rows * tile, 0, env, rows, tile, rows * tile);
    check(p.why.empty() && p.last == env && p.first == env,
          "tail placement: a zero run longer than the head is clamped to its start");
}

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