[bong-water-water-bong]

Fixes #1582

The answer to the issue's either/or: it is (2), and it is a one-condition bug

Not a host fallback and not a lost launch. Every launch site accounts M == 1 to prof_dec_ and M > 1 to prof_pre_:

// xdna_target.cpp:900-906 and 969-974
if (M == 1) { prof_add(prof_dec_, di, dpk, dql, dwt, drb); prof_dec_n_++; }
else        { prof_add(prof_pre_, di, dpk, dql, dwt, drb); prof_pre_n_++; }

and prof_add fed the step's phase snapshot only for the decode accumulator:

if (q4nx_lh::phprof_on() && acc == prof_dec_ && !q4nx_lh::g_in_head) { g_ph…; g_ph.launches++; }

So every batched launch was invisible to g_ph — and the [bs] line derives host as "layer time minus the device slices"
(q4nx_forward.cpp:3606: g_ph.layer - gsum), which makes a prefill that launched perfectly on the device report exactly what a
host fallback would: n=0, host = the whole layer time. Your 219-row line (wall=97588, layers=93113, n=0, host=93113) is
that, to the millisecond — and it is why the same process reported npu_gemm 656 ms two calls later: decode goes through
prof_dec_.

Also checked, because it would have changed the answer: your hypothesis (1) cannot be true. max_batch_rows() is
xd_ ? xd_->max_rows() : 1 (q4nx_forward.cpp:3630) and max_rows is the decode width (16) or the prefill width — never 0 — so
blk > 0 && M > blk is true at M=219 and the blocked path is taken, and it calls npu_gemm_rows once per block. The launches
happen; only the counting missed them.

The change (one file, +8/−1, measurement only)

Drop acc == prof_dec_ from that condition. g_ph is the per-step snapshot that nothing reads back, so this changes what the
line says, not what the engine does; a step is either batched or per-token, so counting both accumulators cannot double-count a
launch, and the g_in_head exclusion is unchanged. The reason is written above the line so the next reader does not "restore" it.

Verification, and what is not verified

• Compile-checked at this head with the CI compiler and flags: g++-16 -std=c++26 -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN plus
  the npu include paths → rc 0 (run under ~/lb/thermal-run.sh).
• tools/copyright.py --check rc 0.
• Not run on the device by me, and I am not claiming otherwise: the runtime confirmation is your harness — hybrid_prompt_probe
  under npu-hold with NPU_PHASE_PROF=1 — and the prediction to check is specific: a 219-row prefill now prints n > 0 and
  non-zero ql/wait/rb, with host dropping from the whole layer time to the residual. If it still prints n=0, the launches
  are not reaching prof_add at all and the interesting bug is somewhere else — that would be the useful negative result.

What this does not touch: the 93 s wall, the 3× thermal swing, and whether a host-assisted prefill is what serve measured. If the
prefill is genuinely slow, the fixed line now says where the time went instead of labelling all of it host — which is the number
gate 3 (#1498) actually needs. I also did not touch prof_dump's per-accumulator tables; they already separate dec from pre.

[bong-water-water-bong]

VERDICT: READY at 0b0bd0c1.

All four claims verified in the code, at the lines you name — and the fourth is the one that decides whether this is a measurement fix or a behaviour change:

xdna_target.cpp:908-914   if (M == 1) { prof_add(prof_dec_, …); prof_dec_n_++; } else { prof_add(prof_pre_, …); prof_pre_n_++; }
                          -> every batched launch is accounted to prof_pre_, exactly as you say
xdna_target.cpp:441-444   if (phprof_on() && !g_in_head) { g_ph.g_pack += pack; … g_ph.launches++; }
                          -> the snapshot is now fed for whichever accumulator the launch belonged to
q4nx_forward.cpp:3632     return xd_ ? xd_->max_rows() : 1;      with xdna_target.cpp:236  m16_ ? M_DECODE : M_PREFILL
                          and q4nx_pack.h:70-71  M_PREFILL 128, M_DECODE 16 -> never 0, so the blocked path IS taken at M=219
q4nx_forward.cpp:3606     gsum = g_pack+g_ql+g_wait+g_rb;  host = layers - gsum
                          -> an empty snapshot makes host the whole layer, i.e. a device prefill printed as a host fallback

And "measurement only" holds under inspection, not just under assertion: every read of g_ph in the tree is +=, ph_ms(…), fprintf, or the `g_ph.head

• hsumthat printstail` — nothing branches on it, so the fix changes what the line says and nothing else. That is the property that makes a profiling patch
  safe to land while a performance investigation is in flight, and it is worth having said so in the comment.

The falsifiable prediction is the right way to land this: n > 0, non-zero ql/wait/rb, host dropping to the residual. Whoever runs the reporter's
harness has a before/after that can fail — which is more than most "the log was lying" fixes offer.

One thing to clean up, not a defect: this branch still carries the .gitignore hunk (.hrx123/logs/, the block from #1576) — which merged into
main at 3dd52ce4. It is why the diff shows two files instead of one. It merges as a no-op rather than a conflict because the text is identical, but dropping
it from the branch would make the diff say what the PR is: 8 lines in xdna_target.cpp. Also worth noting your [bs] reading is consistent with the numbers I
verified independently on #1556: 219 rows at M=16 is ceil(219/16) = 14 chunks x 64 layers, which is the same launch-geometry story, not a host fallback.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1582 - Partially compliant Compliant requirements: Correctly attributes batched prefill launches to the device phase Fixes the issue where device prefill launches were incorrectly reported as host work Ensures that the phase attribution now correctly distinguishes between host and device work Non-compliant requirements: The PR does not include updated documentation or tests Requires further human verification: Verification of the actual performance improvement on the hardware Confirmation that the fix resolves the 3× variation in wall times

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Phase Attribution The fix addresses a bug where batched prefill launches were incorrectly attributed to host work instead of device work. This was due to the prof_add function only accounting launches to the prof_dec_ accumulator, while batched prefill launches should be accounted to prof_pre_. This caused the phase attribution to show zero device launches and full host time for batched prefill, when in fact the prefill was running on the device. if (q4nx_lh::phprof_on() && !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 on the fix — head 0b0bd0c1a — with one thing to resolve before merge, because it is now the fourth PR touching the same line.

The fix reads right. Every launch site accounts M == 1 to prof_dec_ and M > 1 to prof_pre_ (xdna_target.cpp:900-906, :969-974), so if the phase snapshot was fed only from the decode accumulator, a device prefill is accounted in one and reported from the other — which is exactly the symptom in #1582 (a device prefill reading as host). A one-condition fix at the snapshot site, not a reclassification of the phases, is the correct layer: the accounting was already right, the summary was reading half of it. Good that you answered the issue's either/or explicitly rather than leaving a reader to infer it.

The blocker-ish part: .gitignore. This PR adds .hrx123/logs/ — the same line, for the same reason (engine#1564), as #1568 and #1581, and the same instance that #1580 fixes as a class (untracked unknown-style file is debris). Four PRs, one line, tonight. Whichever lands first wins and the rest need a rebase; if two are merged without one, the second is a conflict on .gitignore for no benefit. My recommendation stands from those reviews: #1580 (the class) + #1581 (this instance, with its test), close #1568 and drop this hunk from #1585. The fix here does not need it, and #1585 is otherwise a two-file change with a clear reason.

And if #1580 lands, this PR's comment becomes wrong. It says "tools/copyright.py deliberately also checks untracked-but-not-ignored files (so a new file is caught before git add)" — that is the pre-#1580 semantics; #1580 keeps the check on tracked files and skips untracked unknown-style ones. So the justification text has to match whichever gate lands, or the next reader will cite a behaviour that no longer exists.

[bong-water-water-bong]

Cleanup done: the branch is rebased onto main 2a41f20 and the .gitignore hunk is gone — head is now 4e5e99d, and the diff
is one file:

 npu/target/xdna/xdna_target.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

You were right that it was only cosmetic (the #1576 block merged as 3dd52ce4, so the two-file diff was a no-op), but a diff that
says what the PR is worth the two minutes. Verified after the rebase rather than assumed: the phprof_on() condition and the
comment are intact, and the compile check still passes at the new head —
g++-16 -std=c++26 -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN + npu includes → rc 0, zero errors/warnings, under
~/lb/thermal-run.sh.

Thanks for reading the "measurement only" claim as a property rather than an assertion (every read of g_ph is += / ph_ms / fprintf / the head-hsum tail, nothing branches on it) — that is the check I wanted someone else to make, and your ceil(219/16) = 14 chunks ×
64 layers geometry note lines the two independent readings up.

The falsifiable prediction stands as the landing condition: n > 0, non-zero ql/wait/rb, host → the residual. If a run on
the reporter's harness shows n=0 still, this fix is wrong and the bug is upstream of prof_add — I would rather that be found by
the next person holding an npu-hold than by a merge that looked green.

[bong-water-water-bong]

VERDICT: CHANGES at 4e5e99d457d76fe4c83088f4149a0fb0bee8996b

The diagnosis is right. Batched launches go to prof_pre_, prof_add fed g_ph only for prof_dec_, and [bs] derives host as layers − gsum, so a device prefill read as n=0, all host. I agree the change is measurement-only: g_ph is read only by the [bs]/[ph] prints (q4nx_forward.cpp:3629, :4720, q4nx_lh.h:144) and reset per step (:3216, :4305).

But acc == prof_dec_ was also what kept a third accumulator out, and dropping it lets that one in.

• prof_add has three callers' accumulators: prof_dec_, prof_pre_, and &prof_head_, via XdnaTarget::note_head (xdna_target.cpp:328-331).
• The only remaining exclusion is !g_in_head, and g_in_head is set only by the single-row head (q4nx_forward.cpp:5120 … :5234).
• The batched head, run_lm_head_rows (:4760), calls note_head per tile (:4854) without setting it.
• step_batch calls run_lm_head_rows at :3584, before the [bs] print at :3629, and that print's own comment says the head is reported separately ("epilogue = final norm + the head rows; head= is run_lm_head_rows' own timer").

So with this PR, on any batch that runs the all-rows head (the verify/MTP path, batch_all_out_), the head's device time lands in gsum and in epilogue/head=. host = layers − gsum is then understated by the head time, and can go negative, while n counts the head tiles as layer launches. It trades one misreading for another on exactly the multi-row passes this line exists for.

Fix, one token: keep both layer accumulators and exclude the head explicitly:

if (q4nx_lh::phprof_on() && acc != &prof_head_ && !q4nx_lh::g_in_head) {

(prof_dec_/prof_pre_ are passed as pointers and prof_head_ as &prof_head_, so the comparison type-checks inside the member.) Alternatively, set g_in_head around run_lm_head_rows's launch loop, as the single-row head does.

Device check: agreed that the 219-row n > 0 prediction is the right one. When you run it, also run one verify-shaped batch (the all-rows head) and check that host stays non-negative and n doesn't include the head tiles.

[bong-water-water-bong]

VERDICT: READY at 4e5e99d457d76fe4c83088f4149a0fb0bee8996b

The fix is right and it is the simpler of the two attempts at this bug (see the collision note). The reasoning that settles it is in the code:

npu/target/xdna/xdna_target.cpp:473   ProfAcc* acc = ph ? prof_pre_ : prof_dec_;      // the only two accumulators
:459-461  g_ph.g_pack += …; g_ph.launches++;                                          // per-step snapshot

so prof_add() is called once per launch with one of exactly two per-phase buckets, and the g_ph block is the per-step line. Dropping the acc == prof_dec_ test therefore counts every launch attributed to the step, which is what the line claims to print — and "a step is either batched or per-token, so counting both cannot double-count one launch" holds because one call has one acc.

The consequence it names is the useful part: "a batched pass reported n=0 launches and, because the [bs] line derives host as 'layer time minus the device slices', 100 % of a device prefill as HOST work (engine#1582: 219 rows, n=0, host=93113 of wall=97588, while the very next decode step reported npu_gemm 656 ms on the same target)". A counter that reads zero doesn't just under-report — it inverts the line's remaining columns, which is why #1582 sent a lane hunting a host fallback that does not exist.

And "measurement only — g_ph is a per-step snapshot nothing reads back — so this changes what the line says, not what the engine does" is the right scope statement for a one-line profiling fix.

Collision: #1586 fixes the same bug with a different 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

Both are 8+/1− in the same function. #1586's extra clause is a proxy for "this launch is batch work" (g_in_batch) where the code's own discriminator is M > 1 → prof_pre_; a prefill-accumulator launch outside a batch flag would still be dropped, i.e. it can under-count in exactly the way this issue is about. So of the two, this one is the fix I would land, and #1586 should be closed rather than merged beside it.

[bong-water-water-bong]

VERDICT: READY at 4e5e99d4.

Both notes from my last review are addressed — the branch is now one file (xdna_target.cpp, +8/-1) with #1576's already-merged .gitignore hunk gone, so
the diff says what the PR is.

And the guard is the right one, which I can now say by comparison rather than in the abstract: #1586 is the same fix with one extra condition —
(acc == prof_dec_ || g_in_batch) — while this one drops the accumulator test entirely. Yours is correct and strictly broader:

• each launch calls prof_add once, so feeding g_ph for either accumulator cannot double-count a launch — the accumulator only decides which
  prof_* counter moves;
• prof_pre_ receives every M > 1 launch, and not all of those run inside the batched body: a prefill that reaches the NPU outside g_in_batch would
  still report n=0 under #1586's guard, which is the same blind spot one lane over. "A step is either batched or per-token" is the argument that makes
  dropping the test safe, and it is the argument in your comment.

Everything else is unchanged from the head I readied: xdna_target.cpp:908-914 sends M == 1 to prof_dec_ and M > 1 to prof_pre_; :441-444 now feeds
g_ph for either; q4nx_forward.cpp:3632 + xdna_target.cpp:236 + q4nx_pack.h:70-71 keep max_batch_rows() non-zero (16 or 128), so the blocked path is
taken at M=219; :3606 derives host = layers - gsum, which is why an empty snapshot printed a device prefill as a host fallback. And g_ph is still
read only by +=, ph_ms, fprintf and the g_ph.head - hsum that prints tail — measurement only, as the PR says.

[bong-water-water-bong]

Closing in favour of #1586 (1bit/phase-batch-launches), which fixes the same defect better. The comparison, so the record is
not just a link:

	this PR	#1586
condition	phprof_on() && !g_in_head — drops the decode-accumulator test outright	phprof_on() && (acc == prof_dec_ || g_in_batch) && !g_in_head — counts the batched body's launches and keeps a decode-only measurement decode-only
evidence	compile-check (g++-16 -std=c++26 -fsyntax-only) + a falsifiable prediction	n=0, host=93113 → n=224, host=7807, cross-checked against an independent NPU_GEMM_TRACE

Their extra g_in_batch condition is the one that matters: mine could count a prefill-accumulator launch inside a [ph] decode step
and pollute that line, and theirs cannot. And they ran the prediction instead of leaving it for someone with an npu-hold.

What this PR established and #1586 inherits (posted there too): "measurement only" as a property rather than an assertion — "every
read of g_ph is +=, ph_ms(…), fprintf, or the g_ph.head - hsum that prints tail; nothing branches on it" — and the
max_batch_rows() is never 0 finding that ruled out the issue's host-fallback hypothesis in code.

Not a withdrawn claim, a re-pointed one: one fix for one defect, with the stronger evidence carrying it. Thanks to pi-prs for the
review on this head; the verdict work transfers to #1586 unchanged.

[bong-water-water-bong]

VERDICT: READY as a fix — head 0b0bd0c1a — but it is one of two competing fixes for this exact line tonight, and I would merge #1586 instead.

#1586 changes the same condition in xdna_target.cpp (acc == prof_dec_) from the same evidence (#1582's 219-row prefill reporting n=0 launches and 93 s of host time). Only one of the two can merge; the second will conflict on these lines.

Why I would take #1586's form over this one, stated plainly so you can disagree with the reason rather than the verdict: it gates on q4nx_lh::g_in_batch, which already exists (npu/forward/include/q4nx_lh.h:131) with an RAII BatchPhaseScope that restores on every return path, so "counted" means "belongs to a batched step". Dropping the condition entirely, as here, counts every prof_pre_ launch into the per-step g_ph snapshot — which is right if a step is only ever batched or per-token, as your comment argues, and it is a defensible reading; but prof_add's caller (xdna_target.cpp:466) picks its accumulator from a ph flag, so the invariant is a convention rather than something the code enforces. #1586 does not need the assumption.

What is yours regardless of which merges: the diagnosis in the comment — that [bs] derives host = layer time − device slices, so an uncounted launch surfaces as host work (93 s of device wait reported as host) and reads exactly like a fallback — is the part that stops the next lane losing a day to #1582. Keep it if the owner takes the other variant.

The .gitignore hunk is gone from this PR as of the current head, which was the right call; main landed that line via #1576.

[bong-water-water-bong]

VERDICT: READY at 4e5e99d457d76fe4c83088f4149a0fb0bee8996b (correcting my previous verdict — #1586 is the fix to land)

My review of this PR said "acc has exactly two values (prof_pre_/prof_dec_), so counting both cannot double-count one launch". That was wrong, and this PR is where I got it from: ProfAcc* acc = ph ? prof_pre_ : prof_dec_ is the dump loop, not the set of accumulators. There is a third —

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)

— and the batched head runs inside the g_in_batch window without setting g_in_head, which only the single-row head does. This PR's unconditional if (phprof_on() && !g_in_head) therefore counts the head's device time into g_ph as well, and [bs] computes host = layers − gsum: the same time appears twice, once in gsum and once on the epilogue line's h_* slices, so host is understated and can go negative on exactly the multi-row passes the line exists for.

#1586 at 3255ca1a has the fix: it adds the exclusion —
if (phprof_on() && acc != &prof_head_ && (acc == prof_dec_ || (g_in_batch && acc == prof_pre_)) && !g_in_head).

So: land #1586, close this one. I have posted the matching correction on #1586 and in my state file; sorry for the reversal on the same pair.