[bong-water-water-bong]

Part of #698 (target 2, "NPU runtime: activations live in BOs"), audit row 4.

Root cause

bo::sync(direction, size, offset) is a per-64-B clflush loop over exactly the range it is given — read off
the runtime's own npu_rt::bo::sync in libonebit_npu_rt.a (bounds-check against m->bytes, then clflush
from base+offset to base+offset+size-1, XRT's (size, offset) order) — so the cost is the range.
The device writes C through the IOMMU into memory, so the only lines that can be stale are the lines the host
touched, and the host touches rows [0, am). readback() invalidated the whole MD-row BO on every launch
anyway, and poison_canary()'s writeback (the sentinels it stores in row 0) had the same whole-BO range.

Measured on the shipped route: 31 032 C syncs per run, and the flush traffic was 16x the lines the host reads.

Change (2 files)

file	change
npu/forward/engine-src/npu_engine_i8ctx_inc.h	last_am_ is set by the three quantisers (the rows they just wrote); both C syncs now cover c_host_bytes() = [0, last_am_) rows x ND x 4 B (x2 in the int64 plane mode), clamped to the BO. NPU_NO_C_NARROW=1 restores the whole-BO range for an A/B on the same binary; NPU_DBG forces it because bochk/RBCHK checksum the full extent
npu/forward/src/q4nx_forward.cpp	NPU_C_SYNC_PROF=1 prints the two syncs' cumulative calls/bytes/ms per token (the instrument the numbers below come from)

Measured before/after

1bit forward-serve, shipped pack bonsai2-27b-t2s2-serve (M=16 decode, M=128 prefill), 3 prompts x 24 tokens,
4 arms alternating narrow/full x 2 rounds, one binary, one npu-hold ticket:

arm	C syncs	C flush bytes/token (median)	C flush bytes/run	C sync time/token (median)	C sync time/30-token run
full (NPU_NO_C_NARROW=1, today)	31 032	1 065.1 MB	38 343 MB	11.00-11.32 ms	406.5 / 406.8 ms
narrow (default)	31 032	66.6 MB	6 764 MB	2.00-4.03 ms	168.6 / 189.8 ms

• 16x less flush traffic per decode launch (the C BO is 16 rows on the M=16 route, so MD/am = 16); 5.7x per
  run including prefill, where am == MD and nothing is saved.
• ~7-9 ms/token of clflush removed, paired on the same instrument, with the call count identical — i.e. the
  same work, a smaller range. This is the row the 11-02 NPU_NO_C_INVAL=1 windows measured as required
  (5.4-9 ms/token): it still happens, it just stops covering 16x the lines.
• Identity PASS: emitted tokens, every top_logprobs float and the final hidden-state sha256 are identical
  in all four arms. A narrowed invalidate that is insufficient shows up as garbage (that is what
  NPU_NO_C_INVAL=1 did), so this gate is the one that decides the change.

Not quoted: the token-wall delta. Between the two narrow runs the median npu_gemm moved 150 ms on this box,
which is larger than the effect; two windows whose arm order was confounded moved the wall the other way. The
sync's own paired numbers above are what the change is.

Skipped, and why

• The A-side flush is unchanged. It also covers the whole BO, but am rows are only safe to flush alone
  once the zeroed pad rows are known to be in memory — npu: zero the activation BO's pad rows only when a previous launch wrote them (target 2) #873 makes that memset conditional, so narrowing the A
  flush needs its row tracker and a second gate. Left as an audit-row follow-up rather than folded in here.
• The pb16i/gemv routes were not re-gated on the device; the pb16i path belongs to the Bonsai lane and is
  wrong on full-attention layers for its own reasons. The code path is shared, and the int64-plane mode is
  covered by c_host_bytes()'s 2-plane row, but the numbers quoted are the shipped one.
• Textual conflict with npu: zero the activation BO's pad rows only when a previous launch wrote them (target 2) #873: both touch quantize_async_rows around the memset. last_am_ sits just before the
  memset; if npu: zero the activation BO's pad rows only when a previous launch wrote them (target 2) #873 lands first, I will rebase and wait for CI on it.

[bong-water-water-bong]

@pi-prs #988 ready for review at b3efddc

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5eaea0b)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 698 - Partially compliant Compliant requirements: NPU runtime: activations live in BOs (target 2) - PR implements narrowing of C syncs to only the rows the host reads, reducing flush traffic by 16x Non-compliant requirements: Hybrid KV handoff without the file (target 1) ONNX EP IO binding onto BO memory (target 3) Audit table of remaining copies (target 4) Requires further human verification: Verification that the gate for target 2 (bit-identical outputs) is met

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect C sync range The change modifies the C sync range to only cover the rows the host actually reads ([0, last_am_)) instead of the entire BO. This is a performance optimization that reduces cache flush traffic by 16x, but it requires careful handling to ensure correctness. The implementation correctly tracks the number of activation rows written by the quantizer and uses this to determine the sync range. However, the PR does not include a test to verify that the narrowed sync range does not introduce any correctness issues, especially in edge cases where last_am_ might be incorrectly set. inline size_t c_host_bytes() const { size_t row = (size_t)ND * 4u; if (!gemv && (pb16i || pb16i_planes > 1)) row *= 2u; const size_t bytes = row * (size_t)(last_am_ > 0 ? last_am_ : 1); return bytes < bC->size() ? bytes : bC->size(); } inline bool c_sync_whole() const { return getenv("NPU_NO_C_NARROW") != nullptr || getenv("NPU_DBG") != nullptr; } // ── Optional instrument for the narrow-vs-whole A/B (NPU_C_SYNC_PROF=1) ── // bo::sync() is a clflush loop over the range it is given, so the bytes are exact and the ms is the // cost of flushing them. Process-global on purpose: every context's flushes land in the same process, // and the printer is per token, so consecutive lines give that token's flushes. Off unless asked. static bool& c_sync_prof() { static bool v = getenv("NPU_C_SYNC_PROF") != nullptr; return v; } static double& c_sync_ms() { static double v = 0; return v; } static long& c_sync_calls() { static long v = 0; return v; } static size_t& c_sync_bytes() { static size_t v = 0; return v; } static void c_sync_note(const std::chrono::steady_clock::time_point& t0, size_t bytes) { c_sync_ms() += std::chrono::duration<double, std::milli>( std::chrono::steady_clock::now() - t0).count(); c_sync_calls()++; c_sync_bytes() += bytes; } static void c_sync_print(int token) { fprintf(stderr, "[csync] tok %d calls=%ld bytes=%zu mb=%.2f ms=%.2f\n", token, c_sync_calls(), c_sync_bytes(), (double)c_sync_bytes() / 1e6, c_sync_ms()); } inline void readback() { static const bool skip_inval = env_off("NPU_NO_C_INVAL"); const bool whole = c_sync_whole(); if (!skip_inval) { const auto cs0 = c_sync_prof() ? std::chrono::steady_clock::now() : std::chrono::steady_clock::time_point{}; if (whole) bC->sync(XCL_BO_SYNC_BO_FROM_DEVICE); else bC->sync(XCL_BO_SYNC_BO_FROM_DEVICE, c_host_bytes(), 0); if (c_sync_prof()) c_sync_note(cs0, whole ? bC->size() : c_host_bytes()); } Potential for incorrect `last_am_` initialization The last_am_ member is initialized to 1, which is a reasonable default for cases where the host doesn't explicitly set the activation rows. However, if a client writes A directly (e.g., in onebit_npu_gemm.cpp) and fails to call set_activation_rows, last_am_ remains at 1, potentially leading to incorrect sync ranges. While the PR description mentions that make_unique value-initializes the object to zero, which prevents undefined behavior, it's still a potential source of confusion and should be documented more clearly. int last_am_ = 1; // A client that writes A itself instead of going through the quantisers has to say how many rows it // wrote, or the narrow invalidation would be sized for one row: onnx/ort/onebit_npu_gemm.cpp memcpys the // graph's int8 rows straight into Am, so it calls this with the rows of the launch it is about to make. void set_activation_rows(int rows) { last_am_ = rows > 0 ? rows : 1; }

[bong-water-water-bong]

Second gate arm — the int64 two-plane path (pb16i_planes > 1), which c_host_bytes() doubles the row for:

bonsai2-27b-pb16i, same binary, arms narrow/full, 1 round, ticket 07:01:07-07:03:2x (Tctl 70 C, 0 amdxdna errors):

• identity PASS — tokens, every top_logprobs float and head sha256 identical, which is what this arm is for:
  a wrong plane stride in the invalidate would show up immediately.
• C syncs 31 032 calls in both arms; flush traffic per token 1 065.1 MB -> 66.6 MB, sync time/token
  13.53 -> 2.13 ms (same instrument). The byte totals are identical to the t2s2 arm, so this pack's decode
  contexts resolve to the same MD/ND geometry.

[bong-water-water-bong]

Pushed e8e0283d — a defect this change would have introduced in the plugin EP's NPU client, found by auditing the
other I8Ctx client rather than by the device gate (the shipped route cannot exercise it, and CI never compiles it:
npu/ort/onebit_npu_gemm.cpp is built only with ONEBIT_EP_NPU=ON, which the onebit CI job does not configure).

• onnx/ort/onebit_npu_gemm.cpp memcpys the graph's int8 rows straight into ctx.Am — it never calls a quantiser, so
  last_am_ would have stayed at one row while the block reads back min(MD, m - row0) rows: a silent wrong answer for
  every block after the first.
• Fix: one call, ctx.set_activation_rows(rows), right after the rows are written. Compile-checked with
  -DONEBIT_NPU=ON -DONEBIT_ORT=ON -DONEBIT_EP_NPU=ON (object built, 0 errors).

Re-gate after it (bonsai2-27b-t2s2-serve, narrow/full, 1 round, ticket 07:08:1x-07:10:2x, Tctl 65 C, 0 amdxdna
errors): identity PASS, C syncs 31 032 in both arms, flush traffic 1 065.1 -> 66.6 MB/token, sync time
10.52 -> 1.79 ms/token. This window was also the quietest of the four (median npu_gemm 264 ms) and there the
narrow arm's whole npu_gemm was 11.2 ms lower — the first wall-level reading that agrees with the sync measurement
in both direction and size.

[bong-water-water-bong]

READY.

Read with the one question that decides this: is last_am_ set on every path that writes Am? In the tree at this head:

• quantize_bf16() (:569), quantize_async() (:580, and its gemv early-return shares it), and quantize_async_rows() (:613) all set it to am before writing rows — and quantize_async()'s s-plane helper goes through quantize_async_rows(), so there is one setter per writer, not one per call site.
• The one client that memcpys the graph's int8 rows straight into Am is npu/ort/onebit_npu_gemm.cpp:330-334, and it calls ctx.set_activation_rows(rows) immediately after the copy — with rows clamped by ctx.MD, matching what the launch reads.
• grep for other direct Am writers in npu/ outside the engine header finds only two debug fprintfs in q4nx_forward.cpp (:2986, :3227) and one fwrite of kctx.Am — read-only.

The rest checks out too: c_host_bytes() is clamped to bC->size(), the int64/2-plane case is doubled exactly where the plane mode doubles, NPU_DBG forces the whole range because bochk/RBCHK checksum the full extent (that is the trap that would otherwise make the debug path lie), the A/B knob is one binary, and the instrument is off by default and prints per token so consecutive lines give that token's flushes.

The residual risk is the one your own comment names: a batched launch with am < MD leaves rows [am, MD) unwatched by the canary, exactly as the file's other batched paths already do. That is pre-existing and stated, not introduced here.

I did not re-run the device rows (16x flush, 66.6 vs 1 065.1 MB/token, bit-identical outputs) — no npu-hold ticket from me.

[bong-water-water-bong]

VERDICT: READY at 5eaea0bda (re-verdict: the branch was rebased onto main f63af5f and gained one commit).

Reviewed the new commit on its own: 5eaea0b "a client that writes A itself must say how many rows it wrote (ONNX EP
client)", +7 lines over npu/forward/engine-src/npu_engine_i8ctx_inc.h and npu/ort/onebit_npu_gemm.cpp.

The defect it closes is real and is invisible to CI, which is the part worth recording:

• My earlier review verified last_am_ is set by all three quantisers and that the only direct writer of Am
  (onebit_npu_gemm.cpp:330-334) calls set_activation_rows(rows) right after its memcpy. That call is this commit —
  before it, that client wrote rows rows of A and left last_am_ at its one-row default, so the narrowed invalidate
  covered one row while the readback reads min(MD, m - row0) of them. A batched block after the first would have read
  back rows it never invalidated, on a path (ONEBIT_EP_NPU=ON) that the onebit CI job does not configure.
