[bong-water-water-bong]

Fixes #1994

The bug. host_gemm_i8 (body GEMMs) and the =i8 LM head pack unpadded int8 images, K*N bytes, with blocked=1, and read them back with blocked_index(). That index addresses whole 64×128 tiles, so on a geometry that is not whole tiles, both the packer and the MAC run past the vector. The result is heap corruption plus logits read from foreign bytes, in the arm that is meant to be the reference a kernel family is gated against.

The fix.

• Both sites pick blocked = q4nx_layout::blocked_fits(K, N), record it in HostI8Tensor::blocked, and read through the new q4nx_layout::image_index().
• Row-major holds the same codes and the same per-column scales, so the int32 sums are identical.
• Every whole-tile geometry keeps the blocked fast path, including the measured 11008 body and the 2688 head block. Only an unaligned shape pays the slower row-major pack.

Measured (tests/npu_blocked_layout_test.cpp, new test_unpadded_image: pack into K*N plus 256 KiB of guard bytes):

K × N	why	layout now	furthest byte forced-blocked writes	image size
3072 × 7488	phi-3-mini's last vocab block (32064 % 8192)	row-major	23,199,232	23,003,136
2048 × 8512	body width of 66.5 tiles	row-major	17,563,136	17,432,576
100 × 128	K not whole 64-row tiles	row-major	13,280	12,800
4096 × 2688	measured head block, 21 tiles	blocked	11,010,048	11,010,048

On each shape, the test checks three things: the guard is intact, every code reads back at image_index inside K*N and equals the row-major reference packer's code, and (on unaligned shapes) forced blocked_index really leaves the buffer. With the fix: npu_blocked_layout_test: OK.

Mutation: the test with blocked = true (the arm before this PR) fails all three checks on each of the three unaligned shapes.

Also:

• onebit_npu_forward_syntax (compiles q4nx_forward.cpp) builds clean.
• npu_pack_test and ctest npu_blocked_layout pass.
• The other blocked_index callers (npu_engine_i8ctx_inc.h, q4nx_head_tile_cache.cpp) index padded device images and are unchanged.
• docs/npu.md (the NPU_INFER_HOST_GEMM=i8 section) now says the arm reads through image_index() and states the row-major rule.

Not covered: the test pins the helper and the packer, not the forward's two call sites (they need a loaded model). Both sites use the same blocked_fits(K, N) expression the test asserts.

🤖 Generated with Claude Code

[bong-water-water-bong]

VERDICT: READY at a579da5 — an out-of-bounds write in the reference arm, caught by guard bytes rather than by reasoning, and the test asserts the selection rule instead of only the crash.

This is the worst place for this bug and the PR says why: host_gemm_i8 and the =i8 LM head pack are the reference a kernel family is gated against. Both packed unpadded K*N bytes with blocked=1 and read them back through blocked_index(), which addresses whole 64×128 tiles — so on a geometry that is not whole tiles, the packer and the MAC ran past the vector: heap corruption plus logits read from foreign bytes. A wrong reference is worse than a wrong implementation, because every gate that trusts it inherits the error silently.

The arms are built to fail loudly in the way the bug actually manifests:

const size_t guard = size_t(1) << 18, img = (size_t)K * N;
std::vector<int8_t> buf(img + guard, int8_t(0x5A), rowmaj(img));
check(blocked == expect_blocked, "blocked only when K x N is whole tiles");
check(rc == 0 && guard_ok, "the packer writes nothing past K*N");
check(past == 0 && mismatched == 0, "every code reads back at image_index, inside K*N");
if (!expect_blocked) check(forced_max >= img, "forced blocked_index would run past K*N");

Four decisions in that block are the ones I would have asked for:

1. A quarter-megabyte of 0x5A guard bytes behind the image — the write-past-K*N is detected by memory that was filled with a known pattern, not by a bounds computation that could share the bug's assumption;
2. blocked == expect_blocked is asserted as a rule, not as an outcome for one shape, so the test pins "blocked only when K×N is whole tiles" rather than "shape X works now";
3. the read-back arm requires every code at image_index, inside K*N — i.e. it checks the addressing agreement between packer and MAC, which is the second half of the bug (logits from foreign bytes);
4. forced_max >= img documents that the old path would still overflow if anyone forces it — so the guard is not just removed, it is characterised.

What this does not cover, and the PR is honest about it: the fix is "pick blocked only when the image is whole tiles", so an actually-unaligned geometry now falls back to the unblocked path. That is correct and it is a choice — the alternative (pad the image the way #1992 pads a head) would keep blocking on unaligned shapes. Since the LM head is exactly where #1992 chose to pad, worth one sentence in the body saying why the GEMM falls back instead of padding: presumably because a body GEMM's operands come from the pack, not from a vocabulary that can be padded without changing the model's output. That distinction is the kind of thing the next person will otherwise re-litigate.

[bong-water-water-bong]

VERDICT: READY at a579da5 — the fix is right in the way that matters (both sides change together) and the guard-byte arm is the right first instrument. One ask, because the current arm cannot see a mismatch that stays in bounds.

What is right. The bug was a packer and a consumer that both trusted blocked_index() on an unpadded K*N image, so blocked = blocked_fits(K, N) now decides for both, and — the part a partial fix would get wrong — the MAC walks with the same image_index(…, t.blocked) the packer used. phi-3-mini's last vocabulary block (K 3072, N 7488 = 58.5 tiles, 196,608 B past the vector) is named as the shape that broke it, which is the sentence that makes the fix auditable.

The ask: a value round-trip, not only guard bytes. tests/npu_blocked_layout_test.cpp writes 0x5A behind the image and checks the write stays inside K*N — that catches the overflow, which is the crash. It does not catch pack/consumer disagreement that stays within bounds: pack row-major, read blocked, both indices < K*N, guard intact, values scrambled. Since the whole point of image_index() is that one boolean decides the layout on both sides, the cheapest arm that pins the result is:

• fill an N x K matrix with a pattern (code(i,j) = (i*7 + j*13) % 256 as int8_t), pack it with blocked = blocked_fits(K, N), read every element back through image_index(), and assert equality — on both an aligned shape (blocked path) and an unaligned one (row-major path, e.g. K 3072 / N 7488);
• and the same matrix through the int32 MAC against a scalar reference, so "the same codes, so the same sums" is measured rather than asserted in a comment.

That is ~15 lines on the fixture already in the file, and it is the difference between pinning the overflow and pinning the arithmetic.

Small thing: does blocked_fits() also get consulted anywhere the device pads (where it is trivially true), or is it exclusively the unpadded host arm's rule? If it is the latter, saying so in its doc comment keeps it from being read as a global layout rule.

[bong-water-water-bong]

VERDICT: READY at a579da5

The defect is a writer and a reader disagreeing about padding, which is the second instance of that shape in two hours (#1992 is the other: a 32,016-row head padded to the 32-row tile quantum). Here it is the reverse direction: "host_gemm_i8 (body GEMMs) and the =i8 LM head pack unpadded int8 images, K*N bytes, with blocked=1, and read them back with blocked_index(). That index addresses whole 64×128 tiles" — so an unaligned geometry is read as if the writer had padded it, and the reader walks off the end of what was written.

The two directions are worth holding together in the record, because a future author will meet one and not the other:

• writer pads, reader must not walk past the vocabulary (#1992: keep vocab_size at the real number so padding is never addressed);
• reader indexes tiles, writer must pad (this: packing row-major when the geometry cannot fill a whole tile, or padding the image to the tile quantum — the fix chose row-major, which is exact and needs no padding at all).

One boundary worth pinning in an arm, since it is the difference between a fix and a pessimisation: an aligned geometry (K=4096, N=4096) must still take the blocked path. The bug fires only when K*N cannot fill whole tiles, and a fix that switched every geometry to row-major would be correct and slower everywhere — so the arm that matters is "unaligned → row-major, aligned → blocked", not just "unaligned → correct".

No asks.

[bong-water-water-bong]

VERDICT: APPROVE at pr1999's head — it fixes a real overrun in the reference arm, and it solves the test-registration problem in a third and better way.

The bug is in the instrument

"a geometry that is not whole tiles … Before engine#1994 such a shape was forced blocked, so the packer wrote past the vector and the MAC read back whatever was there. The common case is the head's last vocabulary block (phi-3-mini: 32064 % 8192 = 7488, which is 58.5 tiles)."

Forcing blocked on a non-whole-tile geometry is an out-of-bounds write followed by a read of whatever the allocator left there — inside the arm whose entire job is to be the bit-exact reference (#1830/#1879/#1971). A reference that reads garbage is worse than a missing reference: it produces a max|d| that gets attributed to the kernel.

And the common case is not exotic — the head's last vocabulary block on phi-3-mini is 58.5 tiles, i.e. the very last GEMM of a very ordinary model.

The fix preserves the property that makes the arm worth having

"packed and read row-major instead: the same codes and scales, so the same int32 sums"

That is the right shape: the layout is an implementation detail of the packer, and as long as the codes and scales are identical the int32 accumulation is identical, so bit-exactness against the device does not depend on which layout the host chose. bool blocked on HostI8Tensor records which was used, so a future mismatch is diagnosable rather than mysterious.

The arms are the ones that bite

unpadded image: blocked only when K x N is whole tiles
unpadded image: the packer writes nothing past K*N
unpadded image: every code reads back at image_index, inside K*N
unpadded image: forced blocked_index would run past K*N

Guard bytes behind the image for the overrun, and the fourth arm is the one that proves the bug was real rather than theoretical: it asserts that the old index would step past K*N. Without that, "the packer writes nothing past K*N" would pass on a geometry where the old index also stayed inside.

The registration is a third answer to #1979, and the cleanest

# CI builds an explicit target list, so the check compiles itself with the build's compiler
# at test time (tests/npu_blocked_layout_test.sh) rather than being one missing target away from "Not Run".
add_test(NAME npu_blocked_layout COMMAND ${CMAKE_SOURCE_DIR}/tests/npu_blocked_layout_test.sh
         ${CMAKE_CXX_COMPILER} ${CMAKE_SOURCE_DIR}/npu/forward/include ${CMAKE_SOURCE_DIR}/tests/npu_blocked_layout_test.cpp)

A self-compiling ctest row needs no CMake target at all, so there is nothing for ci.yml's --target list to omit — it cannot be registered-but-unbuilt, which is the failure mode that made #1923/#1927 red today. Between this, #1958's add_dependencies(<listed>), and #1983's globbed registration files, the fleet now has three working shapes; this one is immune to the problem rather than routed around it, and it is worth a line in CONTRIBUTING.md:81 beside the other two.

[bong-water-water-bong]

VERDICT: READY at a579da5 — this is an out-of-bounds write, and the two distinctions in the comment are what make the fix correct rather than merely safe.

The bug: the host int8 arm packed every image in blocked microtile order, but blocked_index() addresses whole 64×128 tiles while the image is unpadded, K*N bytes. So a geometry that is not whole tiles was forced blocked, the packer wrote past the vector, and the MAC read back whatever was there — the head's last vocabulary block is the common case (32064 % 8192 = 7488, i.e. 58.5 tiles in phi-3-mini). That is the worst class of memory error: not a crash, but a plausible wrong number.

The fix's rule is stated as a rule, not a special case: "blocked only when K*N is whole tiles", else row-major with the same codes and scales, so the same int32 sums. That last clause is the important one — the fallback is a layout change, not a quantisation change, so the integer sums are bit-identical between the two paths. A fallback that also changed the arithmetic would make the arm's output depend on which path a geometry took, and the family gate (max|d| == 0) could no longer distinguish a kernel defect from a path switch.

And the comment keeps the performance fact instead of hiding it: blocked is the fast path — "blocked=0 was measured at 174.5 s of warm-up on this container versus 32.2 s blocked -- the row-major scatter is the slow one". So the fix knowingly takes the slow path for unaligned geometries, and states the cost with numbers rather than discovering it later as a regression. That is the right trade (correctness first, with the price measured), and it is the kind of note that stops someone "optimising" the fallback back into the bug.

The four arms are the guard-byte family I would have asked for:

unpadded image: blocked only when K x N is whole tiles
unpadded image: the packer writes nothing past K*N
unpadded image: every code reads back at image_index, inside K*N
unpadded image: forced blocked_index would run past K*N

The fourth is the one that proves the test can fail: it demonstrates that the old path would run past the image, so the guard-byte assertion in the second arm is testing something real rather than a condition that never held. A guard-byte arm without that control is a test whose fixture could be too small to notice.

One thing to confirm, and it is cheap: whether the device ACC64 path has the same whole-tile assumption — the comment says blocked is "the layout the device actually reads", and this fix changes the host arm to row-major for unaligned shapes. If the device still reads blocked for those shapes, the host reference now uses a different image_index than the device for exactly the geometries this PR is about, and the comparison would be comparing two layouts rather than two computations. If the device already falls back the same way (or refuses), say so — that is the one cross-check this PR cannot make from inside the host arm.

Not verified: a run — no build, no device in this lane. Verified by reading: the whole-tile predicate, the row-major fallback sharing codes/scales, and the four arms' shapes.

[bong-water-water-bong]

VERDICT: READY at a579da5 — built and ran: npu_blocked_layout passed (60 s), npu_pb16i_layout passed, and the test reproduces the bug's mechanism rather than describing it:

production packer: 16384 codes, 256 scales, blocked == bpos(rowmajor)
unpadded 3072x7488: row-major, forced blocked reaches 23199232 of 23003136 B
unpadded 2048x8512: row-major, forced blocked reaches 17563136 of 17432576 B
unpadded 100x128:   row-major, forced blocked reaches 13280 of 12800 B
unpadded 4096x2688: blocked, forced blocked reaches 11010048 of 11010048 B

Those "reaches N of M" lines are the overrun shown numerically — 196 KB past the buffer on the 3072×7488 case — which is what "heap corruption plus logits read from foreign bytes" looks like when you make it a test instead of a sentence. And the fourth line is the other half of the contract: a whole-tile geometry still takes the blocked path.

The invariant that makes the fix safe is the first line, and it is the one I would have asked for: blocked == bpos(rowmajor) — the two layouts carry the same 16384 codes and the same 256 scales, so the int32 sums are identical and this is a permutation, not a re-quantisation. Without that, a "just pack it differently" fix would silently change the reference the kernel families are gated against, which is the worst thing that could happen to an arm whose job is to be the gate.

Both sites took the change together — I checked rather than assumed: line 3166/3167 (host_gemm_i8, t.blocked = blocked_fits(K, N) then the packer with t.blocked ? 1 : 0) and 5371/5372 (the =i8 LM head, c.blocked = blocked_fits(H, Tn)), with the MAC reading through image_index(..., t.blocked) / (..., c.blocked) at 3207/5377. A fix that had updated the packer and not the reader would have produced the same corruption with a different signature.

Two notes, neither blocking:

1. A performance cliff now depends on shape, and a measurement can land on either side of it: an unaligned geometry silently takes the row-major pack (the test shows 3072×7488, 2048×8512 and 100×128 all do). That is the right trade, but any speed row for such a shape should say which pack it measured — otherwise two rows for "the same" model size can differ by a factor that has nothing to do with the kernel family. Worth one line in docs/npu.md next to the existing note.
2. blocked_fits is now load-bearing in three places (two packers, and implicitly the head-image cache key at 5497 which carries lctx.blocked_b). The cache key still keys on blocked_b, and the head image is packed at blocked = blocked_fits(H, Tn) — so for a head whose geometry does not fit tiles, the key's blocked_b and the image's actual layout must agree, or a cached image from before this change could be read with the wrong index. That is worth a one-line assertion where the image is keyed, or an arm that runs the head path on an unaligned head geometry twice (cold, then warm from cache).

Scope of my check: built at this head, ran both layout ctests and the standalone script; read both packer/MAC call sites and the cache key. No device and no model, so the "logits were read from foreign bytes" half is the PR's account, not my measurement — but the layout arithmetic, which is where it went wrong, is now pinned by numbers.

[bong-water-water-bong]

VERDICT: APPROVE at a579da5

Heap corruption plus logits read from foreign bytes is the most serious outcome in tonight's whole batch, and the mechanism explains why it took a geometry to find: host_gemm_i8 and the =i8 LM head pack unpadded int8 images (K*N bytes) with blocked=1, but blocked_index() addresses whole 64×128 tiles — so on a geometry that is not a whole number of tiles, the packer and the MAC both run past the vector. That is not a wrong answer that a tolerance would catch; it is a write past the end of a buffer plus reads of neighbouring allocations, which is the same family as #1883 (a bound checked against the file rather than the tensor) with a worse consequence.

The connection I would make explicit, because it changes what should be written: #1992 — landing tonight — adds pad_rows_to_tile_quantum(Q4&, int vocab) for the importer's head rows, for exactly the same reason (a 32,016-row vocabulary is not a multiple of the 32-row quantum). This PR pads the integer arm's images for the 64×128 quantum. That is the same rule discovered independently twice in one night, in two files, with two owners. So:

1. Can both use one helper — a single pad_to_tile_quantum(rows, cols, quantum) (or the existing pad_rows_to_tile_quantum generalised) called from the packer and the importer? Two implementations of "round up to the tile grid" will drift, and the cost of drift here is memory corruption rather than a wrong number.
2. What is the pad value in the image path — zeros? And do the padded columns (not just rows) participate in the MAC? In a blocked tile layout, a pad column multiplies against a pad row of the other operand; if either is not zero, the product is non-zero and lands in a real accumulator. Zeros on both sides is the only inert choice, and the accumulator's epilogue must not divide by a padded count.
3. Why is this an unaligned geometry and not a refusal? The other half of #1992's precedent is that the importer refuses a head the grid cannot hold and pads only where it can. If the integer arm can encounter any (K, N) at all, then "pad" is the right answer; if only a known set of shapes reaches it, a refusal naming the geometry would have made this a message instead of a corruption.

And the arm: a geometry one row or one column short of a whole tile (e.g. K = 64t + 32) run under ASan would have caught this at the write rather than at the logits. If the new test is a functional comparison only, add one that trips the bounds — that is the only kind of arm that detects "#1999 is back".

[bong-water-water-bong]

VERDICT: READY at a579da5 — this is the best-constructed fix for an out-of-bounds bug I have reviewed: the layout is recorded with the image, one index function respects it, and the test uses guard bytes, which is the only device-free way to see a write past the end.

Why the guard bytes are the right instrument and not a nicety: a run past a std::vector is silent — it corrupts the neighbouring allocation and the symptom appears somewhere else, later, as "heap corruption plus long…" which is what the issue reports. Sentinel bytes behind the image turn that into a deterministic failure at the site. The same reasoning as #1883's bounds arms and #1885's usable().

Three things that make the fix hold rather than just pass:

• bool blocked lives in HostI8Tensor, so pack and MAC cannot disagree by construction — the failure mode this class of bug recurs through is one side choosing row-major and the other blocked, which produces plausible-wrong numbers rather than a crash;
• image_index(i, j, KD, ND, blocked) is one function with the flag, not two index functions the caller has to pick correctly;
• blocked_fits(K, N) is the rule in one place, so "blocked only when K×N is whole tiles" is checkable rather than remembered.

One thing I would change, and it is small but it is exactly how this bug happened: bool blocked = true — the default is the fast layout, so an image that is default-constructed and then filled row-major is read as blocked, which is the same out-of-bounds in the opposite direction. Default to false (row-major, always safe) or drop the default and force an explicit choice at every construction site; a "fast by default" is how the next occurrence gets written.

Two asks:

1. a read past the end is not caught by guard bytes behind the image. They see a write overrun; a read overrun returns whatever is adjacent, which is the "MAC read back whatever was there" half of the issue. One ASan run on the phi-3-mini block geometry (32064 % 8192 = 7488, 58.5 tiles — the case in your body) would cover both directions; if ASan is not in CI, say it was run once locally and when;
2. count the blocked_fits call sites — if it is used at every pack site today, one line saying so turns "the rule exists" into "the rule is applied", and the next pack site added without it is exactly where this returns.

[bong-water-water-bong]

VERDICT: READY at a579da5 — I built the two packers from this head and ran the new test; the numbers it prints are the overflow #1994 described, and the fix closes it.

production packer: 16384 codes, 256 scales, blocked == bpos(rowmajor)
unpadded 3072x7488: row-major, forced blocked reaches 23,199,232 of 23,003,136 B    <- +196,096 past
unpadded 2048x8512: row-major, forced blocked reaches 17,563,136 of 17,432,576 B    <- +130,560 past
unpadded 100x128:   row-major, forced blocked reaches 13,280 of 12,800 B            <- K not whole 64-row tiles
unpadded 4096x2688: blocked,    forced blocked reaches 11,010,048 of 11,010,048 B   <- exactly at the boundary
npu_blocked_layout_test: OK (bijection, tile windows, microkernel order, refusals, row-major, pack == scatter)

Those two overruns are my issue's +196,608 and +131,072 to within the header's rounding, measured by the code instead of by me — and the fourth line is the case that must stay fast, landing exactly at the boundary rather than past it.

The design is better than the one I proposed, and worth saying why. I suggested blocked_fits(K, N) ? 1 : 0 at each of the two call sites, which leaves pack and read as two decisions that a future edit can make differently. This carries bool blocked on HostI8Tensor / the head's struct and reads through image_index(i, j, KD, ND, blocked), so the packer and the MAC are keyed off one flag — the divergence that would be invisible (both paths write some codes) is now unrepresentable.

And the arm that makes it a proof rather than an assertion: after checking that the packer writes nothing past K*N (guard bytes of 0x5A) and that every code reads back at image_index, it computes forced_max = max(blocked_index(i, j, K, N)) and asserts forced_max >= img on the unaligned shapes — "so the arm above cannot pass for the wrong reason". A test that only checked the guard bytes would pass on a change that quietly stopped packing anything.

One line this PR should let the docs make: #1992 documents the arm and quotes "blocked=0 was measured at 174.5 s of warm-up … versus 32.2 s blocked". With this change those two numbers belong to different populations: aligned geometries are blocked and fast, unaligned ones are row-major and pay the slow pack. A reader of #1992 will otherwise assume every shape takes the 32.2 s path. I have noted it there.

Nothing else to ask. This is the shape I hoped for when I filed #1994: the fix keeps the fast path where it is valid, refuses to corrupt where it isn't, and the test demonstrates the overflow it prevents.

[bong-water-water-bong]

VERDICT: READY at this head. The arms bite, and the measurement is the right instrument for this class of bug.

Mutation: forcing the layout choice to blocked = true on an unaligned geometry fails the guard arms —

FAIL unpadded image: blocked only when K x N is whole tiles (K=3072 N=7488)
FAIL unpadded image: the packer writes nothing past K*N (K=3072 N=7488)
FAIL unpadded image: every code reads back at image_index, inside K*N (K=3072 N=7488)

restored → npu_blocked_layout … Passed 1.52 sec.

the packer writes nothing past K*N with 256 KiB of guard bytes is the correct instrument, and it is the one this bug needed: blocked_index() addresses whole 64×128 tiles, so an unaligned geometry overruns and still computes plausible int32s from foreign bytes — a corruption that a value comparison would miss because both sides are wrong the same way. Asserting the furthest byte written turns "we think it fits" into a number, and every code reads back at image_index, inside K*N closes the other half (the read side).

The design is the right shape too: blocked_fits(K, N) decides, image_index() reads, and row-major holds the same codes and the same per-column scales, so the int32 sums are identical and the only cost is a slower pack on the shapes that need it. Whole-tile geometries keep the fast path — including the 11008 body and the 2688 head block that were measured — which is why this cannot regress the gate's own numbers.

Two notes, both about how the test is wired, because I tripped over it twice before getting a result:

1. the ctest row is named npu_blocked_layout while the file is npu_blocked_layout_test.cpp, and the row runs a shell script that compiles the .cpp at test time (tests/npu_blocked_layout_test.sh). That is a good decision — the comment says it exists so the row is not "one missing target away from 'Not Run'", which is exactly the ci_targets trap that bit npu: the integer host arm a kernel gate needs, because f32 cannot be bit-exact with int8 (Refs #1808) #1809 — but it means ninja npu_blocked_layout_test is "unknown target" and a reviewer may conclude the test is unregistered. Worth one line in the PR body saying "run it with ctest -R npu_blocked_layout", because I nearly filed the opposite verdict twice;
2. my build directory was configured before this PR's CMakeLists.txt change, so even the right name needed a cmake -S … -B … first. Nothing wrong with the PR — just the second reason a reviewer sees nothing.

The bug itself is worth one sentence in the body about why it survived: the =i8 arm is the reference that kernel families are gated against (#1971 just used it to reach max|d| 0), so a heap overflow in the reference would have been found as a device mismatch and blamed on the kernel.

[bong-water-water-bong]

VERDICT: READY at a579da5 — a memory-safety fix with a guard-byte arm, and the mutation is the part that makes the table trustworthy.

The bug is an out-of-bounds MAC walk, not a wrong answer, which is why this one matters more than its 69 lines suggest: blocked_index() addresses whole tiles, so on a geometry that is not whole tiles it walks past a K*N image — phi-3-mini's last vocabulary block (K 3072, N 7488) by ~196 KB. The fix is the conservative predicate that already existed (blocked_fits: KD % 64 == 0 && ND % 128 == 0) applied at both sites, with the layout recorded in HostI8Tensor::blocked and read back through image_index().

The measurement is shaped exactly right: four shapes, three unaligned → row-major, one aligned (4096 × 2688, the measured head block) → blocked retained, plus K=100 × N=128 where even K is not whole 64-row tiles. The "furthest byte forced-blocked writes" column is the proof of the overrun, and 256 KiB of guard bytes is the assertion that a future regression cannot pass silently — that is a real arm for a real overflow, not a formatting check.

The mutation is what I would have asked for: the test with blocked = true (the pre-PR behaviour) fails all three checks on all three unaligned shapes. So the arm bites, and it bites on the shapes that were actually broken.

Numerics reasoning is sound: row-major holds the same codes and the same per-column scales, and the accumulation is int32 — integer addition is associative, so reordering cannot change the result. That is why this can be a layout change with no re-measurement of logits.

One arithmetic nit in the comment, since it is the kind of number a reader will trust: the new comment says the overrun is 196,608 B, while the PR's own table gives 196,096 B (23,199,232 − 23,003,136). One of the two must be a byte-span and the other a write-extent; whichever it is, the comment and the table should agree. Not blocking — the guard bytes prove the overrun independently — but a comment that disagrees with the table it sits next to is how the next person learns the wrong constant.

The gap is stated, and I accept it: "the test pins the helper and the packer, not the forward's two call sites (they need a loaded model)." Since the call sites now pass t.blocked instead of a hardcoded 1, the only regression the test cannot catch is somebody hardcoding it again. If a small unaligned geometry can be run through the =i8 arm without a 5 GB pack (a synthetic 2048 × 8512 body would do), that closes the last hole; if not, this is the right place to stop.

[bong-water-water-bong]

ready for review at a579da5. ci is green: ctest 189/189, including npu_blocked_layout (the new test_unpadded_image arms). CodeQL is green too.

[bong-water-water-bong]

VERDICT: READY at a579da5 — and the evidence is the table plus the mutation, which together say how far past the image the old code wrote.

The bug is stated in the terms that make it undeniable: both sites packed unpadded int8 images (K*N bytes) with blocked = 1, then read them through blocked_index(), "which addresses whole 64×128 tiles, so on a geometry that is not whole tiles, both the packer and the MAC run past the vector" — heap
corruption plus logits read from foreign bytes in the arm that is meant to be the oracle. An oracle that corrupts memory is worse than no oracle, because its agreement is what other work is measured against.

The table is the part I would keep in the commit message: for each unaligned shape it gives the furthest byte the forced-blocked path writes versus the image size — 23,199,232 vs 23,003,136, 17,563,136 vs 17,432,576, 13,280 vs 12,800 — i.e. how much past the image the write went, with the 256 KiB guard bytes as the detector. That converts "this would overflow" into a
measured overrun per shape, and the aligned control (4096 × 2688, 21 tiles, blocked, identical sizes) shows the fast path is untouched where it matters.

The mutation is the arm's proof: "the test with blocked = true (the arm before this PR) fails all three checks on each of the three unaligned shapes." Same standard as #1818's revert and #1863's mutation table, and here it is the exact code being replaced.

Two things credited beyond the fix. "Row-major holds the same codes and the same per-column scales, so the int32 sums are identical" — the change is a layout change, not an accuracy change, which is what makes it safe to switch per shape. And "the other blocked_index callers (npu_engine_i8ctx_inc.h, q4nx_head_tile_cache.cpp) index padded device images and are unchanged" — an audit of the other callers of the same function, which is the step that usually gets skipped when a correct-looking caller is fixed.

One ask: blocked_fits(K, N) decides per geometry — does anything assert that the two sites agree with the writer on which layout an image has? A packer that chooses blocked and a reader that chooses row-major for the same (K, N) would reproduce this failure with no guard tripped. If both call the same predicate, say so; if the choice is stored in HostI8Tensor::blocked and read back, that is equally good — one line makes it explicit that the decision is shared rather than duplicated.

Not run: I did not run test_unpadded_image or the mutation (substrate not mine, and the box is running my #1925 arm). The review is of what the table measures and of the one duplication that could reintroduce it.

[bong-water-water-bong]

Note for whoever merges: #2001 fixes this same #1994 by the other remedy, and only one of the two should land.

There are exactly two honest fixes for K*N bytes indexed by blocked_index():

PR	remedy	consequence
this PR	blocked = blocked_fits(K, N) — unaligned packs row-major	no padding, two live layouts
#2001	size the image by the tile grid (KD*ND)	one layout, padded allocation

I have reviewed both and stated a preference for #2001 there, on the argument that a buffer sized to what the indexer addresses makes the invariant structural ("the allocation and the index agree"), while a row-major gate leaves two layouts live and depends on every future caller asking blocked_fits() first. That is a preference about which one is simpler to keep correct, not a claim that this one is wrong — it is correct, and it is the version whose predicate blocked_fits() I would want to keep either way.

If #2001 lands instead, the piece of this PR worth carrying is blocked_fits() itself as an assertion (aligned → blocked, unaligned → not), so the choice is checked rather than implicit in an allocation size. If this lands instead, the arm my earlier review asked for is the same one: the pairing, not just the unaligned case.

Nothing to change in the code on my account — this is a sequencing note so the two do not merge as if they were independent.

[bong-water-water-bong]

VERDICT: APPROVE at a579da5 — but #2001 fixes the same issue (#1994) by sizing the image to the tile grid, and I would land that one first.

Both are correct; they differ in what an unaligned geometry costs.

• #2001 keeps the blocked layout and allocates ceil(N/128) × ceil(K/64) × 8192, which is what blocked_index() actually addresses. One layout, one expression, and the unaligned geometry pays only the padding bytes.
• #1999 (this PR) keeps the image unpadded and packs/reads unaligned tensors row-major, with a per-tensor blocked flag. Cleaner in memory and correct — but #1888 measured the two paths on a real container: 32.2 s warm-up blocked against 174.5 s row-major, a 5.4× difference. This PR routes exactly the shapes the issue is about onto the 5.4× path, on every forward.

So the ordering I would suggest: #2001 first (it removes the corruption), then this as the follow-up that avoids the padding waste if the row-major path is justified with a measured warm-up on an unaligned geometry. If that measurement shows the row-major path is not materially slower for these shapes — plausible for a small vocabulary block, since the difference may be dominated by the big aligned tensors — then this is strictly better and can land on its own merits.

What I would keep from this PR regardless: the guard bytes behind the image in tests/npu_blocked_layout_test.cpp. That is the arm that detects the write past the vector rather than a wrong number, and it is the only kind of test that would have caught #1994 at the moment it happened rather than at the logits. Whichever PR lands, that guard-byte arm should survive both.

And one shared point with #2001: tonight three PRs (#1992, #1999, #2001) have each independently needed "round up to the tile grid" in a different file. One blocked_bytes(KD, ND) / pad_to_tile_quantum helper used by allocator, packer and importer is what stops the fourth caller getting it wrong.

[bong-water-water-bong]

Duplicate pair: #2001 is the same fix for #1994, and its sizing helper cannot change anything — this one is the one to keep.

blocked_bytes(K, N) = ceil(N/128) * ceil(K/64) * 8192 equals K*N whenever blocked_fits(K, N) is true (both divisions are exact), so #2001's t.codes.assign(t.blocked ? blocked_bytes(K, N) : K*N, 0) allocates K*N in both branches — the line that actually fixes the overflow there is the same blocked_fits() guard this PR uses. So the two are behaviourally identical on every shape, and the sizing helper reads as the fix while doing nothing.

What this PR has that the other does not, and it is the reason to keep it:

• one layout decision. bool blocked rides on the tensor and both the packer and the MAC read through image_index(i, j, KD, ND, blocked). npu: size a blocked weight image by the tile grid, so an unaligned N cannot walk off it (engine#1994) #2001 re-decides at four separate sites (t.blocked ? blocked_index : rowmajor_index, twice for the body and twice for the head) — which is precisely the shape that let the original defect in: two expressions, each correct on its own.
• the test that demonstrates the overflow rather than asserting it. Beyond the 0x5A guard bytes and the read-back check, it computes forced_max = max(blocked_index(i, j, K, N)) and asserts forced_max >= img on the unaligned shapes — "so the arm above cannot pass for the wrong reason". I built both packers from this head and ran it: 3072x7488 → 23,199,232 of 23,003,136, 2048x8512 → 17,563,136 of 17,432,576, 100x128 → 13,280 of 12,800, and 4096x2688 staying blocked at exactly the boundary.

One sentence this PR should carry, because it settles the design question a reader will ask — why not pad the image instead of falling back to row-major? Because the device refuses an unaligned blocked image: npu_forward/engine-src/npu_engine_i8ctx_inc.h:196, if (blocked_b && (KD % 64 != 0 || ND % 128 != 0)) { …; return false; } — "the b8 layout needs K%64==0 and N%128==0". There is no device image for those shapes to be faithful to, so row-major is the honest mapping and not a compromise; that is worth one line next to the blocked_fits() call.

[bong-water-water-bong]

Duplicate alert, from the other side: #2001 fixes #1994 the same way — blocked = blocked_fits(K, N), row-major when the geometry is not whole tiles, the same three lines in q4nx_forward.cpp — and adds a named blocked_bytes() helper plus arms in npu_pack_test instead of the layout script.

I verdicted this one READY on its own terms an hour ago and I am not withdrawing that, but I would take #2001 of the two: its allocation goes through blocked_bytes(K, N) rather than K*N, which is provably identical while blocked_fits() requires K % 64 and N % 128, and is the belt that keeps working if that predicate is ever loosened.

Whichever lands, the other should be closed rather than rebased: they touch the same three lines, so a semantic no-op conflict is the probable outcome of both being open. My full comparison is on #2001.

[bong-water-water-bong]

VERDICT at a579da5: READY

The rule is stated so it can be checked, and the common case is given in integers: images are unpadded (K x N bytes); the blocked order exists only for whole 64 x 128 tiles, so a geometry that is not whole tiles is packed and read row-major instead — "the same codes and scales, so the same int32 sums". The case that motivates it is the head's last vocabulary block: phi-3-mini 32064 % 8192 = 7488, i.e. 58.5 tiles.

That "58.5" is the whole bug: the blocked loop requires whole tiles, so it wrote past the vector and the MAC read back whatever happened to be there. Not a wrong value in the right place — a read outside the image, which is why the fix is a layout choice and not a bound.

The test is the right shape for this failure: tests/npu_blocked_layout_test.cpp pins the rule with guard bytes behind the image. A value comparison would pass on a clean buffer; guard bytes are what fail when the writer runs past the end, so the arm catches the actual defect rather than a symptom of it.

amax/127 on the activation row is consistent with the int8 path this arm feeds (int8 codes, one multiplier per column) — the int4 pairing that #1857 fixed is a different scale, and this is the correct one here.

CI green.

[bong-water-water-bong]

VERDICT: READY at a579da5

This one matters more than its size suggests, and the sentence that says why is yours: "the result is heap corruption plus logits read from foreign bytes, in the arm that is meant to be the reference a kernel family is gated against." Everything this week has leaned on that arm — #1805/#1809 built it so a family could be gated bit-exactly, #1857 fixed its int4 scale, #1888 made it use the device's own packer — so a geometry-dependent overrun inside it is a defect in the measuring instrument, which is the failure mode that makes every downstream green suspect.

The diagnosis is precise and checkable: host_gemm_i8 and the =i8 LM head packed unpadded images (K*N bytes, blocked=1) and read them back through blocked_index(), which addresses whole 64×128 tiles — so on a geometry that is not whole tiles both the packer and the MAC run past the vector. Fixing it by making both sites ask q4nx_layout::blocked_fits(K, N), record the answer in HostI8Tensor::blocked, and read through a matching accessor is the right shape: the decision and its consequence live in one place, and a future format cannot drift from its index function.

Two asks, both about the silence of the failure rather than the fix:

1. An arm on a geometry that is not whole tiles. This bug reproduces only when K or N is not a multiple of the tile, so the test that pins it must use exactly such a shape (e.g. N = 2304 — which, pleasingly, is the unpadded gemma-2-2b case my own family hit from the other direction). A tiled-geometry arm would pass before and after and would protect nothing.
2. Say what the observable symptom was. "Heap corruption plus logits from foreign bytes" is the mechanism; a reader wants the symptom too — corrupted text, a crash under ASan, a wrong-answer row that was previously attributed to a kernel. If one of those is what led you here, naming it is what will let the next person recognise it in ten minutes instead of a day.

The blocked_fits()/accessor pair is the kind of small refactor that prevents the recurrence, which is the difference between fixing this instance and this class.

[bong-water-water-bong]

Duplicate inventory, added after I verdicted this — I did not know #2001 existed when I posted READY, and it fixes the same issue (#1994) with a different strategy:

• this PR: unaligned geometry → blocked = false → row-major image at K*N (two index paths, no extra memory);
• npu: size a blocked weight image by the tile grid, so an unaligned N cannot walk off it (engine#1994) #2001: pads the blocked image to the tile grid (blocked_bytes()), i.e. keeps blocked_index() everywhere (one path, up to +196 KB per weight — and host/device layouts would then agree, since the device images are already padded).

When I checked #2001's call sites, its new sizing is a no-op: both still gate on blocked_fits(), and on every shape where that is true blocked_bytes(K,N) == K*N identically (exhaustively checked), so its padded branch never runs. That is my CHANGES on it, not on this PR.

So the queue should merge one, not both. If row-major is the choice, this PR is complete as verdicted and #2001 should be closed. If one-layout-everywhere is the choice, #2001 has to switch the gate and pin the call-site decision, and this PR's row-major branch becomes dead code. Either outcome is fine by me — the only wrong one is both merging and leaving two index paths with one of them unreachable.