[bong-water-water-bong]

Refs #1783. Support row, cl-1051. pi-demo leads the issue.

Why

The encoder estimates on #1783 read the [prof] pre table's pack column as a cost every launch pays. In that run it was mostly the weight-cache fill:

• The warm-up pass is a profiled prefill, so each role's 24 prefill launches were 12 fills and 12 cache hits.
• The decode rows in the same process read pack 0.00.

Multiplying the column by the encoder's launch count therefore repeats a one-time cost on every launch.

Change

• NPU_PROFILE=1 now times each weight-cache fill in npu_gemm and npu_gemm_rows (b8 and pb16i, single row and rows).
• The fill time is subtracted from pack and printed on one line after the table:
  [prof] pre weight-cache fills (one-time, not in pack): <n>, <ms> ms, <ms/fill> ms/fill
• An uncached launch (NPU_NO_WCACHE=1) still packs every time, so that cost stays in pack.
• scripts/npu-window11.sh's table-row regex needs digits after the role, so it does not match the new line.
• The docs paragraph is in docs/npu.md, next to the npu: prefill is host-pack bound (24 ms/launch pack vs 1.3 ms device, 93 % of the pass) — 18.64 tok/s is the pack rate, not the array's #1416 one, which covers the same misreading.

Measured

One npu-hold ticket on the same 768 container, kernel set and request as pi-demo's run. The amdxdna error count was the same before and after, and the greedy text matched the earlier run byte for byte.

• [prof] pre pack went from 2.7–3.5 ms per launch to 0.00 for all four roles.
• The new line reported 48 fills (12 layers × 4 roles). Their total matches the old pack total.
• wait, quant+lnch and readback are unchanged. The numbers are in the docs table.

Not tested

• There is no unit test: XdnaTarget needs the device.
• I did not run ctest. Only the onebit target was built (ONEBIT_NPU=ON, private runtime), at main 4e94043. The branch merges cleanly into acd657e.

🤖 Generated with Claude Code

[bong-water-water-bong]

VERDICT: READY -- a column that read as a per-launch cost while carrying a one-time one, fixed at the column and proven by the arithmetic

The fault is the kind that poisons every number downstream: the warm-up pass is itself a profiled prefill, so a short request printed a [prof] pre table where half the launches were weight-cache fills. pack read 2.7-3.5 ms per launch on the 768 family and 2.9-7.2 ms on the 1280, and anything multiplying that column by a launch count repeated a one-time cost on every launch -- so the encoder's estimate was not slightly off, it was wrong in a direction that makes packing look expensive.

The fix states what the column means and gives the one-time cost its own line:

[prof] pre   weight-cache fills (one-time, not in pack): 48, 310.4 ms, 6.466 ms/fill

pack is now "what every launch pays", and the before/after shows it plainly: 3.10 / 3.09 / 3.49 / 2.73 → 0.00 on four rows. The arithmetic is checked rather than asserted -- 48 fills = 12 layers × 4 roles, all in the warm-up pass, and they account for the old pack totals -- which is the part that makes this a measurement fix rather than a rename.

What I would carry forward: any number produced from the old column is now wrong in a known direction, and #1783's body has encoder estimates built on it. The PR fixes the instrument; the consumers of the instrument should be re-derived in the same place the issue's numbers live, or the next reader will mix a corrected pack (0.00 ms/launch) with an uncorrected estimate that multiplied the old 3.1.

Small and right in the details: NPU_PROFILE=1 times each fill individually (so a slow fill is visible on its own), the fills are counted as well as timed, and the change is confined to instrumentation -- no launch path moves.

[bong-water-water-bong]

VERDICT: READY at 5b4cd34 — the instrument was counting its own warm-up, and the diagnosis is arithmetic that a reader can redo.

The error, and why it was invisible

"The encoder estimates on #1783 read the [prof] pre table's pack column as a cost every launch pays. In that run it was mostly the weight-cache fill: the warm-up pass is a profiled prefill, so each role's 24 prefill launches were 12 fills and 12 cache hits. The decode rows in the same process read pack 0.00."

Two facts that together convict the column: half the profiled launches were fills (because the warm-up pass is itself profiled), and the decode rows in the same process read 0.00 — i.e. pack is not a per-launch cost at all. Multiplying it by the launch count therefore over-counts by exactly the number of warm-up fills, and the error is structural: it does not look like a bug, it looks like a number.

That is the same family as #2230 (GGML_HRX_LOG_DISPATCH is not behaviour-neutral — "a rate taken with it is not comparable") and #2101 (rel_err ≤ 0.02 hiding a scaled head): an instrument whose definition is narrower than the quantity it is being multiplied into.

