Skip to content

npu/forward: drop the unused fused-GU host path and kernel-internal comments - #211

Merged
bong-water-water-bong merged 2 commits into
mainfrom
npu/interface-comments
Sep 29, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
npu/interface-comments

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Follow-up to #179/#181/#209. The NPU host code on main carried the private NPU kernels' internals in public: the fused int4 GU kernel's tile layout, which byte regions the object-FIFO delivers, the on-chip dequant stage, the SRAM budget, REG_M tiling limits, generator versions and private issue numbers. Owner rule: NPU kernels, their designs and detailed write-ups stay private.

Removed (dead code on the public route; no callers outside npu_engine_i8ctx_inc.h):

  • npu/forward/engine-src/gu_i4_pack.h and q4nx_raw.h (the fused int4 GU packer and its raw reader)
  • the fused GU→SiLU→D block in npu_engine_i8ctx_inc.h: launch_fused, dequant_fused, fused_elf_capable, packB_into_fused*, update_fused_header*, fused_gs_off and its constants
  • the unused bf16_pair field

Reworded: the remaining comments that named kernel internals now state only the host-side contract (M=128 streams, one instruction BO per context, zero-padded rows).

13 insertions, 1,027 deletions. No behaviour change: q4nx_forward.cpp, the only file including the header, compiles cleanly against XRT's headers on strixhalo (g++ -std=c++20 -fsyntax-only). The private fused path stays in npu-kernels. Git history still holds the removed text.

🤖 Generated with Claude Code

…omments

The public forward never calls the fused GU->SiLU->D path (launch_fused,
dequant_fused, packB_into_fused*, update_fused_header*, fused_gs_off) or the
int4 GU packer (gu_i4_pack.h, q4nx_raw.h): they had no callers outside
npu_engine_i8ctx_inc.h. Those files and blocks described the private NPU
kernels' tile formats, so they go, along with the unused bf16_pair field.
Comments that named private kernel internals (on-chip regions, SRAM budget,
REG_M tiling, generator versions, private issue numbers) now state only the
host-side contract. No behaviour change for the GGUF-on-NPU route.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@context7

context7 Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

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 8a1ec60

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 8a1ec60)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

179 - Partially compliant

Compliant requirements:

  • GGUF-on-NPU serving route implemented
  • Model repacking to Q4NX format
  • Support for Qwen2.5-7B, MiniCPM4-8B, MiniCPM5-1B
  • Verification on real NPU with correct token outputs
  • Fixes for MiniCPM4-8B including embedding_scale, residual_scale, logit_scale, LongRoPE
  • Repack guard implemented to fail on critical scale-like scalars
  • Cache-hit validation added
  • logit_scale fidelity confirmed

Non-compliant requirements:

  • GLM-4.7-Flash (deepseek2, MLA + MoE) support not claimed
  • MiniCPM4-8B tokenizer fallback to bytes due to BPE-only restriction
  • Foreign Q4NX directory trust not addressed
  • Provenance stamp not implemented

Requires further human verification:

  • Verification of GLM-4.7-Flash support
  • Verification of MiniCPM4-8B tokenizer behavior
  • Verification of foreign Q4NX directory trust behavior

181 - Partially compliant

Compliant requirements:

  • Prefer canonical npu-infer converter
  • Forward re-sync from canonical npu-infer forward
  • Fix streaming path's dangling capture

Non-compliant requirements:

  • None

Requires further human verification:

  • None

209 - Partially compliant

Compliant requirements:

  • Support for all six architectures
  • GLM-4.7-Flash fixes
  • Qwen3 MoE and qwen35moe support
  • NPU serve takes /tmp/1bit-npu-device.lock
  • Repack records GGUF arch for provenance stamp
  • Tokenizer accepts Unigram tokenizer.json
  • Registry rows for six GGUF-on-NPU arches
  • Fix leading space stripping in streaming decode

Non-compliant requirements:

  • None

Requires further human verification:

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

Dead Code Removal

The PR removes dead code related to fused int4 GU kernel functionality, including gu_i4_pack.h, q4nx_raw.h, and associated functions like launch_fused, dequant_fused, fused_elf_capable, etc. This removal is correct as these are internal kernel details not needed in the public interface.

        return true;
    }

    // ── M is ALWAYS 128 — there is no valid M<128 instruction stream ──
    // The kernels are built for M=128, so gemm_generate_sequence_i8 ignores
    // its M argument: the stream depends only on (K, N). A stream built for a
    // smaller M does not complete.
    //
    // Smaller batches MUST reuse the M=128 stream and pass the real row count
    // as `am` to go()/launch_*: quantize_async zero-pads rows [am, 128) so only
    // rows [0, am) are valid. That is what npu_engine_universal, zaya_decode
    // and zaya_npu_runner do for single-token decode. Real small-M streams
    // require per-shape small-M xclbins (build_xclbins.sh Peano path), not a
    // runtime regen. (A former regen_insts(int M) re-uploaded an identical
    // M=128 stream and claimed to resize the batch — removed as misleading.)
#else
    // Stub: npu_instr_utils.hpp not available — use init() with pre-gen'd files
    bool init_with_generator(xrt::device&, const char*, int, int, int, int) {
        fprintf(stderr, "  I8Ctx: init_with_generator unavailable (no npu_instr_utils)\n");
        return false;
    }
#endif

    // ── Init: consume a FULL ELF directly (no xclbin) ──
    // Kernel signature: (bo0, bo1, bo2) == (A, B, C). No instruction BO: the
    // TXN control stream lives in the ELF's .ctrltext. Kernel name is
    // "<kernel>:<instance>" from the aiecc full-ELF config ("main:seq" for the
    // v27 designs).
    bool init_elf(xrt::device& d, const char* ep, int M, int K, int N,
                  int nlayers, const char* kname = "main:seq") {
        MD = M; KD = K; ND = N; NL = nlayers;
        fprintf(stderr, "  I8Ctx::init_elf ep=%s M=%d K=%d N=%d nl=%d\n",
Comment Updates

Comments that previously referenced internal kernel details have been updated to reflect only the host-side contract, such as M=128 streams and zero-padded rows. This improves clarity and prevents confusion about internal implementation details.

int bC_nd = 0;   // bo2 (C2) size override: MD*bC_nd*4 bytes when > 0, for a
                 // kernel whose output is wider than the logical ND.
std::unique_ptr<xrt::xclbin> xc;

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 8a1ec60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant