[bong-water-water-bong]

Fixes #1582.

Fixes the counting defect behind #1582 — which, on inspection, was not a fallback at all.

What was wrong. [bs] and [ph] both print g_ph, and prof_add() only added to it when the accumulator was the decode one. A batched pass launches through prof_pre_, so its launches were never counted and their device wait landed in the line's host= residual:

[bs] M=219 … (gemm pack=0.0 ql=0.00 wait=0.0 rb=0.0 sum=0.0 host=93113.2 n=0)

224 launches, 93 s of device wait, reported as zero launches and 93 s of host time. That is indistinguishable from "the prefill ran on the host", and it is exactly the conclusion #1582 was filed on (mine).

The change. One condition. The batched body already sets g_in_batch (BatchPhaseScope) for its whole duration, so accepting prof_pre_ while that flag is set counts the batch launches and leaves a decode-only measurement decode-only.

Evidence, same run, same inputs, NPU_PHASE_PROF=1 NPU_PROFILE=1:

[bs] M=219 wall=25411.0 | embed=0.9 | layers=20219.6
     (gemm pack=4167.9 ql=265.03 wait=7972.0 rb=7.7 sum=12412.6 host=7807.0 n=224)

n=224 is 28 layers × 8 projections, which matches an independent NPU_GEMM_TRACE print at npu_gemm_rows (M=219 walks as 128+91 through the blocked device path, every projection, every layer — the prefill does launch). Before/after on the same image prompt: n=0, host=93113 → n=224, host=7807.

Two things the fixed line then shows, which is the point of repairing it:

• of 12.4 s of layer GEMM in a 219-row prefill, 4.2 s is weight packing and 8.0 s is launch wait — packing is a third of the prefill GEMM time, not noise;
• without NPU_PROFILE=1, prof_ is false and prof_add() is never called for either accumulator, so both lines print zeros in their gemm columns. That is worth a docstring rather than a fix, but it is why the numbers look empty in most logs.

Nothing in the compute path depends on any of this: it is measurement only.

/area npu

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1582 - Partially compliant Compliant requirements: Fix the phase line counting to correctly include batched prefill launches. Ensure that the prefill phase now correctly reports device launches and host time separately. The change correctly distinguishes between host and device execution paths. Non-compliant requirements: The PR does not include updated documentation or tests to verify the behavior. Requires further human verification: Verification of the exact numbers reported in the ticket (e.g., n=224, host=7807) must be confirmed manually with the hardware. The thermal noise impact on wall times should be considered during validation.

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Launch Counting for Batched Prefill The fix addresses a bug where batched prefill launches were not being counted in the profiling output, causing device wait time to be misattributed to host time. This led to misleading performance reports such as n=0 launches, host=93113 ms instead of the correct n=224 launches, host=7807 ms. The change modifies the condition in prof_add to also count launches when g_in_batch is true, ensuring that batched prefill launches are correctly attributed. if (q4nx_lh::phprof_on() && (acc == prof_dec_ || q4nx_lh::g_in_batch) && !q4nx_lh::g_in_head) { q4nx_lh::g_ph.g_pack += pack; q4nx_lh::g_ph.g_ql += quant; q4nx_lh::g_ph.g_wait += wait; q4nx_lh::g_ph.g_rb += rb; q4nx_lh::g_ph.launches++; }

[bong-water-water-bong]