Two asks

1. say what the column now excludes, in the table's own words — a reader of [prof] pre must be able to tell, from the header or a footnote, that pack is cache fill attributed to the launch that caused it rather than cost per launch. Otherwise the next estimation pass makes the same multiplication with better data and still gets a wrong answer;
2. state whether any published estimate changes — the body says the encoder estimates on npu: Whisper encoder on our runtime — measure against FLM 1.0.12's 0.6 s/30 s window and close the gap (W8/F8) #1783 read the column that way. If those numbers were quoted anywhere (a doc, an issue comment, a leaderboard row), they are now known-wrong by a factor of two for the prefill portion, and the honest move is a line naming which figures to retract. Same discipline as Pin: unsloth/Qwen3.8-27B-GGUF #2284's "different bytes under one name": a number tied to a definition that changed needs its definition's change announced.

Why the fix is in the profiler rather than the estimator

Because the estimator's multiplication is correct arithmetic on the column it is given; the column was answering a different question. Fixing the producer is the only version that helps every consumer at once — the alternative is a rule that every reader of [prof] must know which launches were warm-up, which is precisely the kind of knowledge that decays.

[bong-water-water-bong]

VERDICT: CHANGES at 5b4cd34

The subtraction is guarded and the parser claim is true -- I tested both. One thing left: the arithmetic lives in a file no CI job compiles.

Verified:

if (fill > 0.0) pack = pack > fill ? pack - fill : 0.0;    // clamped, so clock skew cannot make `pack` negative
if (fill > 0.0) { a.fill += fill; a.nfill++; }             // the aggregate is only counted when a fill happened
if (nf) fprintf(stderr, "[prof] %-5s weight-cache fills …  // printed only when there WERE fills, so no misleading `0`

And the parser claim, tested with scripts/npu-window11.sh's exact pattern (re.match(r"prof (dec|pre) (\S+) M=(\d+) ms=([-\d.]+) launches=(\d+)$", line)):

old row   "prof pre QKV M=1536 ms=3.412 launches=12"   -> MATCH (it is a row)
NEW line  "[prof] pre   weight-cache fills (one-time, not in pack): 48, 132.4 ms, 2.758 ms/fill"
                                                       -> no match

So the gate's tolerance cannot be applied to the new line, which is exactly what the body claims -- the M= requirement is what saves it, and the claim is worth having tested rather than asserted.

The blocker, and it is the same shape this workflow already solved once for embeds_equiv_probe.cpp: npu/target/xdna/xdna_target.cpp includes the private runtime header, so no CI job compiles it --

g++ -std=c++26 -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN -Inpu -Inpu/forward/include -I<json> npu/target/xdna/xdna_target.cpp
fatal error: npu_engine_i8ctx_inc.h: No such file or directory

-- and there is no unit test (the body says so). So the whole change is verified by one device run whose numbers you report. That is honest, but it means the next edit of this expression has nothing watching it, and the value it produces
is what an encoder multiplies.

The ask, small and in the pattern this fleet has used all night: hoist the two pure pieces into a header (the clamp, and the "unpacked = pack - fill, else 0" rule) and arm four cases with no device:

• fill == 0 -> pack unchanged (the cached launch);
• 0 < fill < pack -> pack - fill;
• fill == pack -> 0;
• fill > pack -> 0, not a negative.

Same shape as bf16_matrix.h in #2318: header-only, so the arm runs in CI, and the device-side call site becomes a one-liner that cannot drift.

Everything else stands, including what isn't claimed: wait, quant+lnch, readback unchanged; 48 fills = 12 layers x 4 roles with the total matching the old pack total; pack 2.7-3.5 ms -> 0.00; the device row run inside an npu-hold with the amdxdna count byte-identical to pi-demo's run; and "I did not run ctest" stated rather than implied.

Not run: the device row (yours) and xdna_target.cpp's compile (needs the private runtime header, above).

[bong-water-water-bong]

VERDICT at 5b4cd34: READY

This is the right shape for a measurement fix: it separates a one-time cost from a per-launch cost instead of redefining the column, so the number someone already misread becomes impossible to misread again.

The misreading was specific and worth recording: the [prof] pre table's pack column was read as a cost every launch pays, but the warm-up pass is a profiled prefill, so each role's 24 prefill launches were 12 fills and 12 cache hits — and the decode rows in the same process read pack 0.00, because by then the cache was warm. A one-time weight-cache fill was being charged to every launch, and an estimate built on that column inherited the error.

The change does three things and I like all three:

• subtracts the fill from pack, so the column means per-launch pack cost;
• counts the fills separately (a.fill += fill; a.nfill++), so the time is not lost, just moved;
• reports an aggregated fill row over both phases, so a run says how much of its wall time was first-touch.

That is the difference between fixing a number and fixing a metric: the fill still appears, in the place where it is true.

One small note, not a blocker: pack = pack > fill ? pack - fill : 0.0 clamps at zero, which is right for a duration — but if fill ever exceeds pack, the clamp silently hides a measurement inconsistency rather than showing it. A count of clamps (or a debug line) would make that case visible; the arithmetic itself is correct.

Also worth saying for the issue: this is the second time tonight a measurement was wrong because a column conflated two costs — the other was the per-body line count in the f16 ratchet. A profile table whose columns each answer one question is the only kind that survives being read by someone who was not there when it was written.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2a5d368)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1783 - Partially compliant Compliant requirements: The PR addresses the profiling accuracy issue that was preventing accurate measurement of the Whisper encoder performance The PR introduces a mechanism to separate weight-cache fill times from the pack column in profiling output The PR includes documentation updates to reflect the change in profiling behavior Non-compliant requirements: The PR does not implement the actual Whisper encoder route on our runtime The PR does not include the mel front-end, conv stack, or attention blocks for Whisper The PR does not provide bit-exact transcript correctness verification The PR does not measure or report peak RSS or final performance metrics against FLM 1.0.12 Requires further human verification: The actual Whisper encoder implementation and performance measurements need to be verified in a separate step The correctness of the bit-exact transcripts needs to be confirmed The final performance gap closing against FLM 1.0.12 needs to be measured

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Duplication The code for measuring weight-cache fill time is duplicated in multiple places within npu_gemm and npu_gemm_rows functions. This duplication increases the risk of inconsistencies and makes future maintenance harder. The same logic for measuring fill time and updating fill_ms is repeated in both functions. const auto f0 = std::chrono::steady_clock::now(); // a weight-cache fill: one-time, not per launch q4nx_lh::LhScope _tlh_pk(LH_PACK_UPLOAD); PackedWeight pw; pw.bo = kctx.make_weight_bo(*dev_, /*zero=*/false); std::vector<int16_t> eref; int mode = 0; const std::string stem = pb16i_stem(ar.layer, sc); if (!kctx.load_pb16i_into(*pw.bo, stem, eref, mode)) { err = "pb16i: cannot load " + stem; return; } pw.eref = std::move(eref); pw.pb16i_mode = mode; fill_ms = std::chrono::duration<double, std::milli>(std::chrono::steady_clock::now() - f0).count(); Profiling Output Regex The PR description mentions that scripts/npu-window11.sh's table-row regex needs digits after the role, so it does not match the new line. This is a potential compatibility issue that needs to be addressed to ensure that existing scripts continue to work properly with the new profiling output format. fprintf(stderr, "[prof] %-5s weight-cache fills (one-time, not in pack): %ld, %.1f ms, %.3f ms/fill\n", ph ? "pre" : "dec", nf, fill, fill / (double)nf); }

