Repository navigation
npu: zero the activation BO's pad rows only when a previous launch wrote them (target 2) - #873
Conversation
PR Reviewer Guide 🔍(Review updated until commit f5444b0)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
One file, npu/forward/engine-src/npu_engine_i8ctx_inc.h, +52/-12, and the bookkeeping is the whole risk, so I read every
write to Am at this head and compiled it:
$ g++-16 -DONEBIT_NPU_RUNTIME_OWN=1 -Inpu/forward/engine-src -Inpu -Inpu/forward/include -Inpu/ort -I<npu_rt> \
-std=c++26 -O1 -fsyntax-only npu/forward/src/q4nx_forward.cpp -> rc 0, no diagnostics
What holds:
prepare_activation()is called from both quantisers (quantize_asyncandquantize_async_rows), so the rows path
cannot keep the old behaviour by omission.- The only writes to
Amare the quantiser's inner loops atAm[m*KD + k]form < am, so rows[am, ...)can only hold
stale data from a previous launch, which is exactly whata_written_rows_tracks. Nothing else in the file writes
Am(grep: the three occurrences are the two quantisers and the map). - The
ak != KDbranch keeps the whole-BO memset, which is the case where a column tail inside rows[0, am)would
otherwise go stale — right call rather than reasoning about it. int a_written_rows_ = MDis the conservative start, and the comment records that leaving it atMDafter a call was
the first version's bug and why the gate measured no change because of it — that note is what makes the field auditable.NPU_NO_A_FLUSH/NPU_NO_C_INVALaregetenvread into a function-localstatic, both default on, so the shipped path
is byte-for-byte the old one and the two switches are only for the measurement the brief asks for.readback()'s gate
(skip_inval) is in the same idiom as the existing code.
Not verified by me: the 383 MB/token memset arithmetic and any device arm — the memset saving only exists on a real launch,
and turning the syncs off is an experiment that belongs to an npu-hold window, which the owner's serve held throughout
this review.
bong-water-water-bong
added
the
tprs-ready
…he shipped loop (target 2)
Every NPU GEMM quantises one activation row to int8 on the host before its launch, in a scalar loop
built at the baseline x86-64 targets where roundf() is a libm call. On the container's own plan
(bonsai2-27b-t2s2-serve/plan.json: 288 projections, 2,310,144 A elements per decode token) that loop
costs 3.96-4.05 ms/token measured; the AVX2 path costs 0.55 ms/token (0.238 ns/element), 7.2-7.4x,
bytes identical on every projection of the mix.
The loop moves to npu/forward/include/quant_i8.h as a scalar reference plus an AVX2 path dispatched on
__builtin_cpu_supports("avx2"), with the reference used for the tail. Identity is by construction, not
by luck: trunc + an exactly-compared half adjustment is roundf's round-half-away-from-zero, and the
vector path reproduces the shipped loop's three sharp edges -- NaN/+-inf zeroed on the *input*, a finite
input whose product overflows left to saturate through the (int) cast (which x86 makes INT_MIN, so the
clamp yields -127, not 127), and |v*ais| just inside 2^31 taking the adjustment normally.
tests/quant_i8_row_test.cpp proves it with no device and no model: the .5 table, both clamps, NaN, +-inf,
fmax, +-2^31 and just inside it, denormals, -0.0, six real layer scales, lengths that leave a tail, and
6.5 M elements of random *bit patterns* per run (env-tunable rounds), where NaN payloads and denormals
turn up at their real density. It is load-bearing, not decorative: GE -> GT on the half comparison makes
it fail with round-half-to-even answers (67.5 -> 67 vs 68); 97.5 M elements of the same fuzz ran clean
against the shipped loop.
This is the other half of audit row 1: #873 removed 99.2% of the pad memset (+2.60 ms/token) and left
this loop alone.
…he shipped loop (target 2)
Every NPU GEMM quantises one activation row to int8 on the host before its launch, in a scalar loop
built at the baseline x86-64 targets where roundf() is a libm call. On the container's own plan
(bonsai2-27b-t2s2-serve/plan.json: 288 projections, 2,310,144 A elements per decode token) that loop
costs 3.96-4.05 ms/token measured; the AVX2 path costs 0.55 ms/token (0.238 ns/element), 7.2-7.4x,
bytes identical on every projection of the mix.
The loop moves to npu/forward/include/quant_i8.h as a scalar reference plus an AVX2 path dispatched on
__builtin_cpu_supports("avx2"), with the reference used for the tail. Identity is by construction, not
by luck: trunc + an exactly-compared half adjustment is roundf's round-half-away-from-zero, and the
vector path reproduces the shipped loop's three sharp edges -- NaN/+-inf zeroed on the *input*, a finite
input whose product overflows left to saturate through the (int) cast (which x86 makes INT_MIN, so the
clamp yields -127, not 127), and |v*ais| just inside 2^31 taking the adjustment normally.
tests/quant_i8_row_test.cpp proves it with no device and no model: the .5 table, both clamps, NaN, +-inf,
fmax, +-2^31 and just inside it, denormals, -0.0, six real layer scales, lengths that leave a tail, and
6.5 M elements of random *bit patterns* per run (env-tunable rounds), where NaN payloads and denormals
turn up at their real density. It is load-bearing, not decorative: GE -> GT on the half comparison makes
it fail with round-half-to-even answers (67.5 -> 67 vs 68); 97.5 M elements of the same fuzz ran clean
against the shipped loop.
This is the other half of audit row 1: #873 removed 99.2% of the pad memset (+2.60 ms/token) and left
this loop alone.
…he shipped loop (target 2) (#908) Every NPU GEMM quantises one activation row to int8 on the host before its launch, in a scalar loop built at the baseline x86-64 targets where roundf() is a libm call. On the container's own plan (bonsai2-27b-t2s2-serve/plan.json: 288 projections, 2,310,144 A elements per decode token) that loop costs 3.96-4.05 ms/token measured; the AVX2 path costs 0.55 ms/token (0.238 ns/element), 7.2-7.4x, bytes identical on every projection of the mix. The loop moves to npu/forward/include/quant_i8.h as a scalar reference plus an AVX2 path dispatched on __builtin_cpu_supports("avx2"), with the reference used for the tail. Identity is by construction, not by luck: trunc + an exactly-compared half adjustment is roundf's round-half-away-from-zero, and the vector path reproduces the shipped loop's three sharp edges -- NaN/+-inf zeroed on the *input*, a finite input whose product overflows left to saturate through the (int) cast (which x86 makes INT_MIN, so the clamp yields -127, not 127), and |v*ais| just inside 2^31 taking the adjustment normally. tests/quant_i8_row_test.cpp proves it with no device and no model: the .5 table, both clamps, NaN, +-inf, fmax, +-2^31 and just inside it, denormals, -0.0, six real layer scales, lengths that leave a tail, and 6.5 M elements of random *bit patterns* per run (env-tunable rounds), where NaN payloads and denormals turn up at their real density. It is load-bearing, not decorative: GE -> GT on the half comparison makes it fail with round-half-to-even answers (67.5 -> 67 vs 68); 97.5 M elements of the same fuzz ran clean against the shipped loop. This is the other half of audit row 1: #873 removed 99.2% of the pad memset (+2.60 ms/token) and left this loop alone. Co-authored-by: agent <agent@local>
16443d7 to
3d7f66e
Compare
|
Persistent review updated to latest commit 3d7f66e |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3d7f66e80 — this is a rebase onto main, not a merge, so the blobs differ for a reason; I checked the re-application and the one thing a rebase could have broken, which is the interaction with main's new activation-quantiser branches.
1. The patch is the one I verdicted. 16443d7d0 → 3d7f66e80 keeps the whole added block verbatim (a_written_rows_ at MD, prepare_activation() with the ak != KD whole-BO fallback, the a_written_rows_ > am partial zero, the comment recording that leaving it at MD was the first version's bug) and both call sites (memset(Am, 0, MD*KD) → prepare_activation(am, ak) in quantize_async and quantize_async_rows, bA->sync(...) → sync_a_to_device() in launch_async_with_bo/_rows). Everything else that moved in the hunks is main's newer text (q4nx_quant_i8::row, the gemv early-out, if (!gemv) poison_canary(...)).
2. The interaction, audited because this is the class that bit #808. main now has a gemv/bf16 activation branch inside the same file:
bAisKD * 2bytes for the gemv family (:209) — one bf16 row — so there are no pad rows to zero, andquantize_asyncreturns at theif (gemv)line beforeprepare_activation, so the bookkeeping is never engaged on that family.quantize_bf16zeroes the K tail itself (for (k = n; k < KD; k++) Ag[k] = 0).- The
M=128paths are the only ones that touchAmas anMD × KDi8 image, and they still callprepare_activationfirst in both variants. - Writes to
Amin the whole file::560/:565(the two memsets),:592/:618(rowm < amof the quantiser). Nothing else writes it —poison_canary()is the output canary and writesCmonly (:768-777), so it cannot invalidate "rows ≥ a_written_rows_ are zero". - Invariant holds across the call orders that occur:
am=1 → 8zeroes nothing and rewrites[0,8);am=8 → 1zeroes[1,8);ak != KDtakes the whole-BO path and resets toam. The initial valueMDis conservative, so even a BO that is not zero-filled on allocation is fully zeroed on the first call.
3. It compiles and links, which CI does not check (engine #898: the build job configures without -DONEBIT_NPU=ON). Incremental NPU-lane build of this head in my standing build dir:
ninja 1bit -j4 → [17/17] Linking CXX executable 1bit rc=0
warnings: only the pre-existing nlohmann is_trivial deprecation (binary_writer.hpp:1805), none from
q4nx_forward.cpp or npu_engine_i8ctx_inc.h
No NPU ticket from me: the ms/launch numbers are the lane's device runs, and what changed here is host-side bookkeeping whose only claim is that the BO bytes after each call are identical to the old whole-BO memset — which is what the code now says and what I verified by reading the writes.
|
Rebased onto Resolution keeps both sides: Re-gated on the rebased pair (base arm The +2.60 ms/token timing above is not re-measured on the rebased pair; the single-round timing in the re-gate window drifted with box warmth (identical output, 320 -> 572 ms wall on one prompt), which is what the original rounds=4 alternating design was for. A rounds=2 window is running and will be appended to the body if it is clean. |
…ote them (target 2)
The M=128 stream reads activation rows [am, MD) on every launch, so quantize_async() and
quantize_async_rows() memset the whole BO (MD*KD) before writing the real rows. On the decode path am
is always 1, so at MD=128 that is ~655 KB of memset per launch re-zeroing rows that are already zero --
~383 MB per token over this model's ~388 launches.
prepare_activation(am, ak) memsets only [am, a_written_rows_) and then sets a_written_rows_ = am: rows
[0, am) are written by the quantiser, rows [am, a_written_rows_) are zeroed here, the rest were already
zero, so the bytes left in the BO are identical either way. A K shorter than the padded geometry
(ak != KD) leaves a column tail inside rows [0, am) that the quantiser does not write, so that case falls
back to the whole-BO zero rather than reason about it.
Measured on the shipped int8 route (1bit forward-serve, bonsai2-27b-pb16i, --kernels kernels-m16), 3
prompts x 24 tokens, arms alternating in one npu-hold window, 4 rounds, fresh process per run:
identity final hidden-state sha256 identical 8/8 runs, and identical to every earlier window's
reference (9f9f3e3a…, 2bf410a93610…, 3bc3f90b35b2…); [ph] argmax id + top/second/margin
floats identical for all 69 emitted tokens.
timing 415.1 ms/token (base, n=9 clean blocks, sd 1.46) vs 412.5 ms/token (this change, n=7, sd 4.13)
= +2.60 ms/token, 0.63 %, ~3.5 sigma, after discarding blocks that other lanes' load had
contaminated (a block counts only if no launch in it exceeded 450 ms of gemm-host).
0.63 % is small and worth saying so plainly. It is also 5x less than the same bytes suggest at a DRAM
write rate, which is why the memset was measured rather than assumed: glibc memset on this box runs at
~200 GB/s for the 889 KiB-of-one-launch chunk size (1.73 ms/token for 388 of them), not at ~20 GB/s, so
the zeroing was never the tens-of-ms item the audit priced it as. What is left of row 1 of that audit is
the scalar quantise loop beside the memset, which this change does not touch.
ALSO HERE, because target 2 asks it: are the two BO syncs required at all? An XDNA2 BO is cacheable host
memory, so bo::sync() is `mfence; clflush…; mfence` over the range -- cache maintenance, not a copy. Every
A writeback now goes through sync_a_to_device(), and the C invalidate through one switch:
NPU_NO_A_FLUSH=1 skip the writeback of the host-written A before the launch
NPU_NO_C_INVAL=1 skip the invalidate of the array-written C before the host reads it
Both default to on, so the shipped path is unchanged and nothing turns them off in normal running; they
exist so the question can be answered with the same logits gate, inside an npu-hold window. It has been
answered: with either switch off the route emits garbage, non-deterministically, with no amdxdna error, no
TDR and no canary line to catch it -- ref 2/2 byte-identical to the reference hashes above, noA 2/2 and
noC 2/2 garbage with a different head sha256 in every run, and both switches off (3/3) the same. So on
this stack the syncs are needed always, both of them, and silently; they stay, and these switches are how
anyone re-measures that. Anyone using them must also set ONEBIT_NPU_CANARY=0: the canary poisons C row 0
and flushes it to memory, so with the invalidate off it reads its own sentinels and reports a short read
that is the instrument's doing, not the array's.
Not covered by this change, stated so it is not assumed: the prefill side of the pad rows (a prefill launch
writes am = MD rows, so it never had a pad to skip -- the win here is decode-only), and any device-side
work on the GDN recurrence, which is a kernel brief and not a copy fix.
3d7f66e to
95b1c83
Compare
|
Rebased once more, onto
|
|
Persistent review updated to latest commit 95b1c83 |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 95b1c8384 — the head moved again and again it is a main merge: the one file this PR owns (npu_forward/engine-src/npu_engine_i8ctx_inc.h) is blob-identical to 3d7f66e80, the head I reviewed in full (rebase faithfulness, the interaction with main's gemv/AVX2 quantiser, and an NPU-lane ninja 1bit because CI never compiles this file — #898). Nothing to re-derive; label stays.
There was a problem hiding this comment.
🟡 Changes recommended
The activation-row tracker is initialized while MD is zero and never updated, causing the first partial launch with ak == KD to leave padding uncleared and expose uninitialized allocation contents to the kernel.
1 open issue
What changed in this PR
This PR reduces repeated activation-buffer zeroing in the NPU runtime and adds diagnostic controls for measuring cache synchronization requirements.
Changes:
- Tracks previously written activation rows to avoid redundant pad-row clearing, retaining full-buffer clearing when K is padded.
- Adds opt-out switches for activation writeback and output invalidation while preserving synchronization by default.
- Preserves #908’s AVX2 quantizer and the gemv-specific handling.
| File | Description |
|---|---|
npu/forward/engine-src/npu_engine_i8ctx_inc.h |
Adds activation-row tracking and cache-sync switches to the quantization and launch paths. |
Review effort: Balanced
…tually zeroes the pad rows Copilot's review of #873 (inline, blocking): the tracker is initialised from MD, but MD is declared without an initialiser (`int MD, KD, ND, NL`) and is only assigned in init_elf(), while a non-static member initialiser runs at construction — and every context is built with std::make_unique<I8Ctx>() and given its geometry afterwards. Measured rather than assumed: make_unique value-initialises, so MD is zero-filled before the initialiser runs (`Ctx{int MD; int w = MD;}` prints MD=0 w=0 through make_unique and through placement-new on 0xAA; there is also no default-initialised I8Ctx anywhere in the tree). So the tracker started at 0 = 'no rows known', and the first launch with ak == KD zeroed nothing, leaving rows [am, MD) to whatever the BO was allocated with. That is exactly the guarantee this change exists to keep. -1 is an explicit 'nothing known yet': prepare_activation now takes the whole-BO zero when a_written_rows_ < 0 or ak != KD, and costs one full memset on the first launch only. After that the sequence is [base behaviour, then the gated behaviour], both of which are identity-proven, so no output can move. Verified: -fsyntax-only clean, ninja -C build-npu 1bit links (only the pre-existing nlohmann is_trivial deprecation). Device re-gate owed and blocked by the #911 rule (no npu-hold tickets until pi-ogaload posts its repro); not claimed as run.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 22d8047fb (re-verdict: the commit after my READY is not a main merge, it is the Copilot fix, so it needs its own verdict).
git log origin/main..22d8047fb is exactly two commits — 95b1c83 (the gated zeroing, which I verdicted) and 22d8047 — and the second one is a real defect in the first: a_written_rows_ = MD runs at construction while MD is declared without an initialiser (int MD, KD, ND, NL) and only assigned in init_elf(), and every context is built with std::make_unique<I8Ctx>() and given its geometry afterwards. The commit's own measurement of that is the right kind (a Ctx{int MD; int w = MD;} probe through make_unique and through placement-new on 0xAA shows MD=0), so the tracker started at 0 = "no rows known" and the first ak == KD launch zeroed nothing — i.e. the pad rows kept whatever the BO was allocated with, which is precisely the guarantee the PR exists to keep.
The fix is the correct shape and cannot move an output: -1 is an explicit "nothing known yet", prepare_activation() falls back to the whole-BO memset when a_written_rows_ < 0 or ak != KD, so the first launch costs exactly the old unconditional memset and every launch after it is the gated behaviour that was already identity-proven (both arms are identity in sequence).
CI at this head: build SUCCESS, CodeQL SUCCESS, Analyze ×5 SUCCESS (pr_agent CANCELLED and hw_gate SKIPPED are the usual two). The device re-gate is stated as owed and blocked by the #911 rule rather than claimed — that is the right way to leave it, and it does not block the code being right.
|
Persistent review updated to latest commit f5444b0 |
…ting lands under this PR's gated rows main gained #873 (zero the activation BO's pad rows only when a previous launch wrote them) at npu/forward/engine-src/npu_engine_i8ctx_inc.h, a file this branch does not modify. It is not a conflict, but it is inside the code set this PR's identity gate covers, so the old pairing (A = main@eda7542e, B = 5c1de39) no longer describes what would merge: the gate is re-run with A = main@f63af5f9 and B = this tip, which also supplies the device identity row #873 says it still owes. Checked by reading the merged result, not by trusting the clean apply: #873's change is entirely inside I8Ctx (prepare_activation, a_written_rows_, sync_a_to_device, the NPU_NO_A_FLUSH/NPU_NO_C_INVAL switches), and XdnaTarget only calls quantize_async/launch_async_with_bo{,_rows} — it neither repeats the memset logic nor touches bA (its only uses of Am are the two debug dumps at xdna_target.cpp:541,569,806). So no second copy of the rule can drift here.
The device identity row this PR says it still owes — now measured, on a base that carries #873Your commit says "Device re-gate owed and blocked by the #911 rule; not claimed as run". Nobody re-ran it, so the row is now measured, from pi-mosaic's lane, inside its own
Read this for exactly what it is: it says #873 moves no output on the shipped 27B route, with a stronger instrument than the sha256 (identity at every launch, not only at the end). It does not reproduce your 0.63 % timing claim — the If a stricter row is wanted later, |
…(target 2, audit row 17) The array reads activation rows [0, MD) on every launch, so sync_a_to_device() flushed the whole MD-row BO before each submit. Only the lines the host has touched since the last flush can be dirty -- every other row was written back when it was last written -- so the range is the dirty span, not the BO. On the decode path that span is one row (am is always 1), i.e. 16x less traffic per launch on the M=16 context. prepare_activation() already knows exactly which rows the host wrote (that is #873's row tracker): the quantiser writes [0, am), and when it zeroes the pad rows it zeroes [am, a_written_rows_). a_dirty() takes the higher of those, and the flush clears it. The EP client that writes Am itself reports through set_activation_rows() but keeps the whole-BO range: it memsets all MD rows before memcpying its own, so every line of bA is dirty there and a `rows`-sized flush would leave the memset's zeroes in cache to be evicted after the kernel read them. Measured on bonsai2-27b-t2s2-serve (NPU_A_SYNC_PROF=1, I8Ctx's own counters, 30 decode tokens, 431 A syncs/token): 48.94 -> 3.06 MB/token flushed, 0.81 -> 0.32 ms/token of clflush. Identity PASS across three arms x 2 rounds (origin/main binary, narrow, NPU_NO_A_NARROW=1 whole-BO) at token, logprob and final hidden-state sha256 level.
…(target 2, audit row 17) (#1004) The array reads activation rows [0, MD) on every launch, so sync_a_to_device() flushed the whole MD-row BO before each submit. Only the lines the host has touched since the last flush can be dirty -- every other row was written back when it was last written -- so the range is the dirty span, not the BO. On the decode path that span is one row (am is always 1), i.e. 16x less traffic per launch on the M=16 context. prepare_activation() already knows exactly which rows the host wrote (that is #873's row tracker): the quantiser writes [0, am), and when it zeroes the pad rows it zeroes [am, a_written_rows_). a_dirty() takes the higher of those, and the flush clears it. The EP client that writes Am itself reports through set_activation_rows() but keeps the whole-BO range: it memsets all MD rows before memcpying its own, so every line of bA is dirty there and a `rows`-sized flush would leave the memset's zeroes in cache to be evicted after the kernel read them. Measured on bonsai2-27b-t2s2-serve (NPU_A_SYNC_PROF=1, I8Ctx's own counters, 30 decode tokens, 431 A syncs/token): 48.94 -> 3.06 MB/token flushed, 0.81 -> 0.32 ms/token of clflush. Identity PASS across three arms x 2 rounds (origin/main binary, narrow, NPU_NO_A_NARROW=1 whole-BO) at token, logprob and final hidden-state sha256 level. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Target 2 of ~/briefs/TZEROCOPY.md ("NPU runtime: activations live in BOs"), first half: the activation BO's
pad-row memset, plus the two switches that answer the brief's other question ("syncs only where the XDNA cache
model needs them — find out when it is required at all").
The change (one file, +52/−12,
npu/forward/engine-src/npu_engine_i8ctx_inc.h)quantize_async()/quantize_async_rows()memset the whole activation BO (MD*KD) before writing the realrows, because the M=128 stream reads rows
[am, MD)on every launch. On the decode pathamis always 1, soat MD=128 that is ~655 KB of memset per launch re-zeroing rows that are already zero (~383 MB/token over this
model's ~388 launches).
prepare_activation(am, ak)memsets only[am, a_written_rows_)and then setsa_written_rows_ = am: rows[0, am)are written by the quantiser, rows[am, a_written_rows_)are zeroed here, the rest were alreadyzero — the bytes left in the BO are identical either way. A K shorter than the padded geometry (
ak != KD)leaves a column tail inside rows
[0, am)that the quantiser does not write, so that case keeps the oldwhole-BO zero.
The first version of this left
a_written_rows_atMD, i.e. it re-zeroed 127 of 128 rows on every decodelaunch — 99.2 % of the memset it was written to remove — and the gate measured that as no change at all
before the bug was found. That is why the bookkeeping is
= amand why the comment says so.The gate (identity), and the measurement (timing)
1bit forward-serve, pack~/models/npu/bonsai2-27b-pb16iwith--kernels kernels-m16(the shipped int8route the owner serve runs), 3 prompts × 24 tokens, arms alternating inside one
npu-holdwindow, freshprocess per run, 4 rounds. Base arm built from
801a4a60, this change on top of it.9f9f3e3aa50a…/2bf410a93610…/3bc3f90b35b2…[ph] argmaxid +top/second/marginfloats, 69 tokens/run[ph] tokwall, clean blocks only (¶)+2.60 ms/token = 0.63 %, ~3.5 σ. ¶ A block counts only if no launch in it exceeded 450 ms of gemm-host
(clean mode 355–400); other lanes' work loaded this box mid-window and 8 of 24 blocks were discarded on that
rule, per-block medians in the window log. The reference hashes are the same ones this route produced in every
earlier window on this box, and the same ones the rebased arm (this PR's tree on
692cfa25) reproduced at21:37, so the change is bit-exact and the number is not the model's numerics moving.
0.63 % is small and is worth saying plainly. It is also 5× less than the same bytes suggest at a DRAM write
rate, which is why the memset was measured rather than assumed: glibc
memseton this box runs at ~200 GB/sfor the 889 KiB-of-one-launch chunk (1.73 ms/token for 388 of them), not ~20 GB/s, so the zeroing was never the
tens-of-ms item the audit priced it as. What remains of that item is the scalar quantise loop beside the memset,
which this change does not touch.
The switches, and the answer they produced
An XDNA2 BO is cacheable host memory, so
bo::sync()ismfence; clflush…; mfenceover the range — cachemaintenance, not a copy. Every A writeback now goes through
sync_a_to_device(), and the C invalidate throughone switch:
Both default to on, so the shipped path is unchanged and nothing turns them off in normal running. With either
one off the route emits garbage, non-deterministically —
ref2/2 byte-identical to the reference hashes;noA2/2 andnoC2/2 garbage with a different head sha256 in every run; both off, 3/3 the same — with noamdxdnaerror, no TDR and no canary line to catch it. So on this stack the syncs are needed always, both ofthem, and silently. They stay; the switches stay as the way to re-measure that. Anyone using them must also set
ONEBIT_NPU_CANARY=0: the canary poisons C row 0 and flushes it, so with the invalidate off it reads its ownsentinels and reports a short read that is the instrument's doing, not the array's.
Environment and limits
npu-hold -l zerocopy;amdxdna.*ERROR0 after every run,legacy_tdr_detect0, theowner serve on :18097 never touched (the keeper stops it for a waiting ticket, by design).
am = MDrows, so it never had a pad to skip. Prefillis not measured and not claimed.
bonsai2-27b-pb16i,kernels-m16), one route (forward-serve). Theserveroute's sealedpb16i profile is a different forward and its numbers are not these.
Rebase after #908 (2026-11-03, box rebooted)
#908 (the AVX2 row quantiser) merged into the same header while this PR was in review, so this branch conflicted.
It is now rebased onto
origin/main65460184(3d7f66e8), one commit, still one file, still +52/−12, andthe conflict resolution keeps both sides rather than picking one:
quantize_async_rows()now callsq4nx_quant_i8::row()(npu: quantise the activation row on an AVX2 path, byte-identical to the shipped loop #908's SIMD path) andprepare_activation()(thischange). The scalar loop this PR originally carried in that function was npu: quantise the activation row on an AVX2 path, byte-identical to the shipped loop #908's to replace; taking npu: quantise the activation row on an AVX2 path, byte-identical to the shipped loop #908's call
is what makes the two changes compose instead of racing.
sync_a_to_device()(this change's switch) followed byif (!gemv) poison_canary(...), andeach readback is the switch-gated invalidate followed by
if (canary_on_ && !gemv) verify_canary()— i.e. thisPR's switches and the new gemv family's "the canary is an int32 sentinel, the gemv output is f32" guard.
quantize_bf16()and the bf16 activation BO are untouched; the gemv family never reachesprepare_activation,because
quantize_asynctakes the bf16 branch first.The gate was re-run on the rebased pair, because the base is no longer the tree the first gate ran on: base arm =
65460184(nothing but main), arm =3d7f66e8(this change on that exact base), same commit, same build dir,same pack pair (
bonsai2-27b-t2s2-serveandbonsai2-27b-pb16i), 3 prompts × 5/2/24 tokens, greedy,ONEBIT_NPU_CANARY=0 NPU_M16_CHECK=1, onenpu-hold -l zerocopywindow:The timing number above (+2.60 ms/token) was NOT re-measured on the rebased pair. Two windows were run for
it and neither yields a quotable figure, because the box was loaded and the load is 3-4x larger than the effect:
A single run of one arm spans 264 -> 1035 ms of the same launch; the change being measured is ~2.6 ms/token. At the
least-loaded blocks the two arms are indistinguishable (t2s2 263-275 vs 261-275 ms; pb16i r1 381 vs 359 ms, r2
polluted on both sides), so the rebased pair neither confirms nor contradicts the +2.60 ms/token — it is simply
below this window's noise floor. The original measurement was taken with
rounds=4arms alternating in one quietwindow, and it stands as the number; the mechanism this change removes (a pad-row memset) is orthogonal to #908's
(element loop) and #908 changed no BO layout, so the saving is stated as carried, not re-measured.
What the re-gate does establish, twice over (two windows, four arms, both packs): the output is bit-identical,
which is the gate that decides whether this merges.
Copilot review, 2026-11-03: one real defect found, fixed in
22d8047fCopilot's inline thread (#873,
npu_engine_i8ctx_inc.h:559) saida_written_rows_capturesMDat construction,when the geometry has not been assigned yet, so the first partial launch leaves the pad rows uncleared. That is
correct, and it is the one thing in this change that could have broken the guarantee it exists to keep:
MDis declared without an initialiser (int MD, KD, ND, NL;) and is only assigned ininit_elf()(MD = M;),while the non-static member initialiser runs at construction;
std::make_unique<I8Ctx>()and given its geometry afterwards(
q4nx_forward.cpp:931,onnx/onebit_npu_gemm.cpp:205).Measured rather than assumed, because it decides how live the defect is:
make_uniquevalue-initialises, so theobject is zero-filled before the member initialiser runs and
MDreads 0 deterministically —Ctx{int MD; int w = MD;}printsMD=0 w=0throughmake_uniqueand through placement-new on0xAAstorage, andgrep -rn 'I8Ctx [a-z_]*;' npu/finds no default-initialised instance, so nothing took an indeterminate path. Thedefect is therefore not UB but a wrong starting value:
a_written_rows_began at "no rows known", the firstak == KDlaunch zeroed nothing, and rows[am, MD)were left to whatever the BO was allocated with. The gatepassed only because a freshly allocated BO already reads as zero.
Fix:
int a_written_rows_ = -1;— an explicit "nothing known yet" that depends on neither member order nor theallocation — and
prepare_activation()takes the whole-BO zero whena_written_rows_ < 0 || ak != KD. One fullmemset on the first launch, nothing after it. No output can move from this: with the fix the launch sequence is
[
baseon launch 1, then the previously gated behaviour on every later launch], and both are identity-proven.-fsyntax-onlyclean andninja -C build-npu 1bitlinks (only the pre-existing nlohmann deprecation). The devicere-gate is owed and not claimed: the #911 rule holds this lane off
npu-holduntil pi-ogaload posts its repro.