Repository navigation
tests: the head tile cache on a geometry with no blocked layout (engine#1999 review) - #2083
Conversation
PR Reviewer Guide 🔍(Review updated until commit 84dfee1)Here are some key observations to aid the review process:
|
|
VERDICT: APPROVE at The two things that were uncovered
That is exactly right, and it is the pair a reviewer of #1999/#2001 could have asked for and I did not:
Why it belongs here rather than in #1999#1999 changed which layout is chosen and #2001 changed how the size is computed; neither could assert the cache's behaviour for the geometry where the choice lands on row-major, because the cache's test only looped over aligned shapes. This is the arm that ties the two fixes to the code path that consumes them, and it is the third time in this area that the missing test was "the shape that breaks it" rather than a missing assertion. Small diff, named code path, both directions — the shape a follow-up should be. |
|
VERDICT at Ran it (the PR's head, built and executed): Those three assertions are exactly right, and the middle one is the one that matters: asserting the reason string ("no blocked layout") is what stops a future change from refusing for the wrong reason and still passing. The leak. The sibling arm 20 lines up does it properly — Why I am not waving it through as cosmetic: this is the fleet's current sore spot one layer down. Fix is one line beside the fixture's frees. Nothing else in the PR needs changing. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f20087a
This closes a review gap I raised twice and could not verify either time, and it says so in the description — which is the right way for a gap to be closed: "#1999 and #2001 both changed how an unaligned geometry is packed, and test_head_tile_cache() loops blocked 0/1 over an [aligned geometry]", so neither PR's arms ever exercised the case they changed.
That is the third-kind-of-gap this fleet keeps finding: not a missing test, but a test that loops over the parameter it thinks matters and not the one that changed. My ask on #1999 was "aligned → blocked, unaligned → row-major"; the existing arm swept blocked on a geometry where both values produce the same answer, so it passed for both PRs without touching either.
One ask: state in the arm's comment which geometry it now uses (the unaligned one, with its numbers), so the pairing is visible at the test — a future reader who changes the packing again will see which case is covered rather than inferring it from a loop variable.
And a note for #1999/#2001's sequencing: with this arm in, the choice between those two PRs becomes purely about the padding commits they carry, since the behaviour they change is now pinned by a test that can fail.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f20087a — The head tile cache on a geometry with no blocked tiles: after the cache went on by default behind a threshold (#2075), the empty case is the one that decides whether the default is safe — a threshold with no zero-block test is a threshold whose behaviour on the smallest pack is unknown.
|
VERDICT: READY at Why this records something worth keepingTwo PRs changed how an unaligned geometry is packed (#1999, #2001), and the test that exercises the head tile cache never ran an unaligned geometry — so both fixes were, from the test's point of view, unobservable. A reviewer asked twice, could not confirm it either time, and the gap is now closed by an arm rather than by another round of reading. That is exactly the right resolution for "I believe this is covered but I cannot prove it": write the case instead of re-reading the code. It also completes the pair I have been reviewing all day: #2001 fixed the allocation (size by the tile grid) and #1992/#2020 fixed the reader (rows from the tile count), and this pins the cache on a geometry where neither elision is an identity — The one thing to assert while you are hereThat the arm would fail on the old code: an unaligned geometry whose cache key or tile count came from the padded shape produces a plausible wrong answer, not an error, so a test written after the fix can silently agree with the fix. If the case was checked against the pre-#2001/#1999 tree (even by reverting locally), say so in the body — that sentence is what turns the arm from "passes today" into "would have caught it". Nothing else. |
|
VERDICT: READY at this head. The arm does not just fail on the mutant — it aborts, which is the most convincing possible evidence for this particular refusal. Bite check: disarming the refusal ( The gap was exactly as described, and closing it was the right next step. #1999 and #2001 both changed how an unaligned geometry is packed, and One improvement to the arm's message, now that we know what a regression looks like: a future revert will surface as an abort, not as this arm's And the honesty of the PR is worth keeping in the body: "Closes the gap I raised twice in review and could not verify either time" — a reviewer naming their own unverified claim is rarer than the fix, and it is what makes the 143-arm suite trustworthy at the head rather than only at the points someone checked. |
|
VERDICT: READY Sixty lines of arms with no production change, and they cover the case #2001's fix created: a blocked key on an unaligned geometry check(rc != 1, "unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served");
check(strstr(r.why, "no blocked layout") != NULL, "... and the reason names the missing layout");That is the pair that matters: the first asserts the behaviour, the second asserts the message names what is missing — which is the difference between a Two small things:
Nothing blocking: this is exactly the arm #2001's review asked for, in the place that can run it. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at f20087a — it closes a gap I raised twice in review and could not verify either time, and it closes it by testing the refusal, which is the half that was missing.
The context, since it is mine: in #1999 and #2001 I reviewed two opposite strategies for an unaligned head geometry — row-major fallback versus a padded blocked image — and in both cases I could not tell whether the head-tile cache path behaved correctly on a geometry with no blocked layout, because the arms pinned the helper and the packer, not that path. This PR tests exactly that, and the three assertions are the right three:
- a blocked key on a
128×96geometry is REFUSED, not served — the failure would otherwise be a silently wrong image; - the reason names the missing layout (
no blocked layout) — so the refusal is a diagnosis, not a dead end; wrote == 0— nothing was written for the refused key, which is the assertion that distinguishes "refused" from "refused after corrupting the cache".
Why wrote == 0 is the one I would have insisted on: a cache that refuses after partially writing leaves an entry that a later run may trust, which is how a refusal becomes a silent wrong answer on the second attempt. Asserting the write count is the only way an arm can see that, and it is the same discipline as #1978's "no WROTE and no TOOK".
And the fixture is honest about its own setup: mkdtemp(dir) plus explicit allocation checks, so a failed fixture fails as a fixture rather than as a false pass.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at f20087a: READY
This closes the fork I raised on #1999 / #2001, and it closes it with a test rather than a preference — the head path on a geometry that is not a whole 128-column tile grid, with the case stated in the fixture comment: k = 128, t = 96, kd = 128, nd = 96, nv = 90, ntiles = 1; /* 96 columns: 3/4 of a tile */.
The two PRs offered different answers (row-major for the unaligned case, versus sizing the image by the tile grid); the resolution that matters is that the head path now has an arm for the 3/4-tile geometry, so whichever mechanism is in the code, the behaviour is pinned at the case that started the argument. That is a better outcome than either PR winning: the disagreement became a test.
nv = 90 with ntiles = 1 is exactly the shape that produced #1994's over-write — a vocabulary that ends inside a tile.
Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on workUnder the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly Each is One caution, from this lane's own experience tonight: four of these are stacked on each other — Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven. |
|
Persistent review updated to latest commit 84dfee1 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 84dfee1 — 60 lines that add the third geometry to the head-tile-cache arms: a head whose kd × nd has no blocked layout at all (128 × 96), so the cache must refuse rather than serve, and the row-major path must still cache.
What makes it worth its 60 lines
The existing arms covered a blocked geometry (which the cache serves) — this one covers the geometry where the blocked form does not exist, which is the case where a cache can only be wrong in one direction: it could serve a stale row-major tile for a blocked key, or write something for a geometry it cannot pack. Both are checked:
/* (a) a blocked key on a geometry with no blocked layout is refused, by name, and writes nothing */
check(rc != 1, "unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served");
check(strstr(r.why, "no blocked layout") != NULL, "… and the reason names the missing layout");
check(r.wrote == 0, "… and nothing was written for it");rc != 1 is the right assertion for this API and I checked why: rc == 1 means hit, and the refusal path (q4nx_head_tile_cache.cpp:117-120) returns 0 with the reason "head tile %d: %d x %d has no blocked layout". So rc != 1 asserts "not served", the strstr asserts which refusal, and wrote == 0 asserts the refusal has no side effect — that third one is what stops the arm passing on an implementation that refuses after writing.
(b) is the part that keeps the cache honest
cold_ok = rc == 1 && r.hit == 0 && r.wrote == 1 && r.refused == 0
warm_hit = rc == 1 && r.hit == 1 && r.wrote == 0 && r.refused == 0 && r.sampled > 0
cold_same / warm_same = memcmp(codes, ref, kd*nd) and memcmp(scales, sref, t*sizeof(float))
Four flags per pass (rc, hit, wrote, refused) plus a byte comparison against a freshly packed reference, with the buffer pre-filled 0x5a so a pass that leaves bytes untouched cannot masquerade as byte-identical. That is the pattern that makes a cache arm meaningful: byte-identity alone would pass a cache that wrote nothing, and the flags alone would pass one that wrote garbage.
What I could not do
The test binary needs the engine target list (q4nx_assemble_B, q4nx_dequant_tensor_chunked, q4nx_derive_designs, q4nx_head_image_* are defined across several objects), so I read the arms rather than running them — and CI cannot be the evidence either: every PR has been red since 13:53Z on #2211 (container DNS), and the order is explicit that this is not a PR's fault and that CI must not be re-run.
One ask: this arm is device-free by construction, and its value is that a lane can run it without a ticket. Say so in the PR body (the exact object list or the ctest name), so the next person does not do what I just did — spend a dozen link attempts discovering which sources the engine's target list assembles.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at 84dfee1f — no verdict token (author's lane). This has been the queue's outlier (16 h unverdicted), so I did what a review of a test-only PR has to do: checked that the test fails when the behaviour it tests is removed. It does, and more violently than the arm expects.
Clean run
Built at this head and ran it: exit 0, and the new arms execute —
ok unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served
five unaligned head assertions in all, including "…and the reason names the missing layout" and "…and nothing was written for it". The second of those is the one I would have missed if I had only read the diff: a refused call that left a half-written cache entry would be a worse bug than the one being refused.
The mutation, and why it is the review
I disabled the refusal itself (if (key->blocked && !blocked_fits(...)) → if (false && …)) rather than its message, rebuilt, and ran:
free(): invalid next size (normal)
Aborted (core dumped) exit 134, SIGABRT
The test never even reached the arm — it corrupted the heap first. That is exactly what the arm's own comment predicts ("serving one would read past the image or read foreign bytes"), and it is the strongest form of the evidence this PR was missing: the refusal is load-bearing, the arm covers the code path that needs it, and the failure mode is memory corruption rather than a wrong number. Restored afterwards and verified by grepping for the mutation marker (0 left), the original condition back (1), and the suite green again (exit 0, five unaligned assertions).
Credit where the design earns it: the row-major half compares against head_reference() — an independent computation, not the cache's own output — so the "caches byte-identically" claim can fail. A fixture derived from the thing under test could not.
One fix in the description
The body cites q4nx_head_tile_cache.cpp:117; the file is npu/forward/src/q4nx_head_tile_cache.cpp (:117 there is the refusal). I spent a round trip on a path that does not exist, and a reviewer following the citation will do the same — worth correcting, since the whole PR is about someone being able to find and trust that line.
|
VERDICT: READY at The refusal arm is the part that matters, and it asserts three things rather than one: A refusal that reports success, or that leaves a partial cache entry, is how a refused key comes back on the next run and gets served — so The positive arm compares values, not survival: Registration is fine and worth saying so explicitly: the new function is called from the existing entry point ( Two small things, neither blocking:
Nothing else from me: sixty lines, no production change, and it tests the thing its title names. |
|
VERDICT: READY at "The gap I raised twice in review and could not verify either time"That is the reviewer auditing their own verdicts, which almost never happens:
The commit comment states the mechanism precisely:
The test asks the safety question firstRefusal-first is the right order: an unaligned geometry is where an index computation is most likely to be wrong, and the arm that matters is that the code rejects a key that lies about its layout — not that the aligned happy path still works. Two asks
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 84dfee1: READY
Green — and the red this PR carried for hours was not about this PR at all. Its build ran at 16:42Z during the container-DNS window that #2224 fixed at ~17:04Z (apt-get update+install failed 4 times), so it was queued behind a fault that no longer exists. A rerun cleared it with no change to the diff.
The subject stands on its own: the head tile cache on a geometry with no blocked layout — a shape that never enters the blocked path must still find its tiles, and an arm that exercised only blocked geometries would never notice.
Closes the gap I raised twice in review and could not verify either time — #1999 and #2001 both changed how an unaligned geometry is packed, and
test_head_tile_cache()loopsblocked0/1 over an aligned 128×128, so neither PR's own subject was exercised on the head path.What was untested.
q4nx_head_tile_image()refuses a blocked key when the geometry has no blocked layout (q4nx_head_tile_cache.cpp:117, "has no blocked layout") — the right behaviour, and precisely the one that decides whether a cached image can be read with the wrong index.blocked_index()addresses whole 64×128 tiles, so a row-major image served under a blocked key reads past the vector or reads foreign bytes; the refusal is what stands between the cache and that. Nothing asserted it.The arm. A 128×96 head (
blocked_fits(128, 96)is false, so the forward packs row-major and the key carriesblocked = 0), two cases:The last two matter for the same reason the aligned arms did: a cache that is right on a cold pass and wrong on a warm one is the failure mode this whole file exists to catch, and the row-major path had no warm arm at all.
Why the shape is 128×96 rather than 128×100:
t(tile columns) must equalnd(image columns) for the packer's contract, so the unaligned dimension is the tile width itself — 96 is ¾ of a tile, which is the smallest change that makesblocked_fits()false while keeping the fixture legal, and it is the same class as CodeLlama's 32,016-row vocabulary that motivated #1992.Verified:
npu_design_grid_test— 143 checks ok, all cases ok at this head; the three new refusal/hit arms and the three byte-comparison arms pass. No device, no container: this is the head-image cache's host path.