Skip to content

npu: t2s wire format v2 -- the ordering moves into the stream, so the core does no shuffle - #836

Merged
bong-water-water-bong merged 3 commits into
mainfrom
npu/t2s-v2-format
Nov 2, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
npu/t2s-v2-format

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

The kernel's expansion has to spread one 2-bit code byte onto the mmul operand's even byte (out[2i]) and the next 64 onto its odd bytes (out[2i+1]). Under the shipped order (now v1) that spread is a lane-crossing aie::interleave_zip in the core — measured 11.1-12.6 GB/s against 30-32 GB/s for the plain contiguous stores on the same box. v2 changes only the order inside each 64-byte block, so the plain two-store expansion is exactly right:

v2: byte b -> lanes 0/1: k = kb*16 + b/8,      n = nb*16 + 2*(b%8), +1
              lanes 2/3: k = kb*16 + 8 + b/8,  n = nb*16 + 2*(b%8), +1

because operand byte b needs element 2b = 16*(b/8) + 2*(b%8), and byte 64+b needs 2*(64+b) = 128 + 2b, i.e. eight k rows down — which is what lanes 2/3 carry. The tile stays 2,176 B, the tile order and the ms area are unchanged, so no DMA descriptor moves and the kernel binary does not change either.

Device evidence

One npu-hold ticket, ≤ 60 s, amdxdna counter 0 after it (arena GU16T2S, K=5120 N=17408, M=16):

run verdict
seeds 1 / 2 / 3, v2 stream PASS, 0 mismatches each, reads 2/2 clean
timed, 5 launches PASS, 0 mismatches
rate not claimed here: that minute was at load 15 / Tctl 82 with another lane's GPU ticket live (every seed needed a readback retry). A leader-control ticket is queued; the number goes in the arena leaderboard, not this body.

And the strongest device-free check available: from_pb16i_image_v2() reproduces, byte for byte, the exact 23,674,880-B stream that passed on the array (run-t2s/v2_s1.B).

Contents

  • tools/q4nx_t2s.py: from_pb16i_image_v2() + decode_v2(); v1's functions are untouched.
  • module docstring: both byte orders, and why v2 wins.
  • docs/npu.md: the format section, the q4nx_weight_layout_version mapping (5 = v1, 6 = v2) and the warning that the two are not interchangeable — a v1 image read by the no-shuffle kernel is a silent permutation of the weight matrix, which is exactly the failure three single-code probes had to name.
  • tests/q4nx_t2s_test.py: 12 new checks — same codes and multipliers as v1, only the code area differs, and the 2b / 2*(64+b) block algebra. Green locally (0 failure(s)).

Refs: #821 (the stream itself), #826 (the launch path), and the arena HARVEST.md entries for the probe gate and this ordering rule. No merge — the orchestrator merges.

… core does no shuffle

The kernel's expansion has to spread one 2-bit code byte onto the mmul operand's even byte (`out[2i]`) and
the next 64 onto its odd bytes (`out[2i+1]`). Under the shipped order (now called v1) that spread is a
lane-crossing `aie::interleave_zip` in the core, measured on the array at 11.1-12.6 GB/s against 30-32 GB/s
for the plain contiguous stores. v2 changes only the order inside each 64-byte block so the plain stores are
exactly right:

    v2: byte b -> lanes 0/1: k = kb*16 + b/8,      n = nb*16 + 2*(b%8), +1
                  lanes 2/3: k = kb*16 + 8 + b/8,  n = nb*16 + 2*(b%8), +1

because operand byte `b` needs element `2b = 16*(b/8) + 2*(b%8)` and byte `64+b` needs `2*(64+b) = 128 + 2b`,
i.e. eight k rows down. The tile stays 2,176 B, the tile order and the ms area are unchanged, so no DMA
descriptor moves.

Device evidence (arena, one <=60 s npu-hold ticket, `amdxdna` counter 0 after it): the same kernel binary that
needs the interleave under v1 is **byte-exact under v2 on seeds 1/2/3, 0 mismatches**, and the v2 stream this
module now emits is byte-identical to the stream that passed (23,674,880 B, sha-compared here).

Contents: `from_pb16i_image_v2()` and `decode_v2()` in tools/q4nx_t2s.py (v1's functions unchanged), the
module docstring documents both orders and why v2 wins, docs/npu.md gains the format section incl. the
`q4nx_weight_layout_version` mapping (5 = v1, 6 = v2) and the warning that the two are not interchangeable,
and tests/q4nx_t2s_test.py pins v2 to the same codes and multipliers as v1, that only the code area differs,
and the `2b` / `2*(64+b)` block algebra. 12 new checks, all green locally.
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit c024333)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

821 - Partially compliant

