[bong-water-water-bong]

This PR lands the pi agents' GGUF-on-NPU work from strixhalo (branch npu-glm-kb-fix in ~/1bit-engine-gguf-npu; 9 commits were local only), rebased onto main, plus one fix of mine. The agents' commits keep their own authorship.

Agents' commits (goals muhz0s39 / mulmuy25):

• a58c976, d22ef1a: GLM-4.7-Flash (deepseek2/MLA). The repack neutralises the inverted k_b transpose, and the forward re-syncs two MLA fixes: no expert transpose, and the v_b index. Each is checked element by element against the GGUF (correlation 0.9992/0.9969 against ~0 for the old indexing).
• 6484552, 9ad4fdf: Qwen3 MoE (Qwen3-Coder-30B-A3B) and qwen35moe (Qwen3.6-35B-A3B, GDN + MoE) on the per-op forward. MoE geometry is now parsed independently of MLA, and the shared expert gets its sigmoid gate.
• e5b9f38: the NPU serve takes /tmp/1bit-npu-device.lock, like the ELF runner, so two serves no longer fail on CREATE_HWCTX.
• 669c105: the repack records the GGUF arch for the provenance stamp check.
• 97a43e2: tokenizer. Accepts a Unigram tokenizer.json (array vocab, no merges) and strips SentencePiece's dummy-prefix space.
• 4784b06, b6b5864: registry rows for the six GGUF-on-NPU arches, plus tests/anchor_rerun.sh.
• 1f800d2: the LM head uses the widest K=H design, which fewer launches and also fixes a SIGFPE on MLA. Adds an NPU_PROFILE=1 dispatch profiler. Adds opt-in forward->lane routing (ONEBIT_NPU_LANE_DIR, off by default).

My commit: 97a43e2 stripped the leading space from every decode. Both servers decode one streamed token at a time, so every word-initial SentencePiece token lost its space: the agents' benchmark shows MiniCPM4 streaming "…-TheUnitedStates". decode(ids) still strips it for whole texts; decode(id), the streaming form, keeps it.

Evidence (captured by the agents on strixhalo, ~/evidence/gguf-npu-six/benchmark-2026-09-28.md): 1bit serve -m <gguf> --device npu answers " Paris" on all six arches (llama MiniCPM5-1B, qwen2 Qwen2.5-7B, qwen3moe Qwen3-Coder-30B-A3B, deepseek2 GLM-4.7-Flash, minicpm MiniCPM4-8B, qwen35moe Qwen3.6-35B-A3B). Speed is 0.006–0.17 tok/s, because it is still the per-op forward (~173 synchronous dispatches per token). The fused per-layer path is the agents' next goal.

Verified here: the rebase onto main is clean. ctest passes 12/12. The MiniCPM4 tokenizer golden (llama.cpp oracle ids) passes 5/5 against the repacked tokenizer.json.

Review notes, not blocking:

• The ~40-line MoE loader is duplicated in load_layer and load_gdn_layer.
• leading_dense_block_count now defaults to 0 when missing. GLM's config carries it, so GLM is unaffected.
• ONEBIT_NPU_LANE_DIR leaves an npu symlink in the repack cache dir. The GGUF path doesn't read that dir afterwards.

🤖 Generated with Claude Code

[context7]

Docs7 for 1bit-monster/engine

Result	Status	Action
Deployment	➖ Not used	—
Content review	➖ Did not run. This site has no agent runs available this month. Wait for the monthly reset or check your Docs7 plan.	—

