From 88811d9624cfba4a4a894f76b4b1f11e364cfe85 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 10 Nov 2026 12:06:56 -0400 Subject: [PATCH 1/2] npu-pack: measure the tile bias slots the decoder skips instead of assuming they are zero (refs #1946) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decode_external_tile()` skips the `kk == 0` subvolumes because they are *"the tile's bias pair, all zero here"* -- a comment, not a measurement, and it is load-bearing: a non-zero bias in a slot nobody reads is an additive term missing from every projection of that tensor. Found while chasing a family whose imports are complete and whose output is fluent garbage (#1889), where the pack declares an fp32 bias for **every** projection and the import writes **0** bias tensors. Every tile now gets one check -- two 2,176 B subvolumes -- and the result is a field in the container's config: ``` bias_slots_note: "all 512 tile bias slot(s) read were zero, so skipping them is exact" bias_slots_note: " tile N bias subvolume S holds M of 2176 non-zero bytes, and the decoder skips it" ``` so any import of any pack states which it was, and a pack that does carry biases is a named fact rather than a silent omission. The first non-zero slot is kept, with the tensor name, because that is what a reader can act on. ## Arms `test_bias_slot_witness()`, driven on a fabricated tile (N=64, K=128 = one tile, four subvolumes), both ways: * an all-zero bias pair leaves the witness empty; * one non-zero byte at `nz[5]` in the `kk == 0` slot is **reported**, names the tensor (`zero-bias`), counts the non-zero bytes (`1 of …`), and the tiles looked at are counted. Mutation: making `note_bias_slot()` never witness fails those three arms. `all npu-pack checks passed`; copyright rc 0. Refs #1946, #1889. Note this only instruments the **external-tile** decoder; the `.fconst` path reads its bias pair into `TileW.bias` already, so the two paths differ in a way the next reader should know before concluding anything from a zero count. --- npu/pack_import.cpp | 38 ++++++++++++++++++++++++++++++++-- npu/pack_import.h | 7 ++++++- tests/npu_pack_import_test.cpp | 27 ++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 29b1decb6..16e3155e6 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -1778,7 +1778,28 @@ Q4 container_from_external_q4(const Q4& pack, long long* m_inexact) { return q; } -Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::string& err, bool row_major) { +// engine#1946: `kk == 0` is the tile's bias pair and the decoder below skips it, on the strength of a comment that says it is +// "all zero here". That assumption is load-bearing (a non-zero bias we never read is an additive term missing from every +// projection) and it was measured on exactly one subvolume of one pack. Every tile gets ONE check here -- two subvolumes of +// 2,176 B -- so any import of any pack says whether it held, instead of assuming it. +static std::string g_bias_witness; +static int64_t g_bias_tiles_checked = 0; +const std::string& bias_slot_witness() { return g_bias_witness; } +int64_t bias_slots_checked() { return g_bias_tiles_checked; } + +static void note_bias_slot(const char* who, int64_t tile, int slot, const uint8_t* p, int64_t n) { + if (!g_bias_witness.empty()) return; + int64_t nz = 0; + for (int64_t i = 0; i < n; ++i) + if (p[i]) ++nz; + if (nz == 0) return; + g_bias_witness = std::string(who ? who : "a projection") + " tile " + std::to_string(tile) + " bias subvolume " + + std::to_string(slot) + " holds " + std::to_string(nz) + " of " + std::to_string(n) + + " non-zero bytes, and the decoder skips it"; +} + +Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::string& err, bool row_major, + const char* who) { Q4 q; if (N <= 0 || K <= 0 || N % kExtTileRows != 0 || K % 128 != 0) { err = "external tile: N must be a multiple of 64 and K a multiple of 128 (got " + std::to_string(N) + " x " + @@ -1800,6 +1821,10 @@ Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::st q.scale.assign(size_t(N) * (K / 32), 0.0f); q.zero.assign(size_t(N) * (K / 32), 0.0f); for (int64_t tile = 0; tile < tiles; ++tile) { + // the two kk == 0 subvolumes are this tile's bias pair; the loop below starts at kk == 1 + for (int slot = 0; slot < 2; ++slot) + note_bias_slot(who, tile, slot, data + (tile * per_tile + slot) * kExtSubvBytes, kExtSubvBytes); + ++g_bias_tiles_checked; for (int half = 0; half < 2; ++half) { for (int64_t kk = 1; kk <= blocks; ++kk) { // kk == 0 is the tile's bias pair, all zero here const uint8_t* subv = data + (tile * per_tile + kk * 2 + half) * kExtSubvBytes; @@ -3378,7 +3403,8 @@ Report import_external(const std::string& pack_dir, const std::string& out_dir, if (!mul_ok(uint64_t(wt.length ? wt.length : wt.raw_len), 1, need) || need == 0) throw std::runtime_error(mm.weight + ": no external length recorded"); std::string derr; - const Q4 pack = decode_external_tile(bytes_of(wt, need), size_t(need), int(mm.N), int(mm.K), derr); + const Q4 pack = decode_external_tile(bytes_of(wt, need), size_t(need), int(mm.N), int(mm.K), derr, + kExtQuantsRowMajor, mm.weight.c_str()); if (!derr.empty()) throw std::runtime_error(mm.weight + ": " + derr); const Q4 conv = container_from_external_q4(pack); std::vector outs; @@ -4677,6 +4703,14 @@ Report import_pack(const std::string& pack_dir, const std::string& out_dir, cons const TextArtifacts ta = scan_text_artifacts(pack_dir); c["vocab_size"] = V; c["attention_bias"] = r.attn_bias_tensors > 0; // what the container actually carries + // engine#1946: the pack's bias TENSORS are not imported (the tile decoder skips the kk == 0 slot that holds them on + // the strength of "all zero here"), so say what the decoder found instead of leaving the assumption in a comment. + // Empty witness = every slot of every tile it read was all zero, which is the condition the skip is valid under. + if (!g_bias_witness.empty()) + c["bias_slots_note"] = g_bias_witness; + else if (g_bias_tiles_checked > 0) + c["bias_slots_note"] = "all " + std::to_string(g_bias_tiles_checked) + + " tile bias slot(s) read were zero, so skipping them is exact"; if (!r.head_synthesized_note.empty()) c["q4nx_head_note"] = r.head_synthesized_note; // The forward picks between "logits from the token embedding" and "logits from a separate head tensor" on this // key, and defaults to TIED when it is absent (npu/forward/src/q4nx_semantics.c). An untied pack that ships no diff --git a/npu/pack_import.h b/npu/pack_import.h index bb50c25ab..80359a7bc 100644 --- a/npu/pack_import.h +++ b/npu/pack_import.h @@ -397,7 +397,12 @@ bool external_container_tensors(const std::string& role, int nh, int nkv, int hd std::string& err); Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::string& err, - bool row_major = kExtQuantsRowMajor); + bool row_major = kExtQuantsRowMajor, const char* who = nullptr); + +// engine#1946: what the decoder found in the tile bias slots it skips. Empty = every slot it checked was all zero, which is +// the assumption `kk == 0` rests on; a non-empty string is the first slot that was not, with its tile and byte count. +const std::string& bias_slot_witness(); +int64_t bias_slots_checked(); Q4 decode_matmulnbits(const uint8_t* qweight, const float* scales, const uint8_t* zeros, int N, int K, int block, bool swap_nibbles = false, bool drop_zero = false); diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index b40a08fa1..e1e8cba67 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -3874,8 +3874,35 @@ void test_qk_gain_offsets() { check(n4.k_norm == 0 && n4.q_norm == 0, "qk gains: the byte count alone is not enough, head_dim has to agree"); } +// engine#1946: the external-tile decoder SKIPS the kk == 0 subvolumes because they are "the tile's bias pair, all zero +// here" -- an assumption in a comment, valid only while it holds. It now checks them and reports the first slot that is not +// zero, so an import says which it was. Driven here with both shapes, because a witness that fires on everything is as +// useless as one that never fires. +void test_bias_slot_witness() { + // N=64 rows, K=128: tiles = 1, blocks = 1, per_tile = 2*(1+1) = 4 subvolumes, each kExtSubvBytes + const int N = 64, K = 128; + const size_t bytes = size_t(N / 64) * size_t(2 * (K / 128 + 1)) * size_t(pi::kExtSubvBytes); + std::vector zero(bytes, 0); + for (size_t i = 2 * size_t(pi::kExtSubvBytes); i < bytes; ++i) zero[i] = uint8_t(i * 7 + 3); // the kk >= 1 payload + std::string err; + (void)pi::decode_external_tile(zero.data(), bytes, N, K, err, pi::kExtQuantsRowMajor, "zero-bias"); + check(err.empty(), "bias slot: a zero-bias tile decodes"); + check(pi::bias_slot_witness().empty(), + "bias slot: an all-zero bias pair leaves the witness empty (" + pi::bias_slot_witness() + ")"); + std::vector nz = zero; + nz[5] = 0x80; // one non-zero byte in the kk == 0 slot of tile 0 + (void)pi::decode_external_tile(nz.data(), bytes, N, K, err, pi::kExtQuantsRowMajor, "nonzero-bias"); + check(!pi::bias_slot_witness().empty(), "bias slot: a non-zero byte in a bias slot is REPORTED, not skipped silently"); + check(pi::bias_slot_witness().find("nonzero-bias") != std::string::npos, + "bias slot: the report names the tensor it was decoding"); + check(pi::bias_slot_witness().find("1 of ") != std::string::npos, + "bias slot: and counts the non-zero bytes rather than saying 'something'"); + check(pi::bias_slots_checked() >= 2, "bias slot: the tiles it looked at are counted"); +} + int main(int argc, char** argv) { test_qk_norm_refusal(); + test_bias_slot_witness(); test_qk_gain_offsets(); test_tail_head_placement(); const std::string golden_dir = argc > 1 ? argv[1] : ""; From b3d78c0de53c7695a4b268c5eb1e05dca4ce00e6 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 10 Nov 2026 17:43:35 -0400 Subject: [PATCH 2/2] npu-pack: reset the bias-slot witness between packs, so one import cannot report another's finding (review of #1966) The witness and its tile count are file statics and nothing cleared them, so `note_bias_slot()` returned early on a set witness and every later `import_pack()` in the same process wrote the FIRST pack's finding into its own container's `bias_slots_note` -- and counted the stranger's tiles in its "all N slots were zero". The reviewer built a harness against `libonebit_npu.a` showing exactly that: ``` after a pack with a non-zero bias: witness="pack-A.q_proj tile 0 ... holds 1 of 2176 non-zero bytes" checked=1 then a clean pack in the same process: witness="pack-A.q_proj tile 0 ... holds 1 of 2176 non-zero bytes" checked=2 ``` and it is already live in the test binary: `test_bias_slot_witness()` leaves the witness set to "nonzero-bias", so the pack leg and every external-data arm after it run with a false note in their containers. Nothing asserted on `bias_slots_note`, which is why the suite stayed green. `reset_bias_slot_witness()` is now exported (a caller that decodes several packs in one process needs it, and the test does), and `import_pack()` calls it before it decodes anything. Arms (in `test_bias_slot_witness()`, on the same fabricated tile): * a reset clears the witness and the count -- "the next pack starts clean"; * **decoding a clean tile after a dirty one leaves the witness empty** and counts only that decode's tile -- the case the reviewer's harness measured; * and the arm leaves the process reset, so the pack leg that follows is not carrying this test's finding. Mutation: making `reset_bias_slot_witness()` a no-op fails all four new checks by name. Verified: `npu_pack_import_test tests/golden` -> `all npu-pack checks passed`; mutation -> 4 FAIL; copyright rc 0. Refs #1946. --- npu/pack_import.cpp | 9 +++++++++ npu/pack_import.h | 3 +++ tests/npu_pack_import_test.cpp | 13 +++++++++++++ 3 files changed, 25 insertions(+) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index 970ad7088..70fee1d70 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -2125,6 +2125,13 @@ static std::string g_bias_witness; static int64_t g_bias_tiles_checked = 0; const std::string& bias_slot_witness() { return g_bias_witness; } int64_t bias_slots_checked() { return g_bias_tiles_checked; } +// engine#1946 review: the witness is process-global, so one import's finding would otherwise be written into the NEXT +// import's container as its own ("pack-A.q_proj … holds 1 of 2176 non-zero bytes" in pack B's config.json), and B's +// "all N slots were zero" count would include A's tiles. import_pack() calls this before it decodes anything. +void reset_bias_slot_witness() { + g_bias_witness.clear(); + g_bias_tiles_checked = 0; +} static void note_bias_slot(const char* who, int64_t tile, int slot, const uint8_t* p, int64_t n) { if (!g_bias_witness.empty()) return; @@ -4049,6 +4056,8 @@ Report import_external(const std::string& pack_dir, const std::string& out_dir, } Report import_pack(const std::string& pack_dir, const std::string& out_dir, const Options& opt) { + reset_bias_slot_witness(); // engine#1946 review: a process-global witness must not leak between imports + const Plan p = plan(pack_dir); // Census yes, writer no (see the note where graph_weights is decided): this genre's weights are 1-D external-data // blocks, and until the repacker exists an import would write a container nothing can read. Refused here rather diff --git a/npu/pack_import.h b/npu/pack_import.h index ab6aae8b6..7e3b9ac9c 100644 --- a/npu/pack_import.h +++ b/npu/pack_import.h @@ -547,6 +547,9 @@ Q4 decode_external_tile(const uint8_t* data, size_t bytes, int N, int K, std::st // the assumption `kk == 0` rests on; a non-empty string is the first slot that was not, with its tile and byte count. const std::string& bias_slot_witness(); int64_t bias_slots_checked(); +// engine#1946 review: both are process-global, so a caller that decodes more than one pack in a process must reset them +// between packs. `import_pack()` does it itself; a test or a batch tool that calls decode_external_tile() directly has to. +void reset_bias_slot_witness(); Q4 decode_matmulnbits(const uint8_t* qweight, const float* scales, const uint8_t* zeros, int N, int K, int block, bool swap_nibbles = false, bool drop_zero = false); diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 6ef2f36ee..33d27e50c 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -4811,6 +4811,19 @@ void test_bias_slot_witness() { check(pi::bias_slot_witness().find("1 of ") != std::string::npos, "bias slot: and counts the non-zero bytes rather than saying 'something'"); check(pi::bias_slots_checked() >= 2, "bias slot: the tiles it looked at are counted"); + + // engine#1946 review: the witness and the count are process-global, so the NEXT import used to inherit the last + // one's finding -- pack B's container recorded pack A's tensor as its own non-zero bias, and B's "all N slots were + // zero" count included A's tiles. Both are reset between packs now, and this pins that a reset is a reset. + pi::reset_bias_slot_witness(); + check(pi::bias_slot_witness().empty(), "bias slot: a reset clears the witness, so the next pack starts clean"); + check(pi::bias_slots_checked() == 0, "bias slot: and the count, so the next pack's note does not add a stranger's tiles"); + (void)pi::decode_external_tile(zero.data(), bytes, N, K, err, pi::kExtQuantsRowMajor, "clean-after-dirty"); + check(pi::bias_slot_witness().empty(), + "bias slot: decoding a clean pack after a dirty one leaves the witness empty (" + + pi::bias_slot_witness() + ")"); + check(pi::bias_slots_checked() == 1, "bias slot: and counts only this decode's tile"); + pi::reset_bias_slot_witness(); // leave the process clean for the pack leg that follows } /* engine#1985: a REGRESSION, not a new bug -- `Qwen3-1.7B` imported with 0 gaps on yesterday's main and refused with 280