Compliant requirements:

  • Implement the 2-bit ternary stream (t2s) format that reduces weight bytes by half
  • Ensure the t2s format is exact, not a requantisation
  • The multiplier is already per-128-K and does not need rounding
  • The block order is chosen for expansion, not for the decoder
  • Runs do not shrink
  • Add new tool tools/q4nx_t2s.py to derive the stream form the XDNA decode kernel reads
  • Add new test ctest q4nx_t2s to verify size, bandwidth, and correctness
  • Verify that the t2s stream produces identical results to pb16i for real Bonsai-2 images
  • Verify that the t2s stream produces identical results to pb16i for kernel arithmetic

Non-compliant requirements:

  • None

Requires further human verification:

  • None

826 - Partially compliant

Compliant requirements:

  • Implement the t2s launch path that allows containers to stream the 2-bit stream
  • Add support for "q4nx_weight_layout": "t2s" and "q4nx_weight_layout_version": 5
  • Ensure images are stored as blk_<layer>_<tag>_weight.t2s with unchanged .ref files
  • Add support for q4nx_t2s.h to handle the difference in file format
  • Ensure that the engine can read t2s images correctly
  • Ensure that the engine refuses to read pb16i images as t2s and vice versa
  • Add new tests for t2s layout and plan loading

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect v2 byte order logic

The v2 byte order logic in from_pb16i_image_v2 function appears to have an incorrect mapping of the wire bytes to the operand elements. The current implementation uses k_lo, n0 = b // 8, 2 * (b % 8) which may not correctly map to the intended operand byte positions. The block algebra described in the module docstring suggests that byte b should map to element 2b and byte 64+b to element 128 + 2b, but the current indexing logic does not seem to align with this.

b = np.arange(ROWS * 4)                              # the 64 wire bytes of one block
k_lo, n0 = b // 8, 2 * (b % 8)
wire = ((v[:, :, :, k_lo, n0] << 0) | (v[:, :, :, k_lo, n0 + 1] << 2) |
        (v[:, :, :, k_lo + 8, n0] << 4) | (v[:, :, :, k_lo + 8, n0 + 1] << 6)).astype(np.uint8)
out = np.array(v1, copy=True)
out[:, :CODE_BYTES] = wire.transpose(0, 2, 1, 3).reshape(-1, CODE_BYTES)
return out
Potential memory layout mismatch in v2 decoding

In the decode_v2 function, the way the codes are being assigned to the final codes array may not correctly reflect the v2 byte order. The assignment codes[:, :, :, k_lo, n0] = v[..., 0] and similar lines for other lanes may not correctly map the decoded values to their intended positions in the final tensor, especially considering the specific v2 byte order.

codes = np.empty((blocks.shape[0], KB, NB, ROWS, COLS), dtype=np.int8)
codes[:, :, :, k_lo, n0] = v[..., 0]
codes[:, :, :, k_lo, n0 + 1] = v[..., 1]
codes[:, :, :, k_lo + 8, n0] = v[..., 2]
codes[:, :, :, k_lo + 8, n0 + 1] = v[..., 3]
ms = np.ascontiguousarray(t[:, CODE_BYTES:]).view("<i2").reshape(-1, NB, 2, COLS)
return codes, ms

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Measured on the array: the v2 format this PR adds is ~2x the phase. One window, both runs byte-exact, amdxdna counter 0 (probe/measure-t2s-v3.sh, 2026-11-02 13:50 AST, box load 8.8 falling from 23):

run bytes med_ms GB/s
pb16i leader (same GEMM at 4.5 b/w) 50,135,040 1.625 30.85
v2 stream + hoisted expansion 23,674,880 0.829 (min 0.809) 28.58

1.96x in that window (1.52x against the 13:48 leader at 1.261 ms — the leader's own rate moved 30.85-39.75 GB/s across today's windows, so the ratio travels with its window, never alone). Seeds 1/2/3 all PASS with 0 mismatches.

Two things this PR's packer is now validated against, beyond its own tests:

  1. from_pb16i_image_v2() reproduces byte for byte the 23,674,880-B stream that passed on the array.
  2. An independent implementation of the same order (the arena's layout_t2s.py lineage) produces the identical file, and the cost of getting the order wrong is now measured: with v1 + a plain two-store expansion the kernel returns uncorrelated numbers, and doing the spread in the core instead (aie::interleave_zip) costs 11.1-12.6 GB/s against 30-32.

Follow-up this PR deliberately does not do (needs a PR of its own, on top of #826): wire the version into the launch path — q4nx_weight_layout_version 6 selects v2, and v2 containers must require a distinct ELF suffix (_t2s2_m16) so that a v1 kernel and a v2 image can never be paired silently. That naming decision is written down in ~/wt/npu-arena/submissions/GU16T2S/NOTES.md with the rest of the bundle.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 2, 2026

@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 1395afe60 — and the device-free claim is the strongest one here, so I checked it against the artifact
the array actually consumed.

from_pb16i_image_v2() reproduces the arena file byte for byte, measured here: I regenerated the v2 stream from
~/models/npu/bonsai2-pb16i-ternary/blk_0_ffn_gate_weight.pb16i (K=5120, N=17408) and compared it with
~/wt/npu-arena/run-t2s/v2_s1.B, the stream the PR says passed on the array:

v2 stream 23,674,880 B (10,880 tiles x 2,176)   arena run-t2s/v2_s1.B 23,674,880 B
byte-identical:            True
v1 vs the same arena file:  21,938,181 bytes differ     (so the file really is v2 order, and v1 is not it)

That is the claim that matters and it does not depend on the kernel: the bytes the array was fed are exactly what the new
converter produces from the same pb16i image.

The test suite pins the rest, python3 tests/q4nx_t2s_test.py at this head: 0 failure(s), with the 12 new checks —
v2 stream has the same size as v1 [(64, 2176) vs (64, 2176)], v2 stream is not the v1 stream (the order really changed), decode_v2 recovers the same codes as decode, … the same multipliers, v2 and v1 disagree only inside the code area (ms bytes identical), and the two index proofs the body argues from in prose —
v2 lane0/1 land on operand element 2b, v2 lane2/3 land on operand element 2*(64+b).

Scope is honest: v1's functions are untouched (from_pb16i_image still produces the v1 order, which is why the arena file
differs from it), the tile stays 2,176 B, the ms area and tile order do not move, so no DMA descriptor changes and the
kernel binary is unchanged. The rate is explicitly not claimed here (the ticket ran at Tctl 82 with another lane's GPU
ticket live), and §"What this does not claim" says the leader-control ticket is queued — that is the right way to leave it.

CI at this head: build pass, CodeQL pass, pr_agent pass. tprs-ready applied. (Verdict posted a few minutes past the
hour: this PR opened 17:39:58Z and I was mid-cycle on the owner-worded #833/#835 pair.)

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit a07e1e0

@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 a07e1e0 (the main merge; own diff unchanged at +105/−3, 3 files).

Re-derived at this head, not carried over:

$ python3 tests/q4nx_t2s_test.py                -> 0 failure(s), rc 0
$ from_pb16i_image_v2(blk_0_ffn_gate_weight.pb16i, 5120, 17408)
  v2  23,674,880 B   arena run-t2s/v2_s1.B  23,674,880 B   byte-identical: True
  v1  23,674,880 B   vs the same arena file:            21,938,181 bytes differ

so the bytes the array was fed are exactly what this converter produces from the same pb16i image, and v1 is not them. Same
numbers as at 1395afe; the merge of main changed none of it.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit c024333

@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 c0243335e3 (the main merge; author diff byte-identical to the head I verdicted: 3 files, docs/npu.md +25, tests/q4nx_t2s_test.py +24, tools/q4nx_t2s.py +56/−3).

Re-run at this head:

  • tests/q4nx_t2s_test.py -> 22 PASS, 0 failures, including the two rows that are the point of the change: v2 lane0/1 land on operand element 2b and v2 lane2/3 land on operand element 2*(64+b), plus v2 and v1 disagree only inside the code area (ms bytes identical).
  • Real tensor, not a synthetic one — ~/models/npu/bonsai2-pb16i-ternary/blk_0_ffn_gate_weight.pb16i, 50,135,040 B, 10,880 tiles (K=5120, N=17408):
    • v1 stream 23,674,880 B, and it is byte-identical to the arena artifact ~/wt/npu-arena/run-t2s/s1.B (2,367,880 B... 23,674,880 B, 0 differing bytes), so the tool reproduces a stream an independent path produced;
    • v2 stream the same size, differing from v1 in 21,938,181 bytes — the ordering really moved, and only in the code area;
    • decode_v2(v2) returns the image's codes exactly, and its multipliers equal the image's shared 128-K multipliers (ms[:, :, ::4, :]) exactly; 2.1250 bits/weight, 0.47222x of the pb16i bytes.

One correction to my own earlier note, so nobody chases it: the file that survives in ~/wt/npu-arena/run-t2s/ is the v1 stream (s1.B), and it matches v1 byte for byte — not v2. So this pass I can prove v1's reproduction against an artifact and v2's identity by decode, but there is no v2 arena artifact on the box to compare bytes against. Everything the PR claims about v2 is verified against the real tensor either way.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to a typo in the verdict just above: the arena artifact ~/wt/npu-arena/run-t2s/s1.B is 23,674,880 B and v1 matches it with 0 differing bytes (the "2,367,880 B" was a slip while writing the compare). Same conclusion, and the v2 figures in that review are unaffected.

@bong-water-water-bong
bong-water-water-bong merged commit 40d9632 into main Nov 2, 2026
8 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the npu/t2s-v2-format branch November 2, 2026 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 4/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant