Repository navigation
npu forward: refuse a launch whose buffer is narrower than the ELF's declared argument (closes #868) - #869
Conversation
…declared argument A full ELF states, in its .dynsym, the byte width of every kernel argument (the driver patches those very symbols), and the kernel's shim DMA touches exactly that much. A launcher that passes a narrower buffer does not fail: the DMA walks past the mapping, the IOMMU logs IO_PAGE_FAULTs, and the run returns garbage. Measured on 2026-11-02 with an 896-hidden OGA pack: its ELF family is (weights, activations, output) = (655,360, 2,048, 4,096) B, the int8 launcher passes (MD*KD, KD*ND, MD*ND*4) = (16,384, 1,048,576, 65,536) B at M=16, so 639 KB of weight stream was read past a 16 KB activation BO. Three IO_PAGE_FAULT bursts, no amdxdna ERROR, no TDR, and a wrong answer that looked like a model problem. - q4nx_elf_arg_sizes() (npu/forward/src/q4nx_pack.c) reads the three widths out of the artifact: 32-bit LE ELF, section headers, SHT_DYNSYM, STT_OBJECT symbols named "0".."2". Returns how many it found, or -1 for a non-ELF / no .dynsym, zeroing out[] first. - I8Ctx::init_elf() refuses the load when a declared width exceeds the buffer the launch would pass, printing both numbers, the geometry and the argument's role. One-directional by design: a declared width may be smaller than the buffer (the tile-family ELFs address a tile of a larger BO through the control text) but never larger -- so the tile family every validated model uses is unaffected, and the check cannot fire on a working geometry. tests/npu_elf_args_test.c builds the ELF32 fixtures by hand (no aiecc, no model, no device): the attention-ELF widths, a narrow tile-family ELF, and the three refusals; added to the CI target list so ctest does not report it "Not Run". Not fixed here, and the reason this is a gate rather than a launcher: driving this ELF family needs (weights, activations, output) with the per-core weight stream layout forge_bench uses, which is a separate change with its own evidence.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 49e04750d, + tprs-ready. The parser reproduces on the real artifacts, and no family the engine
drives today can be refused by it — checked with numbers, below.
Built and run here (GCC, thermal-run -j4):
plain Release ninja npu_elf_args_test npu_plan_test -> clean
ctest -R "npu_elf_args|npu_plan" -> 3/3 passed
./npu_elf_args_test 8/8 "ok" lines, "all cases ok", rc 0
ONEBIT_NPU=ON, own runtime ninja 1bit -> 40/40, links
— the last one matters because args_fit_elf() lives in npu_engine_i8ctx_inc.h, which a plain build never compiles.
It compiles, uses q4nx_elf_arg_sizes through the header's extern "C", and the three "passed" byte counts are the
real BO sizes, not estimates: bA = mkbo(MD*KD) (int8), layerB[l] = mkbo(w_bytes()), bC = mkbo(MD*ND*4*(pb16i?2:1)).
I re-derived the parser's numbers myself — a 20-line harness against the PR's own q4nx_pack.c, on the box's real
ELFs, cross-checked against readelf -sW … | awk '$4=="OBJECT"':
| ELF | q4nx_elf_arg_sizes |
readelf |
|---|---|---|
attn_mha_1024_nh16.elf (our xclbins) |
ret=-1 — no symbols named 0/1/2 (they are 3,4,5) |
agrees |
bonsai2-27b-NPU/kernels-m16/full_i8_D_K17408_N5120_m16.elf |
(512, 1024, 4096) |
agrees |
oga4/kernels-m16/full_i8_O_K1024_N1024.elf |
(163,840, 2,048, 1,024) |
agrees |
…/full_i8_GU_K1024_N9728_m16.elf |
(1,556,480, 2,048, 9,728) |
agrees |
…/full_i8_LMHEAD_K1024_N152064_m16.elf |
(24,330,240, 2,048, 152,064) |
agrees |
the same five values the body lists, from the artifacts rather than from the description.
The "nothing that works today is refused" claim, checked against every ELF family on this box. I swept the ELFs the
engine actually loads:
~/1bit-MONSTER-iso-build/engine/npu/xclbins 48 ELFs 0=512 1=1024 2=4096
~/models/npu/bonsai2-27b-NPU/kernels 14 ELFs 0=512 1=1024 2=4096
~/models/npu/bonsai2-27b-NPU/kernels-m16 7 ELFs 0=512 1=1024 2=4096
~/qdq-elfs-m32 5 ELFs 0=512 1=1024 2=4096
~/ogaload/oga4/kernels-m16 10 ELFs 0=0x28000 … 0x1734000, 1=2048/9728, 2=1024/9728/0x25200
74 of the 74 ELFs the int8 path drives declare the tile triple (512, 1024, 4096), and the smallest buffers the launcher
can pass are (MD*KD, w_bytes(), MD*ND*4) = (1024, …, 4096) at M=1, K=1024, N=1024 — i.e. equal or larger at every
index, and the comparison is declared > passed -> refuse. At M=16/K=1024 it is (16,384, 1,048,576, 65,536), 10x to
1,024x clear. The OGA family is the only one that trips, and it trips on argument 0 by 10x-1,480x, which is exactly the
measured fault. The 27B container's own kernel sets are tile-family, so the 27B device path is untouched.
Also worth recording, because it bounds what this gate can ever catch: our attn_mha_* ELFs export their objects as
3,4,5, not 0,1,2, so q4nx_elf_arg_sizes returns -1 there and the gate is a no-op for that family. A future
ELF whose arguments are not named by index will be skipped silently; that is the right default (refuse only on evidence),
but it means the gate's coverage is "aiecc full ELFs with indexed argument symbols", not "every ELF".
Nits, none a gate:
- The printed role names are the launcher's, not the ELF's.
kWhat = {"activations","weights","output"}describes
what this launcher passes at index 0/1/2, while the OGA family declares the weight stream first (163,840 B at arg 0,
and 2,048 = 2xK at arg 1 is the activations). The comparison by index is correct and that is what the driver patches, so
the refusal is right — but the message reads "activations declares argument 0 (activations) = 163840 B", which points a
reader at the wrong buffer. "argument 0 (this launcher passes its activations buffer here)" would say it without
implying the ELF's own roles. q4nx_pack.hsays the return is "the number of widths found (0..3) or -1";found ? found : -1can never return 0, so
it is 1..3 or -1. Worth fixing in the comment, since the caller's-1test is load-bearing.args_fit_elf()returnstruewhen the ELF cannot be read at all (fopenfailure, short read) — the right choice, but
it is a deliberate hole and the comment does not say so while it does say why the width comparison is one-directional.
CI at the time I write this: build/CodeQL still in flight (no conclusion yet), hw_gate SKIPPED. Nothing red.
…ain (#878) #869 gave I8Ctx::args_fit_elf() a call to q4nx_elf_arg_sizes() and the ONNX EP's NPU leg broke twice: the inc header never declared it (npu/forward/include/q4nx_pack.h does, and only the forward route happened to include it first), and onnx/ep's _npu_src list never compiled the file that defines it (npu/forward/src/q4nx_pack.c), so adding the declaration alone moves the failure from compile to link. Both lines are here; -DONEBIT_EP_NPU=ON builds 9 targets again and the forward route is unchanged. closes #874 Co-authored-by: pi-qdq <pi-qdq@1bit.gg>
…nels) — #855's launcher ABI The OGA pack's kernels are a different launch family from the int8 per-tile one I8Ctx was written for: their runtime sequence declares (arg0 = the tensor's whole q4_1 tile stream, arg1 = K bf16 activations, arg2 = N f32 outputs), and its .dynsym symbols are per core, so arg0 and arg2 are 1/cores of the stream and of the output while arg1 is the whole activation row. Driving them with the int8 launcher is what made every array run of this pack either fault (IO_PAGE_FAULT bursts, 2026-11-02 20:35) or come back degenerate. The family is detected from the artifact, not from a config knob: q4nx_gemv_family() decides from the three declared widths (the same artifact-level information #869's guard reads), and I8Ctx then * sizes the activation BO K*2 (bf16) and the output BO N*4 (f32), * takes the weight BO as the tensor's own 5,120-byte q4_1 tiles ((K/256)*(N/32) of them), * launches (weights, activations, outputs), * converts the activation row f32 -> bf16 (round-to-nearest-even) and reads the result back as f32, skipping the int32 canary (its sentinel would land in an f32 buffer). The forward builds that weight image by copying the container's chunks -- row-tile-major, k-chunk inner, the order the vendor packer and the bit-exact 2026-11-01 bench both use -- so every tile keeps its own bf16 scale and zero; nothing is re-quantised. Fused designs (QKV, gate|up) concatenate their parts in the design's N order and pad the tail with zero row-tiles. The head uses its own tile tensor; a tied bf16 table has no tile stream and is refused by name rather than quantised here. The family has no M dimension (its _m16 ELFs are byte-identical to the one-row design), so npu_gemm_rows/run_lm_head_rows fall back to per-row launches: correct, and slower for prefill -- that is #671 item 2's m128 work, stated rather than hidden. Device-free verification, because the rule is where the first attempt went wrong: * q4nx_gemv_family() unit cases in tests/npu_elf_args_test.c: the five pack shapes classified from their measured declarations (QKV 245,760/2,048/1,536 at K=1024 N=1536 ... head 24,330,240/2,048/ 152,064), plus six near misses that must not match -- the int8 per-tile family (512/1,024/4,096), a full-width stream, a byte-count output, int8-width activations, K off the 256 grid, a zero stream. 11 declarations, all classified correctly. * the same rule run over the real artifacts (a harness calling the exact function init_elf uses): all five pack ELFs -> gemv, the 27B's int8 per-tile ELF -> not gemv. * ctest 5/5 (npu_elf_args, npu_design_grid, npu_forward_loader, npu_pack_import, npu_q4nx), copyright.py --check rc 0, and the host-GEMM path unchanged on the corrected container ("The capital of France is Paris.", AMD's own oracle string). Depends on #885 for the batched row strides; stacked on it rather than on main because the per-row fallback fills the same M*N buffers #885 fixed the callers to read.
…nels) — #855's launcher ABI The OGA pack's kernels are a different launch family from the int8 per-tile one I8Ctx was written for: their runtime sequence declares (arg0 = the tensor's whole q4_1 tile stream, arg1 = K bf16 activations, arg2 = N f32 outputs), and its .dynsym symbols are per core, so arg0 and arg2 are 1/cores of the stream and of the output while arg1 is the whole activation row. Driving them with the int8 launcher is what made every array run of this pack either fault (IO_PAGE_FAULT bursts, 2026-11-02 20:35) or come back degenerate. The family is detected from the artifact, not from a config knob: q4nx_gemv_family() decides from the three declared widths (the same artifact-level information #869's guard reads), and I8Ctx then * sizes the activation BO K*2 (bf16) and the output BO N*4 (f32), * takes the weight BO as the tensor's own 5,120-byte q4_1 tiles ((K/256)*(N/32) of them), * launches (weights, activations, outputs), * converts the activation row f32 -> bf16 (round-to-nearest-even) and reads the result back as f32, skipping the int32 canary (its sentinel would land in an f32 buffer). The forward builds that weight image by copying the container's chunks -- row-tile-major, k-chunk inner, the order the vendor packer and the bit-exact 2026-11-01 bench both use -- so every tile keeps its own bf16 scale and zero; nothing is re-quantised. Fused designs (QKV, gate|up) concatenate their parts in the design's N order and pad the tail with zero row-tiles. The head uses its own tile tensor; a tied bf16 table has no tile stream and is refused by name rather than quantised here. The family has no M dimension (its _m16 ELFs are byte-identical to the one-row design), so npu_gemm_rows/run_lm_head_rows fall back to per-row launches: correct, and slower for prefill -- that is #671 item 2's m128 work, stated rather than hidden. Device-free verification, because the rule is where the first attempt went wrong: * q4nx_gemv_family() unit cases in tests/npu_elf_args_test.c: the five pack shapes classified from their measured declarations (QKV 245,760/2,048/1,536 at K=1024 N=1536 ... head 24,330,240/2,048/ 152,064), plus six near misses that must not match -- the int8 per-tile family (512/1,024/4,096), a full-width stream, a byte-count output, int8-width activations, K off the 256 grid, a zero stream. 11 declarations, all classified correctly. * the same rule run over the real artifacts (a harness calling the exact function init_elf uses): all five pack ELFs -> gemv, the 27B's int8 per-tile ELF -> not gemv. * ctest 5/5 (npu_elf_args, npu_design_grid, npu_forward_loader, npu_pack_import, npu_q4nx), copyright.py --check rc 0, and the host-GEMM path unchanged on the corrected container ("The capital of France is Paris.", AMD's own oracle string). Depends on #885 for the batched row strides; stacked on it rather than on main because the per-row fallback fills the same M*N buffers #885 fixed the callers to read.
…nels) — #855's launcher ABI (#891) The OGA pack's kernels are a different launch family from the int8 per-tile one I8Ctx was written for: their runtime sequence declares (arg0 = the tensor's whole q4_1 tile stream, arg1 = K bf16 activations, arg2 = N f32 outputs), and its .dynsym symbols are per core, so arg0 and arg2 are 1/cores of the stream and of the output while arg1 is the whole activation row. Driving them with the int8 launcher is what made every array run of this pack either fault (IO_PAGE_FAULT bursts, 2026-11-02 20:35) or come back degenerate. The family is detected from the artifact, not from a config knob: q4nx_gemv_family() decides from the three declared widths (the same artifact-level information #869's guard reads), and I8Ctx then * sizes the activation BO K*2 (bf16) and the output BO N*4 (f32), * takes the weight BO as the tensor's own 5,120-byte q4_1 tiles ((K/256)*(N/32) of them), * launches (weights, activations, outputs), * converts the activation row f32 -> bf16 (round-to-nearest-even) and reads the result back as f32, skipping the int32 canary (its sentinel would land in an f32 buffer). The forward builds that weight image by copying the container's chunks -- row-tile-major, k-chunk inner, the order the vendor packer and the bit-exact 2026-11-01 bench both use -- so every tile keeps its own bf16 scale and zero; nothing is re-quantised. Fused designs (QKV, gate|up) concatenate their parts in the design's N order and pad the tail with zero row-tiles. The head uses its own tile tensor; a tied bf16 table has no tile stream and is refused by name rather than quantised here. The family has no M dimension (its _m16 ELFs are byte-identical to the one-row design), so npu_gemm_rows/run_lm_head_rows fall back to per-row launches: correct, and slower for prefill -- that is #671 item 2's m128 work, stated rather than hidden. Device-free verification, because the rule is where the first attempt went wrong: * q4nx_gemv_family() unit cases in tests/npu_elf_args_test.c: the five pack shapes classified from their measured declarations (QKV 245,760/2,048/1,536 at K=1024 N=1536 ... head 24,330,240/2,048/ 152,064), plus six near misses that must not match -- the int8 per-tile family (512/1,024/4,096), a full-width stream, a byte-count output, int8-width activations, K off the 256 grid, a zero stream. 11 declarations, all classified correctly. * the same rule run over the real artifacts (a harness calling the exact function init_elf uses): all five pack ELFs -> gemv, the 27B's int8 per-tile ELF -> not gemv. * ctest 5/5 (npu_elf_args, npu_design_grid, npu_forward_loader, npu_pack_import, npu_q4nx), copyright.py --check rc 0, and the host-GEMM path unchanged on the corrected container ("The capital of France is Paris.", AMD's own oracle string). Depends on #885 for the batched row strides; stacked on it rather than on main because the per-row fallback fills the same M*N buffers #885 fixed the callers to read. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
…N stride (engine#855 part 2) (#885) * npu forward: read the batched prefill's GEMM outputs at the kernel's N stride (engine#855 part 2) #854 derives each GEMM design's N on the kernel's column grid (q4nx_pad_n, 32*4*cols = 512), so npu_gemm_rows() writes M rows of the *design's* N. The batched prefill read those buffers back at the REAL width instead -- O[m*H], D[m*H], gv/uv[m*im], ACT[m*2*im] -- so row 0 was right and every later row was read k*(padded-real) floats early. On a prompt of >1 token that corrupts every position after the first, which is exactly the shape of #855: with the container's contents fixed (#849), position 0 matched a numpy reference to 6 digits and every later position did not. Models whose hidden/intermediate are already on the grid (Qwen3-0.6B/1.7B/4B, the 27B) are unaffected, because for them q4nx_pad_n() is the identity -- which is why this went unseen. Measured on the OGA 0.5B container (AMD's pack), host-GEMM, greedy, raw completion "The capital of France is": batched (default) "意为:" / "意为: - 带有" <- before batched, this commit " Paris. The capital of the United States is Washington" sequential (NPU_NO_BATCH=1) " Paris. The capital of the United States is Washington" and the chat prompt "What is the capital of France?" returns AMD's own oracle string, "The capital of France is Paris." The engine's own diagnostic agrees: NPU_BATCH_CHECK=1 printed `seq_argmax=12095 batch_argmax=36589 MISMATCH` before, and the two paths return the same text now. * npu forward: the QKV2 split reads its four GEMMs at the design's N too The same class as the previous commit, in the path a QKV2 pack takes: qh0/qh1/kvk/kvv are written by npu_gemm_rows() as M rows of their design's N (padded, engine#854) but were read at the real widths nhd/nkv, so row 0 was right and every later row was shifted. The size guard follows the buffers to the same stride, so a short return is still caught. Correct by construction, and the identity whenever nothing is padded: the design's N is literally the row width npu_gemm_rows() assigned, so this cannot be wrong for an on-grid geometry (it compiles to the previous arithmetic there). No QKV2 pack is imported on this box, so this one is reasoning + the existing tests (4/4) rather than an end-to-end text, and that is stated rather than glossed. * npu forward: the split gate/up branch packs ACT at the fused stride too (review of #885) ACT is read once, after the if/else, at the fused design's N (pad_n(2*im)) -- but it reached that read from two branches with different layouts: the fused branch by construction (npu_gemm_rows writes M rows of the design's N), and the split branch via a buffer packed at the REAL width (2*im). With pad_n(2*im) > 2*im that read puts row m at m*(act_w - 2*im) floats past its data and the last row past the end of the vector. Latent rather than live on this box, which is why every test and the OGA text stayed green: every intermediate_size in the containers here is 512-aligned (2*im = 1536/6144/7168/34816, and the pack's 4864), so act_w == 2*im. The first container with im % 256 != 0 takes the split branch and gets shifted rows plus an out-of-bounds read. Fixed by giving both branches the same layout instead of teaching the read two strides: the split branch now sizes its packed buffer at act_w (zero-filled, so the pad columns match what assemble_B leaves in the fused case) and indexes it at act_w. The comment says which layout it is describing rather than pointing at O/D, whose reason does not hold for a swapped buffer. * npu forward: drive the forge q4_1 GEMV ELF family (the pack's own kernels) — #855's launcher ABI (#891) The OGA pack's kernels are a different launch family from the int8 per-tile one I8Ctx was written for: their runtime sequence declares (arg0 = the tensor's whole q4_1 tile stream, arg1 = K bf16 activations, arg2 = N f32 outputs), and its .dynsym symbols are per core, so arg0 and arg2 are 1/cores of the stream and of the output while arg1 is the whole activation row. Driving them with the int8 launcher is what made every array run of this pack either fault (IO_PAGE_FAULT bursts, 2026-11-02 20:35) or come back degenerate. The family is detected from the artifact, not from a config knob: q4nx_gemv_family() decides from the three declared widths (the same artifact-level information #869's guard reads), and I8Ctx then * sizes the activation BO K*2 (bf16) and the output BO N*4 (f32), * takes the weight BO as the tensor's own 5,120-byte q4_1 tiles ((K/256)*(N/32) of them), * launches (weights, activations, outputs), * converts the activation row f32 -> bf16 (round-to-nearest-even) and reads the result back as f32, skipping the int32 canary (its sentinel would land in an f32 buffer). The forward builds that weight image by copying the container's chunks -- row-tile-major, k-chunk inner, the order the vendor packer and the bit-exact 2026-11-01 bench both use -- so every tile keeps its own bf16 scale and zero; nothing is re-quantised. Fused designs (QKV, gate|up) concatenate their parts in the design's N order and pad the tail with zero row-tiles. The head uses its own tile tensor; a tied bf16 table has no tile stream and is refused by name rather than quantised here. The family has no M dimension (its _m16 ELFs are byte-identical to the one-row design), so npu_gemm_rows/run_lm_head_rows fall back to per-row launches: correct, and slower for prefill -- that is #671 item 2's m128 work, stated rather than hidden. Device-free verification, because the rule is where the first attempt went wrong: * q4nx_gemv_family() unit cases in tests/npu_elf_args_test.c: the five pack shapes classified from their measured declarations (QKV 245,760/2,048/1,536 at K=1024 N=1536 ... head 24,330,240/2,048/ 152,064), plus six near misses that must not match -- the int8 per-tile family (512/1,024/4,096), a full-width stream, a byte-count output, int8-width activations, K off the 256 grid, a zero stream. 11 declarations, all classified correctly. * the same rule run over the real artifacts (a harness calling the exact function init_elf uses): all five pack ELFs -> gemv, the 27B's int8 per-tile ELF -> not gemv. * ctest 5/5 (npu_elf_args, npu_design_grid, npu_forward_loader, npu_pack_import, npu_q4nx), copyright.py --check rc 0, and the host-GEMM path unchanged on the corrected container ("The capital of France is Paris.", AMD's own oracle string). Depends on #885 for the batched row strides; stacked on it rather than on main because the per-row fallback fills the same M*N buffers #885 fixed the callers to read. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Closes #868.
The fault this prevents
On 2026-11-02 an OGA-pack container ran on the array through our own runtime (
npu_rt, no XRT) and, while returning garbage, made the kernel log threeIO_PAGE_FAULTbursts (10 entries each, one per 4 KB page). NoamdxdnaERROR, no TDR, and nothing in the run's output said "buffer too small".Cause, measured: a full ELF declares each argument's byte width in
.dynsym— the very symbols the driver patches — and the kernel's shim DMA touches exactly that much. This pack's ELF family is(weights, activations, output):so 639 KB of weight stream was read from inside a 16 KB activation BO.
forge_bench— which passes(w, x, y)in exactly the declared widths — is bit-exact on the same files, which is the cross-check that made it unambiguous.What this changes
q4nx_elf_arg_sizes()(npu/forward/src/q4nx_pack.c+q4nx_pack.h): reads the three widths out of the artifact (32-bit LE ELF → section headers →SHT_DYNSYM→STT_OBJECTsymbols named"0".."2"). Returns how many it found,-1for a non-ELF / no.dynsym, zeroingout[]first.I8Ctx::init_elf()(npu/forward/engine-src/npu_engine_i8ctx_inc.h): refuses the load when a declared width exceeds the buffer that launch would pass, printing both numbers, the argument's role andM/K/N. One-directional on purpose — a declared width may be smaller than the buffer (the tile-family ELFs address a tile of a larger BO through the control text) but never larger, so no geometry that works today can be refused by it.tests/npu_elf_args_test.c: hand-built ELF32 fixtures (no aiecc, no model, no device) — the attention-ELF widths, a narrow tile-family ELF, and three refusals (non-ELF, no.dynsym, truncated header, plusout[]zeroing). Added to the CI target list, since the workflow's own comment says a missing target is reported as "Not Run" and never executes.Verification (run here, not asserted)
Release/ninja configure off currentmain,ONEBIT_NPU=1with the runtime libninja onebit npu_elf_args_testcleannpu_elf_args_testctest -R "npu_elf_args|npu_plan|npu_q4nx"tools/copyright.py --check(163,840 / 2,048 / 1,024),(245,760 / …),(1,556,480 / …),(778,240 / 9,728 / 1,024),(24,330,240 / …); the 27B tile ELF →(512 / 1,024 / 4,096)(fits its buffers, so it is not refused)Not in this PR
The launch itself. Driving this family needs
(weights, activations, output)with the per-core weight-stream layoutforge_benchbuilds, and a decision on whether these kernels stay a family at all — their "M=16" and "M=128" files are byte-identical one-row GEMV designs, so the engine's M dimension means nothing for them. Tracked on #845 / #868 rather than folded into a safety gate.