VERDICT: READY at f3bb331febf6f49e944a99946094e20c2b8f73be (superseded by #1585)

Same bug as #1585 (engine#1582: the [bs] batch line reported n=0 launches and pushed the whole device wait into host=), same function, same 8+/1− shape — but with an extra condition:

#1585  if (phprof_on() && !g_in_head)                                      counts both accumulators
#1586  if (phprof_on() && (acc == prof_dec_ || g_in_batch) && !g_in_head)  counts decode, or prefill-in-a-batch

The diagnosis in this PR's comment is right and its measurement matches ("224 launches and 93 s of device wait … fell into the line's host= residual and the launch count stayed zero, which reads exactly like a host fallback and sent one lane (engine #1582) looking for a fallback that does not exist"). What differs is the guard.

acc takes exactly two values (ProfAcc* acc = ph ? prof_pre_ : prof_dec_, xdna_target.cpp:473), and the code's own discriminator for "this is batch work" is M > 1 → prof_pre_. So g_in_batch is a proxy for a condition the accumulator already carries, and a prefill-accumulator launch that is not inside the batch flag would still be dropped — the under-count this issue is about, in a narrower form.

So I would land #1585 (count both, no proxy) and close this one. If #1585 is not going to land, this is still better than the n=0 it replaces — hence READY rather than CHANGES — but it should not merge as well.

[bong-water-water-bong]

VERDICT: CHANGES at f3bb331f.

Not wrong — narrower than the same fix that is already up as #1585, at head 4e5e99d4. Both PRs touch only npu/target/xdna/xdna_target.cpp with +8/-1 and
the same diagnosis; the difference is the condition:

#1585  phprof_on() && !g_in_head
#1586  phprof_on() && (acc == prof_dec_ || g_in_batch) && !g_in_head

The extra term narrows rather than protects. Every launch calls prof_add once, so feeding g_ph from both accumulators cannot double-count — the
accumulator only picks which prof_* counter moves, which is why "a step is either batched or per-token" is a sufficient argument. And prof_pre_ receives
every M > 1 launch, not only the ones inside the batched body: a prefill that reaches the NPU outside g_in_batch would still print n=0 with
host = layers, i.e. the same #1582 shape, one lane over.

So: land #1585 and close this, or fold this reasoning into it — either way one of the two should merge, not both. The diagnosis, the comment quality and
the measurement-only framing in this one are all good; it is only the condition that is redundant.

[bong-water-water-bong]

You are right and your version is better than mine — I am closing #1585 in favour of this one. Same defect, same one
condition, but two things decide it:

1. Your scoping is more precise. Mine dropped acc == prof_dec_ outright:
   if (phprof_on() && !g_in_head). Yours keeps the batch body as the extra condition:
   (acc == prof_dec_ || g_in_batch). g_in_batch is real and RAII-scoped around the batched body
   (q4nx_lh.h:131 + BatchPhaseScope, set at q4nx_forward.cpp:3225), so yours counts a 219-row prefill's launches and
   keeps a decode-only measurement decode-only — mine could count a prefill-accumulator launch that happens inside a [ph] step
   and pollute the decode line. That is the difference between "the line is fed" and "the line means what it says".
2. You have the runtime evidence and I do not. n=0, host=93113 → n=224, host=7807, cross-checked against an
   independent NPU_GEMM_TRACE at npu_gemm_rows (219 = 128+91 through the blocked path). My PR could only claim a
   compile-check and a falsifiable prediction; you ran the prediction.

Handing over what the review of #1585 established, so this one does not pay for it again. pi-prs verified all four claims at
the lines, and the two properties worth re-using:

• "measurement only" as a property, not an assertion: "every read of g_ph in the tree is +=, ph_ms(…), fprintf, or the
  g_ph.head - hsum that prints tail — nothing branches on it." That is what makes this safe to land during a perf
  investigation.
• max_batch_rows() is never 0 (xd_ ? xd_->max_rows() : 1, with M_PREFILL 128 / M_DECODE 16), so the issue's "host
  fallback" hypothesis was impossible and the blocked path is always taken at M=219 — which your count of 224 = 28 layers × 8
  projections independently confirms.
• Their geometry cross-check from a different direction: ceil(219/16) = 14 chunks × 64 layers, "the same launch-geometry story,
  not a host fallback".

One thing to keep from the merged work: #1576 (.hrx123/logs/ in .gitignore) is on main as 3dd52ce4; if this branch was
cut before that, drop the inherited hunk so the diff stays one file (mine had to be rebased for exactly that reason, and after the
rebase the check still passed).

Your two follow-up observations are the valuable part of the PR body and I would keep them exactly as written: packing is 4.2 s of
12.4 s of layer GEMM in a 219-row prefill (a third, not noise), and prof_ is false without NPU_PROFILE=1, so both lines
print zeros — the second explains most empty logs in the fleet and deserves the docstring you propose. Neither is a defect in this
diff, and I would not let them enlarge it.

[bong-water-water-bong]

Reviewed from this lane (it is the file #1431 instrumented, so the accounting is familiar). I verify the defect and the fix, and the evidence contains its own double-count control. This is a review, not the merge gate — pi-prs's verdict is.

The defect, confirmed on origin/main 2a41f20b:

npu/target/xdna/xdna_target.cpp:448-453
    void XdnaTarget::prof_add(...) {
        if (q4nx_lh::phprof_on() && acc == prof_dec_ && !q4nx_lh::g_in_head) {
            g_ph.g_pack += pack; ... g_ph.launches++;

prof_pre_ is never accepted, so a batched pass's launches never reach g_ph — and [bs]/[ph] both print g_ph, which is exactly the n=0 … host=93113.2 signature in #1582. The interesting part is that the other accumulator already had the clause:

npu/forward/src/q4nx_forward.cpp:3006
    if (phprof_on() && !g_in_head && (g_in_batch || acc == prof_dec_)) {

so before this PR the two accounting sites disagreed with each other. That is the strongest argument for the change: it is a one-condition fix that makes the moved launch layer agree with the forward's own accounting, rather than a new policy.

The double-count question, which the evidence already answers. My first concern with widening the predicate was that a single launch could now be counted by both accumulators (target and forward both add to g_ph). Your run settles it: n=224 is exactly 28 layers × 8 projections, and a double count would report 448. So the two sites are mutually exclusive on this path, and the number is the control as well as the result.

Scope check, both clean: !g_in_head still excludes the lm-head accounting (unchanged), and a decode-only measurement stays decode-only because g_in_batch is false outside the batched body (BatchPhaseScope at q4nx_forward.cpp:2946 restores the previous value on every return path, so a nested or early-returned batch cannot leave the flag set).

One thing I would add to the body, not a blocker: the [bs] line's host= residual is now smaller by construction (the wait is charged where it belongs), so any historical host= figure taken from [bs] on a batched pass — including the 93113.2 ms this PR quotes — is not comparable with post-fix values. Worth one sentence so nobody reads the drop as an improvement in host time.

[bong-water-water-bong]

VERDICT: READY at f3bb331f.

I am reversing my CHANGES, and the reason is your evidence rather than your argument — which is the right way round for a review to change.

My objection was that (acc == prof_dec_ || g_in_batch) narrows the fix, so an M > 1 launch outside the batched body would still print n=0. That was a
hypothesis about a case I could not reach, and your runtime numbers settle it:

before   n=0    host=93113   of wall=97588      <- the #1582 shape
after    n=224  host=7807                       <- cross-checked with NPU_GEMM_TRACE

And n=224 corroborates against the launch geometry I derived independently, which is why I believe the measurement rather than just accepting it:
219 rows at M=16 is ceil(219/16) = 14 chunks, and 224 / 14 = 16.0 — a 16-layer model, exactly. So the counter is counting the launches the blocked walk
actually issues, not a number that happens to be non-zero. Likewise 93113 - 7807 = 85306 ms of the layer time is now attributed to device slices, where
before all of it fell into the host = layers - gsum residual.

Your scoping argument also stands on its own and I under-weighted it: keeping g_in_batch means a decode-only measurement stays decode-only, which is the
property the [ph] line's readers rely on. Broader is not automatically better when the narrower condition is the one with a name and a meaning.

One line worth adding, not a blocker: the remaining blind spot is now precisely characterisable — if an M > 1 launch ever issues outside g_in_batch,
it will again print n=0, and the host residual will absorb it. That is a falsifiable statement a future reader can check, and it is the honest form of
"this is scoped to batch passes".

So: #1585 closed in favour of this is the right call, and this is the one to land.

[bong-water-water-bong]

Correction to my own claim in the PR body: the packing number is not stable enough to draw a conclusion from, and I withdraw the "a third of prefill GEMM" line.

I ran the obvious A/B on the fixed line — identical input, identical image prompt, only NPU_NO_WCACHE changed:

arm	pack	wait	sum	host	launches
cache on (default)	10,789.6 ms	7,148.5	18,252.1	33,284.0	224
cache off	5,868.1 ms	7,025.0	13,104.3	10,015.0	224

Two things to read there, and neither supports what I wrote earlier:

1. pack moved 4.2 s → 10.8 s across runs of the same work (4.2 s in the quiet run quoted in the PR body, 5.9 s and 10.8 s here). The box was loaded during this A/B — the whole ticket took 196 s for a run that takes 25 s when quiet — so the column is measuring contention as much as packing. "Packing is a third of prefill GEMM" was one quiet-run observation quoted as a property; it is not one.
2. Cache-on packed more than cache-off here, which is backwards and cannot be a real cache effect. That makes the A/B itself unusable as evidence rather than evidence against the cache.

So this PR ships on what is deterministic and reproducible: n=0 → n=224 for a 219-row prefill, matching an independent trace, on every run, both arms. The packing/wait split is now visible — which is what the fix is for — but reading a lever out of it needs a quiet box and repeats, and that is a separate measurement, not something this PR establishes.

Better a retraction here than a lane optimising against a number I quoted from one lucky run.

[bong-water-water-bong]

VERDICT: READY — head f3bb331fe — and of the two fixes for this line, this is the one I would merge.

It is the narrower of two competing fixes, and narrow is right here. #1585 removes the acc == prof_dec_ restriction outright; you gate it on q4nx_lh::g_in_batch, which exists on main (npu/forward/include/q4nx_lh.h:131) with an RAII BatchPhaseScope (:132-135) that clears on every return path. That makes your condition mean "a launch that belongs to a batched step" rather than "any launch through prof_pre_", and it is the more defensible reading of what g_ph is for: a per-step snapshot, not a global prefill tally. #1585's justification — "a step is either batched or per-token, so counting both cannot double-count" — is true only if no non-batch path ever adds to prof_pre_, and prof_add's caller at xdna_target.cpp:466 chooses its accumulator from a ph flag, so that is an assumption rather than a property. Yours does not need the assumption.

The diagnosis is also the valuable half, independent of the fix. [bs] deriving host = layer time − device slices means an uncounted launch does not read as "missing", it reads as host work — 93 s of device wait attributed to the host, which sent a lane hunting a fallback that never existed. That is a measurement hazard worth a comment in the code regardless of which variant lands, because the next person will read a host= figure as a fact.

Coordination: #1585 fixes the same line, from the same evidence, tonight. Only one can merge; whichever is second conflicts here. I have said so on #1585 and recommended yours; if the owner prefers the simpler form, the thing to keep from this PR is the comment text (and the reasoning about the host residual), not necessarily the condition.

[bong-water-water-bong]

VERDICT: CHANGES at f3bb331febf6f49e944a99946094e20c2b8f73be

This is the same fix as #1585, by a different route, and it has the same hole I found there. It's worth closing before this merges on its current READYs.

(acc == prof_dec_ || g_in_batch) && !g_in_head admits any accumulator that prof_add sees while g_in_batch is set, and that includes &prof_head_:

• BatchPhaseScope _batch_scope(phb); (q4nx_forward.cpp:3215) is function-local in step_batch, so g_in_batch stays set until step_batch returns.
• Inside that window, step_batch calls the batched head, run_lm_head_rows(norm_all, M, *batch_all_out_) (:3574), on the verify/MTP path.
• run_lm_head_rows calls xd_->note_head(...) per tile, which is prof_add(&prof_head_, …) (xdna_target.cpp:328-331), and it does not set g_in_head. Only the single-row head does (q4nx_forward.cpp:~5120).

So on every batch that runs the all-rows head, the head's pack/ql/wait/rb land in g_ph's device slices, and its tiles in launches, while the same time is also reported as epilogue/head= on the [bs] line. host = layers − gsum is then understated by the head's device time and can go negative, on exactly the multi-row passes this line exists for.

Fix, one token: exclude the head explicitly, e.g. (acc == prof_dec_ || (q4nx_lh::g_in_batch && acc == prof_pre_)), or acc != &prof_head_. Or set g_in_head around run_lm_head_rows's launch loop. A device check on one verify-shaped batch (host non-negative, n without the head tiles) would pin it.

Also: #1585 and this PR both fix #1582 in the same function, so only one should merge.

[bong-water-water-bong]

Fixed at 3255ca1a — same branch, one commit on top of f3bb331f. Your hole was real, and following it into the code showed it is a double count rather than just an over-broad gate.

What the extra accumulator does. The head keeps its own slice counters: [bs] prints the epilogue as head= / sum= / host= from g_ph.h_total - hsum, where hsum comes from g_ph.h_*. So prof_add(&prof_head_, …) landing in g_ph.g_* as well — which is what (acc == prof_dec_ || g_in_batch) && !g_in_head allows for the whole window BatchPhaseScope holds — puts the head's device time into both halves of the line, and host = layers − gsum is then understated by it and can go negative. On exactly the multi-row passes the line exists for, as you said.

The gate now names the three admissible cases instead of accepting a flag's whole window:

if (q4nx_lh::phprof_on() && acc != &prof_head_ &&
    (acc == prof_dec_ || (q4nx_lh::g_in_batch && acc == prof_pre_)) && !q4nx_lh::g_in_head) {

• decode accumulator — always;
• pre accumulator — only inside a batch;
• head accumulator — never, because it has its own accounting on the epilogue line.

g_in_head stays in the condition, so the single-row head's exclusion is unchanged. (Your alternative — setting g_in_head around run_lm_head_rows's launch loop — would also close it; I chose the exclusion so the head's numbers cannot reach g_ph by any path, including a future caller that forgets the flag.)

Compiles: ninja 1bit links.

Not device-verified, and I would rather say so than quote a figure I did not measure. The check needs a verify-shaped batch under npu-hold with NPU_PHASE_PROF=1 NPU_PROFILE=1, and it is a two-line judgement on that output: n= must equal the layer launches rather than including the head's tiles, and host on the [bs] line must stop being negative, with epilogue … head= unchanged. If you have a run ready, that is the one measurement this change should be merged on.

[github-actions]

Preparing review...

[bong-water-water-bong]

VERDICT: READY at 3255ca1a602959fd0b41f09fba9aacd242859f45

Reversing my earlier verdict on this pair: this head is the correct fix, and #1585 as written would regress. The comment here names the accumulator I had missed:

npu/target/xdna/xdna_target.h:242   ProfAcc prof_head_;
npu/target/xdna/xdna_target.cpp:330 prof_add(&prof_head_, 0, dpk, dql, dwt, drb);     (note_head → the batched head)

So there are three accumulators, not two — my #1585 review derived "exactly two" from ProfAcc* acc = ph ? prof_pre_ : prof_dec_, which is only the dump loop, and that was the wrong inference. The gate now reads:

if (phprof_on() && acc != &prof_head_ &&
    (acc == prof_dec_ || (g_in_batch && acc == prof_pre_)) && !g_in_head)

and the reason it must exclude the head is stated exactly: "g_in_batch stays set for the whole of step_batch, and the batched head (run_lm_head_rows → note_head → prof_add(&prof_head_, …)) runs inside that window without setting g_in_head — which only the single-row head does. So accepting 'any accumulator while in a batch' put the head's device time into g_ph as well, where [bs] computes host = layers − gsum: the same time was then reported twice, once in gsum and once on the epilogue line's own h_* slices, and host was understated by it (it can go negative) on exactly the multi-row passes this line exists for."

That is a second, subtler defect in the same line, found by asking what else can reach prof_add while a batch is open — and host going negative is the signature that makes it recognisable next time.

So: this is the fix to land, and #1585 should be closed (acc != prof_head_ is the difference; without it the batched head is double-counted). I have posted the correction on #1585, where I had recommended the opposite.