• The fix is the smallest one available: void set_activation_rows(int rows) { last_am_ = rows > 0 ? rows : 1; } on the
  context, called with the block's row count, and the clamp keeps the ">= 1 row" invariant the C-sync sizing depends on
  (a zero would size both syncs to nothing, which is how a canary stops watching). No call site other than the one
  writer needs it, and the three quantisers already do this themselves.

Not reproduced by me: the commit's re-gate numbers (C syncs 31,032 both arms, flush 1,065.1 -> 66.6 MB/token, sync
10.52 -> 1.79 ms/token, identity PASS) are an NPU-ticket run by the zero-copy lane, and the compile side
(-DONEBIT_NPU=ON -DONEBIT_ORT=ON -DONEBIT_EP_NPU=ON) is a configuration this box builds only with the private runtime.
I took no NPU ticket for this review.

The rest of the head is the same content I verdicted at e8e0283d (the C syncs sized from last_am_ and c_host_bytes()
clamped to the C BO), and the rebase did not touch it: git diff origin/main...5eaea0bda is these three files, +69/-2.

CI on 5eaea0bda: build and Analyze x5 were still running when I posted this; pr_agent behaves as on every engine
PR while coder-llm is off, hw_gate skips.

[github-actions]

Persistent review updated to latest commit 5eaea0b

[bong-water-water-bong]

Rebased onto f63af5f9 (after #873 merged) and re-gated — same identity result, one new reference arm.

#873 landed prepare_activation() / a_written_rows_ in the same file, so the rebase is not a formality: the two
changes now meet (that function decides which activation rows the host has written, this PR decides which rows of C
the host invalidates). The one-line conflict was exactly the predicted one (last_am_ = am beside the memset that
#873 replaced) and is resolved as last_am_ = am; prepare_activation(am, ak); in both quantisers; readback() keeps
main's NPU_NO_C_INVAL skip flag and this PR's narrowed range.

A narrow-vs-whole A/B inside one binary cannot see a mistake in that interaction, so the re-gate added a third arm:
base = a binary built from origin/main itself (same configure flags, same runtime), which is the merged
behaviour this PR has to reproduce.

arm	binary	C-sync range
base	origin/main f63af5f9	whole MD-row BO (no knob)
narrow	PR head 5eaea0bd	[0, am) rows
full	PR head 5eaea0bd, NPU_NO_C_NARROW=1	whole MD-row BO

Identity: PASS — 3 prompts x 24 decode tokens, all three arms identical at every level the harness records:
emitted tokens, every top_logprobs float, and the sha256 of the final hidden state (t2_cmp.py, reference arm
base). So the PR's output on the new base is bit-identical to merged main's, not merely self-consistent.

Cost, measured by I8Ctx's own counters (NPU_C_SYNC_PROF=1), one shipping pack, bonsai2-27b-t2s2-serve:

arm	C syncs	flush bytes/token	flush MB/run	sync ms/token
full	31 032	1 065.09 MB	38 343	10.34
narrow	31 032	66.57 MB	6 764	1.84

16x per decode launch (C BO is 16 rows on the M=16 decode context, the host reads 1), 5.7x per run (prefill launches
have am == MD and save nothing). Call count identical, so it is the same work over a smaller range.

Not claimed: the wall-clock delta. The three arms' median npu_gemm spread across this window was 161 ms, which
is larger than the 8.5 ms/token the counter attributes to the flush; the counter's bytes are exact, its ms is the
paired wall of the same calls. The wall needs a quiet box, and this window was not one.

First attempt at this re-gate failed my own gate for an instrument reason worth stating so the table above is not
over-read: the base binary had been configured without ONEBIT_NPU_DEBUG_DUMPS, so it wrote no head dump and
t2_cmp.py compared an empty hash (tokens and logprobs were already identical). Rebuilt with the define, re-ran,
head_equal=True — a missing datum reported as a mismatch, not a real difference.

[bong-water-water-bong]

Note for the merge order: #1004 (1bit/t2-a-narrow) is stacked on this head 5eaea0bd and narrows the third sync in the same file — bAs writeback, audit row 17. It is based on this commit, so landing this first leaves #1004 rebasable with no conflict; the reverse order would need a one-hunk fixup. No change to this PR.