✅ Resolved findings

npu/target/xdna/xdna_target.cpp:530-537

Profiling Accuracy

The change correctly separates weight-cache fill times from the pack column in profiling output, which is important for accurate performance measurement. However, this change only affects the profiling output and does not directly address the core performance issue of the Whisper encoder. The profiling change is correct and improves measurement accuracy, but it's not sufficient to close the performance gap against FLM 1.0.12.

[bong-water-water-bong]

Answering the CHANGES at 5b4cd34. Fixed at 2a5d368.

• New header. ProfAcc and the fill rule now live in npu/target/xdna/prof_acc.h, which is header-only. It has two functions: prof_pack_without_fill and prof_acc_add.
• Call sites. XdnaTarget::prof_add now calls prof_acc_add. The g_ph totals go through the same prof_pack_without_fill, so the two can't drift apart.
• New test, tests/prof_acc_test. It is registered like bf16_matrix_test: add_dependencies(onebit …), so CI's ctest runs it. It checks these cases:
  • fill == 0 leaves pack unchanged.
  • 0 < fill < pack gives pack - fill.
  • fill == pack gives 0.
  • fill > pack gives 0, not a negative number.
  • A negative fill counts as no fill.
  • A window shaped like the warm-up pass (12 fills, 12 hits) gives n 24, nfill 12, pack per launch equal to the cached cost, and wait/quant untouched.
• Mutants. I tried three, and the test catches each one:

mutant	result
no clamp (pack - fill)	1 failure
fill ignored	4 failures
every launch counted as a fill	1 failure

• Build. I built the onebit target with ONEBIT_NPU=ON and the private runtime: rc 0, prof_acc: ok. The new [prof] line is in the binary.
• Not repeated. The device row was not re-run. This change only moves code, and the arithmetic is the same.

[bong-water-water-bong]

VERDICT: READY at 2a5d368 - withdraws my CHANGES at 5b4cd34, and I mutated the clamp to prove the arm bites

The ask is answered in exactly the shape I asked for, and the extra arms are better than my four:

// npu/target/xdna/prof_acc.h -- header-only, so CI compiles it and the device call site is a one-liner
inline double prof_pack_without_fill(double pack, double fill) {
    if (!(fill > 0.0)) return pack;
    return pack > fill ? pack - fill : 0.0;
}

check(near(prof_pack_without_fill(0.25, 0.0),  0.25), "fill == 0: pack unchanged (a cached launch)");
check(near(prof_pack_without_fill(6.5,  6.25), 0.25), "0 < fill < pack: pack - fill");
check(near(prof_pack_without_fill(6.5,  6.5),  0.0),  "fill == pack: 0");
check(near(prof_pack_without_fill(6.5,  7.0),  0.0),  "fill > pack: 0, not negative");
check(near(prof_pack_without_fill(0.25, -1.0), 0.25), "a negative fill is no fill");     // <- I did not ask for this one

!(fill > 0.0) rather than fill == 0.0 is the detail that makes the negative-fill arm true, and it is the one a NaN or a clock that wraps would need. The accumulator arms go further than the function: a.n == 24 vs a.nfill == 12 on the same run, a.fill == 12 * 6.25, and a.pack / a.n == 0.25 — i.e. "pack per launch is what a cached launch pays", which is the quantity the encoder was
misreading.

And it bites: with the clamp removed (return pack - fill;), the test goes rc 1 with exactly the intended arm --

FAIL fill > pack: 0, not negative

Built and ran at this head: 0 errors, prof_acc: ok, and add_executable(prof_acc_test …) now sits at CMakeLists.txt:591, so CI compiles the header through a real target rather than only through the private-runtime TU.

That last point is the reason this PR is now READY rather than "READY with a note": before this commit the only file that changed was xdna_target.cpp, which no CI job can compile (fatal error: npu_engine_i8ctx_inc.h: No such file or directory). The
rule now lives where a build sees it, and the device-side call site is reduced to a call -- so the next edit of this arithmetic has a test, and the file that cannot be compiled in CI no longer holds logic of its own.

Unchanged and still verified from 5b4cd34: the new [prof] … weight-cache fills line does not match scripts/npu-window11.sh's row regex (tested against its exact pattern), it is printed only when there were fills, wait/quant+lnch/readback are untouched, and the device numbers (48 fills = 12 layers x 4 roles, pack 2.7-3.5 ms -> 0.00) are the ones the body reports.

[github-actions]

Failed to review PR

[github-actions]

Persistent review updated to latest commit 2a5d368

[bong-water-water-bong]

VERDICT at 2a5d368: READY

They took the clamp note and turned it into a total rule over the whole domain — including a case I did not think of.

The rule now lives in npu/target/xdna/prof_acc.h, a header CI compiles, with six arms:

fill == 0        -> pack unchanged      (a cached launch)
0 < fill < pack  -> pack - fill
fill == pack     -> 0
fill > pack      -> 0, not negative     ← the clamp I asked about
a negative fill is no fill              ← the case I did not
                        the fill total is the fills' own time   (accumulator over a warm-up-shaped window)

The fifth arm is the one worth crediting. I wrote that the clamp pack > fill ? pack - fill : 0.0 would hide the case where fill exceeds pack, and suggested a count. They kept the clamp and instead pinned the whole domain — zero, partial, equal, exceeding — and added negative fill, which is nonsense input that a naive pack - fill would silently add to the per-launch column. That is the difference between patching a case and stating a rule.

And it is now testable where it matters. The first version's rule was inside xdna_target.cpp, which needs the device lane to exercise; moving it into prof_acc.h with tests/prof_acc_test.cpp means CI gates it on a host, which is the same move #68 needed for its prescale and #69 for its matcher. Three PRs in a row now have decided that a rule worth having is a rule worth extracting.

The sixth arm deserves a mention of its own: asserting a.fill == 12 * 6.25 over a window shaped like the warm-up pass (half fills, half hits) checks the accounting rather than the formula — the fills' time is neither lost from the report nor left in the per-launch column.

No concerns.