Commit 7dfccad

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7dfccad)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The change to mla_attn in q4nx_forward.cpp modifies how k_b and v_b are indexed, which is critical for correctness. The new indexing logic assumes a flat order that reproduces the GGUF's flat order exactly, but this change could introduce a correctness issue if the tensor layout or indexing assumptions differ between the GGUF and the forward pass. This is especially concerning since the PR description mentions that the old indexing was "pinned" against a specific oracle, and the new logic explicitly states it was verified elementwise against the GGUF. // k_b: Q4NX tile grid [kv_lora][nope padded to 256], indexed flat[n*kv_lora+k]. // v_b: the Q4NX tiles v_b with cols=256 while its LOGICAL row is 512 wide, so // the per-head [512][256] tile grid is NOT the logical matrix. Decoding the // whole tensor at (rows=10240, cols=256) reproduces the GGUF's flat order // exactly (corr 0.996860 vs the 0.996870 a faithful Q4_1 requant scores), // i.e. the logical index is (h*256+n)*512 + k = out-major. Indexing flat // [k*vh+n] instead permuted the whole MLA value path; the earlier check that // "pinned" it compared a single SUM, which a permutation can still match. Possible Issue The MoE implementation in q4nx_forward.cpp now parses MoE parameters independently of MLA, which fixes a bug where MoE configs were not read for non-MLA models. However, the code introduces a new potential issue where the MoE parameters are read only once during initialization, and if the model's architecture changes dynamically (which is unlikely but possible), this could lead to incorrect behavior. This is a subtle correctness concern. // MoE params are parsed INDEPENDENTLY of mla_ so a non-MLA MoE arch (qwen3moe / // qwen35moe) gets its expert geometry too; reading them only inside `if (mla_)` // left n_expert_/top_k_/expert_im_ zero and the standard MoE path dead. // expert_im_ is the per-EXPERT intermediate, which c_.IM already holds for a MoE // config (the converter writes the per-expert FFN width as intermediate_size). n_expert_ = q4nx_config_int(cfg_path.c_str(), "n_expert", 0); if (n_expert_ > 0) { top_k_ = q4nx_config_int(cfg_path.c_str(), "top_k", 0); expert_im_ = c_.IM; dense_im_ = q4nx_config_int(cfg_path.c_str(), "dense_intermediate_size", 0); leading_dense_ = q4nx_config_int(cfg_path.c_str(), "leading_dense_block_count", 0); expert_gating_sigmoid_ = q4nx_config_int(cfg_path.c_str(), "expert_gating_func", 1) == 2; // 2 = sigmoid expert_w_scale_ = read_config_float(cfg_path, "\"expert_weights_scale\"", 1.0f); if (top_k_ <= 0 || expert_im_ <= 0) { fprintf(stderr, " [moe] config incomplete (n_expert=%d top_k=%d im=%d)\n", n_expert_, top_k_, expert_im_); err_ = "MoE config incomplete"; return false; } fprintf(stderr, " [moe] %sMoE: NH=%d n_expert=%d top%d im=%d dense_im=%d gating=%s\n", mla_ ? "MLA+" : "", c_.NH, n_expert_, top_k_, expert_im_, dense_im_, expert_gating_sigmoid_ ? "sigmoid" : "softmax"); } Possible Issue The mla_expert function in q4nx_forward.cpp has been changed to not transpose the expert weights, which was previously done. This change is claimed to be verified elementwise against the GGUF, but if the tensor layout or indexing assumptions are incorrect, this could lead to incorrect expert routing or incorrect computation of the MoE layer. The change is significant and should be carefully validated. // Do NOT transpose. The tile decode at (rows=K, cols=N) already reproduces the // GGUF's flat order, and that flat sequence IS the logical [N][K] = [out][in] // weight. Verified elementwise against the GGUF per expert: reshape(N,K) gives // corr 0.999198 (gate) / 0.996949 (down), while transposing gives -0.001079 / // 0.000417 -- i.e. the transpose destroyed the expert entirely, making every // routed expert's contribution ~10-20x wrong and flipping the top-k selection. std::copy(kn.begin(), kn.end(), out.begin()); Possible Issue The run_lm_head function in q4nx_forward.cpp now selects the widest design with K=H for the head, which is intended to reduce the number of launches. However, this change could potentially lead to performance degradation if the selected design is not optimal for the specific model or if the number of tiles is not well-balanced. This change should be validated with performance benchmarks. // Reuse an existing GEMM geometry for the head (the variable keeps its old name // for the block below). QKV when the arch has one; the MLA/deepseek2 arch // (GLM-4.7-Flash) has QA/QB/KVA/O instead, so idx_of(QKV) returned -1 and T was // read out of designs_[-1] -- garbage, and the ntiles division below raised // SIGFPE. Only this NPU path uses T (the host path is a plain matvec), which is // why GLM crashed here the first time the ELF path was exercised. // Pick the WIDEST design with K=H for the head. Each tile is a separate // synchronous launch (~one NPU round-trip each, ~constant regardless of N), // so fewer/larger tiles mean fewer launches for the same math. int i_qkv = -1; for (int d = 0; d < ndesigns_; d++) { if (designs_[d].K != H || designs_[d].N <= 0) continue; if (i_qkv < 0 || designs_[d].N > designs_[i_qkv].N) i_qkv = d; } if (i_qkv < 0) { err_ = "LM head: no GEMM design with K=H available"; return false; } const int T = designs_[i_qkv].N; Possible Issue The NPU device lock mechanism in forward_serve.cpp uses flock with LOCK_EX | LOCK_NB and falls back to LOCK_EX if the first attempt fails. This could lead to a race condition where two processes might attempt to acquire the lock simultaneously, potentially causing one to wait indefinitely or fail to acquire the lock. This is a critical issue for concurrent access to the NPU device. if (!std::getenv("NPU_NO_DEVICE_LOCK")) { // flock needs only a read fd, so the file stays 0644 and other users can // still open it; O_NOFOLLOW refuses a symlink planted in /tmp. const char* lk = "/tmp/1bit-npu-device.lock"; int lfd = ::open(lk, O_CREAT | O_RDONLY | O_NOFOLLOW | O_CLOEXEC, 0644); if (lfd >= 0) { if (::flock(lfd, LOCK_EX | LOCK_NB) != 0) { std::fprintf(stderr, "[npu] waiting for the exclusive device lock (%s)...\n", lk); if (::flock(lfd, LOCK_EX) != 0) std::fprintf(stderr, "[npu] device lock unavailable; continuing unlocked\n"); else std::fprintf(stderr, "[npu] device lock acquired\n"); } } } Possible Issue The repack_gguf.py script introduces a new mechanism to patch the converter for specific architectures like deepseek2. The patching logic modifies the converter's source code, which could introduce subtle bugs or inconsistencies if the patching is not done correctly or if the patching logic itself has errors. This is a high-risk change that should be carefully validated. _kb_file = os.path.join(converter_dir, "q4nx", "models", _arch + ".py") if _arch == "deepseek2" and os.path.exists(_kb_file): try: if 'transpose_slice = "k_b_proj" in nm' in open(_kb_file).read(): _kb_patch = True except Exception: pass if (_t and _t != "Q4_1") or _kb_patch: import shutil as _shutil, tempfile as _tempfile run_dir = _tempfile.mkdtemp(prefix="q4nx-converter-") _shutil.copytree(converter_dir, run_dir, dirs_exist_ok=True) if _t and _t != "Q4_1": _c["default_tensor_type"] = "Q4_1" with open(os.path.join(run_dir, "configs", _arch + ".json"), "w") as f: json.dump(_c, f, indent=4) print(f"repack: {_arch} config asks for default_tensor_type={_t}; the NPU " f"forward reads Q4_1 tiles, so the converter runs from a patched " f"copy ({run_dir})") if _kb_patch: _p = os.path.join(run_dir, "q4nx", "models", _arch + ".py") _s2 = open(_p).read().replace( 'transpose_slice = "k_b_proj" in nm', 'transpose_slice = False # engine: k_b packs as-is -> 12 tiles/head') with open(_p, "w") as f: f.write(_s2) print(f"repack: {_arch} k_b transpose is inverted (gives 16 tiles/head, the " f"forward reads 12); neutralised in the patched copy ({run_dir})")

[github-actions]

Persistent review updated to latest commit 7dfccad

[bong-water-water-bong]

Post-merge note: scripts/repack_gguf.py still has the source-text k_b workaround (_kb_patch, matching 'transpose_slice = "k_b_proj" in nm'). The converter reverted that transpose at the source (5fc52149b), so the block is dead code today. It would silently rewrite the converter again if that line ever comes back for another reason. Please remove it in a follow-up and rely on a load-time tile-count check instead.