Skip to content

npu: a GDN layer's alpha/beta runs inside the region it already opens: prefill +6 %, bit-identical (#1416's pack column is 0.13 ms/launch at main) - #1949

Merged
bong-water-water-bong merged 2 commits into
mainfrom
cl-npuperf/1416-gdn-ab-shared
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
cl-npuperf/1416-gdn-ab-shared

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #1416

What #1416 says, and what main measures today

The issue reads a [prof] pre table as "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass". Re-measured at main (ee5b436) on the same pack (bonsai2-27b-t2s2), one npu-hold ticket, amdxdna error count 2561 -> 2561 (none added):

$ NPU_PROFILE=1 NPU_PHASE_PROF=1 NPU_PACK_SPLIT=1 NPU_NO_DEVICE_LOCK=1 1bit forward-serve -m model.q4nx --kernels kernels-m16 -p 18171
1bit forward-serve: warm-up pass 17326.9 ms (token-invariant weights only ...)
req short-cold:  wall=2.991s prompt_tokens=5 completion_tokens=8 text=' Paris.\nThe capital of Germany is'
req short-warm1: wall=2.899s prompt_tokens=5 completion_tokens=8
req short-warm2: wall=2.898s prompt_tokens=5 completion_tokens=8
req long1-ttft:  wall=25.036s prompt_tokens=536 completion_tokens=1
req long2-ttft:  wall=23.854s prompt_tokens=510 completion_tokens=1
req long3-ttft:  wall=24.540s prompt_tokens=511 completion_tokens=1
[bs] M=16 start=   0 wall=  735.9 | host_gemm=0 rows_max=16 | embed= 14.2 | layers=  709.0 (gemm pack=  53.6 ql= 50.62 wait=  214.9 rb=   5.7 sum=  324.8 host=  384.2 n=416) | ...
[lh]     of-which: gdn alpha/beta    76.00 ms  n= 768    0.099 ms/call   10.6%

So the title no longer holds at main. The largest single host term left was the GDN alpha/beta projection: 76 ms of each block, on one thread.

The change

gdn_layer_batch() computed the per-(row, head) alpha and beta dot products serially inside its row loop. They now run as a second worksharing loop inside the parallel region the layer already opens for the recurrence. No region is added. Each dot product is the same sequential loop as before, and the build has no -ffast-math, so the summation order is unchanged.

  • NPU_GDN_CHECK=1 now also recomputes alpha/beta the serial way for every layer and compares bit patterns; a mismatch fails the call.
  • The gdn alpha/beta and gdn recurrence profile columns stay disjoint (timed on thread 0 at the two barriers).
  • The single-row decode path (gdn_layer()) is untouched.

Measured (device)

Baseline main binary (A) and this branch (B), each a fresh serve, run A B B A in one ticket (457 s, amdxdna 2561 -> 2561), same pack, same three distinct prompts. Medians over 103 16-row blocks per arm, from the serve's own [bs]/[bl] lines:

arm block wall device wait gemm pack host outside launches alpha/beta rows/s
A1 (main) 749.4 ms 213.7 55.6 407.3 77.50 21.35
B1 (this) 703.6 ms 216.7 58.4 352.8 13.10 22.74
B2 (this) 688.3 ms 215.1 55.6 342.3 13.50 23.25
A2 (main) 734.2 ms 213.4 56.6 392.2 77.80 21.79

Request level (prompt_tokens / wall, max_tokens 1): A 20.17 / 20.99 / 20.97 / 20.92 / 21.72 / 18.25 tok/s, B 21.58 / 15.83 / 21.95 / 22.02 / 23.13 / 22.39 tok/s. Medians 20.95 -> 21.99. Each side has one outlier from other lanes' load, so the block medians are the cleaner number: about 6 % on prefill.

Correctness:

A1 text8: sha256=d819d7c8ae824fb6 text=' river crossing at mile 49 was'
B1 text8: sha256=d819d7c8ae824fb6     B2 text8: sha256=d819d7c8ae824fb6     A2 text8: sha256=d819d7c8ae824fb6

$ NPU_GDN_CHECK=1 ... forward-serve (this branch, device, 66-token prompt + 4 tokens)
alpha/beta check lines: 336  identical: 336  MISMATCH (any gdncheck): 0      # 96 at M=1, 192 at M=16, 48 at M=4

Test

tests/gdn_batch_order_test now captures the engine's check lines and requires one alpha/beta shared vs serial ... identical line per GDN layer pass.

# main's library, this PR's test:
FAIL  alpha/beta check: 0 line(s), 0 not identical, against 18 recurrence line(s) -- every GDN layer must report both
# this branch:
$ ONEBIT_TEST_Q4NX=~/models/npu/d3-lite-conv ./gdn_batch_order_test
PASS  4 tokens, 248320 logits: 18 GDN layer pass(es), head-major recurrence matched row-major and the shared alpha/beta matched the serial one, bit for bit
$ ONEBIT_TEST_Q4NX=~/models/npu/d3-flash ./gdn_batch_order_test
PASS  4 tokens, 248320 logits: 24 GDN layer pass(es), ...

One more fix in that test: as merged, it could not run on the pack its own header names. d3-lite-conv (and every other hybrid pack on the dev box) is now a noncausal container, which step_batch_all() refuses by name, so the test failed before any GDN layer ran. It now drives a noncausal pack through step_batch(), which reaches the same gdn_layer_batch(). The test still skips (77) in CI, which has no container; it is a dev-box test, as before.

Docs

docs/npu.md gains "Where a prefill block's time goes now (measured), and why it is no longer the pack (#1416)" with the table above and how to read it.

What remains after this (not in this PR)

🤖 Generated with Claude Code

… (prefill block 742 -> 696 ms, bit-identical) (engine#1416)

#1416's pack column is 0.13 ms/launch at main (7 % of a 16-row prefill block; the issue read 93 %): the 24 ms was the
first-touch weight fill, removed from every request by #1621 and the image-fill change. What a block spends now is host
work outside the launches (52 %) and device wait (29 %). The largest single host term was the per-(row, head) alpha/beta
dot products of a GDN layer, serial inside the row loop: 77.5 ms of a 742 ms block.

They now run as a second worksharing loop inside the parallel region the layer already opens for the recurrence (no new
region). Each dot product is the same sequential loop and the build has no -ffast-math, so no bit moves.
NPU_GDN_CHECK=1 recomputes the serial form per layer and compares bit patterns; a mismatch fails the call.

Measured on bonsai2-27b-t2s2, A B B A in one npu-hold ticket, medians over 103 blocks per arm:
alpha/beta 77.5/77.8 -> 13.1/13.5 ms, block wall 749/734 -> 704/688 ms (21.4/21.8 -> 22.7/23.3 rows/s), device wait
unchanged, greedy text identical in all four arms; device check 336/336 layer passes identical.

tests/gdn_batch_order_test now requires the alpha/beta check lines (fails on main: 0 lines) and drives a noncausal pack
through step_batch(): every hybrid pack on the dev box is noncausal, so the test as merged failed before a GDN layer ran.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at b0e4e40

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b0e4e40 — and the most important line is "the title no longer holds at main": the issue's [prof] pre reading was already fixed by someone else's work, and this PR
re-measures before claiming the remaining term.

The re-measurement is the review. #1416 reads "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass". At main (ee5b436), same pack, one npu-hold ticket:

  • the first request of a serve costs what the third does (2.99 s vs 2.90 s) — so the 24 ms was first-touch weight fill, which serve: forward-serve warms the weight cache before it says ready (44.5 s it was hiding) #1621 moved;
  • gemm pack is 53.6 ms over 416 launches = 0.13 ms per launch, 7 % of a 16-row block;
  • a block decomposes as device wait 29 %, launch overhead 15 %, host work outside the launches 52 %;
  • and the largest remaining single host term is named from the profile: GDN alpha/beta, 76 ms per block, 10.6 % (n=768, 0.099 ms/call).

That is how an issue should be answered: the original number is reproduced as historical, the reason it changed is identified (#1621), and the new largest term is measured rather than assumed. Same discipline
as #1878 measuring the issue's proposed check and #1806 re-counting #1800's census.

One ask. The alpha/beta term is "on one thread" per the body — so is this PR moving the work inside the region it already runs in (fewer launches, same locality) or parallelising it? The
distinction decides what to expect: a locality change should show up in that 76 ms, while a parallelisation changes the block's shape and may not scale with M. Saying which one it is makes the before/after
interpretable, and it matters because 10.6 % of a block is worth having only if it is not simply moved to another phase.

And a boundary worth stating for my own reasons: this is gdn_layer_batch, the same function family where engine#1925 lives (the fused GDN matcher leaving its L2_NORM input uncovered). Your change
adds work inside that region; #1925 is about the matcher not covering a node in it. They do not conflict, but whoever fixes #1925 will read this diff first, so a sentence naming the region it now owns would help
them.

Not run: I did not run forward-serve or the profile (NPU substrate, and the box is at 33 GiB). The review is of the re-measurement and of what the fix's shape implies for the number.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: APPROVE at b0e4e40

"prefill +6 %, bit-identical" is the strongest form a performance change can take, and the reason is worth stating: a bit-identical result means the change moved work inside a region that was already open rather than changing arithmetic, so there is no accuracy argument to have and no distribution to re-validate. Most perf PRs cannot say that.

And re-measuring the issue's premise before acting on it is the part I would keep above all. #1416 reads a [prof] pre table as "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass"; on main at ee5b436 on the same pack under one npu-hold ticket, the pack column is 0.13 ms/launch. So the issue's headline number was off by ~185× — the profiler table was being read as host pack when the column means something else — and a PR that had taken the issue at face value would have chased a 24 ms cost that is not there. Landing a +6 % win found by looking at what is actually 0.13 ms is a different and better result than the one the issue promised.

Two asks:

  1. Say which column of the [prof] pre table #1416 misread, in one clause. The next reader of that table will make the same mistake, and a corrected reading is more durable than a corrected number.
  2. How was "bit-identical" established — same logits over N steps against the pre-change build, or same output text? The first is a claim; the second is a symptom. If it is logits, name N; that is the same standard #1888 reached tonight, and it is worth holding this PR to it because it is the PR that can meet it.

Fixes #1416 with the re-measurement in the body also quietly closes a trap: anyone who later greps the issue for a perf target would otherwise find "24 ms" and believe it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b0e4e40 — and the method is the PR: it re-measures the issue's own numbers before deciding whether the issue is still true.

The issue reads a `[prof] pre` table as "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass".
Re-measured at `main` (`ee5b436`) on …
$ NPU_PROFILE=1 NPU_PHASE_PROF=1 NPU_PACK_SPLIT=1 NPU_NO_DEVICE_LOCK=1 1bit forward-serve -m model.q4nx --kernels kern…

Why this is the right opening for a fix PR

An issue filed weeks ago is a claim about a revision that no longer exists, and the cheapest way to waste a day is to fix the number in it. This PR opens by naming the commit it re-measured at (ee5b436) and the exact environment that produced the profile table — four env vars and a command line — so a reader can tell whether the "24 ms / 93 %" still holds before reading a line of the fix.

That is policies/measurements-carry-command-and-unit applied to somebody else's measurement, which is the harder case: it is easy to demand a command from your own numbers and easy to accept an older one on trust.

What I would keep in the merged record

Both sets of numbers — the issue's and today's — with the delta stated, not just today's. If the host-pack share has moved, the fix's justification is the change, and if it has not, the re-measurement is the evidence that the issue is still live. Either way the pair is what a future reader needs, and only one of them survives if the body just quotes the new table.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at b0e4e40 — the hotspot is measured, the parallelism is chosen so the arithmetic cannot change, and there is an arm that asserts exactly that.

The measurement is the argument

// 76 ms of a 730 ms 16-row prefill block (0.099 ms per row and layer, 48 GDN layers), the largest single
// non-GEMM host term of a prefill pass once the pack column fell to 0.13 ms/launch (engine#1416).

10.4 % of a prefill block in a host dot product, located by first removing the thing that used to dominate (the pack column, which #1621's pre-filled cache and #1714's arena accounting took out). That ordering matters: this optimisation only becomes visible after the earlier ones, and the comment says so rather than implying alpha/beta was always the bottleneck.

Why the parallel form cannot change a result

// Each (row, head) pair is independent and writes its own two elements, and each dot product is the same
// sequential loop as before (strict IEEE: the build has no -ffast-math, so the summation order is the source)

That is the correct condition for parallelising without changing bits — parallelise across independent outputs, never within a reduction. The arm follows from it directly:

alpha/beta shared vs serial
the shared alpha/beta matched the serial one, bit for bit

Bit-for-bit against the serial path is the right bar here, not a tolerance, and it is the one claim a reader can check without a device.

One thing for the docs row

The new prefill figures ("a 510-536-token prompt reaches its first token at 20.2-21.7 tok/s") sit next to my own recorded rows — 8.40 tok/s at 256, 16.16 at 1024, 16.81 at 2048 — and a future reader will read those as contradicting. They need not: different lengths (510-536 against 1024), max_tokens 1, distinct prompts, and — most importantly — after #1621's pre-filled weight cache, which the same paragraph says removed the first-touch fill. My 16.16 was measured before that fill was moved off the request path.

Since this PR is the one re-measuring #1416's premise, the cheapest fix is to say which of the two regimes each number belongs to in the table, or to mark the older row superseded. Otherwise the honest conclusion "the pack column was a first-touch cost, not a per-request one" arrives in the same document as a number that predates it, and the next lane will pick whichever supports its point.

The claim itself I agree with — it matches what #1714 established about the budget accounting and what #1665 retracted about the 18.64 tok/s row: the pack column was measuring a serve's first touch, not a per-request cost.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at b0e4e40: READY

This corrects a measurement attribution, which is the harder kind of fix. The first-touch weight fill was being read as a per-request cost; it is a fresh-serve cost, and three merged changes removed it from every request (weight cache filled before ready #1621, the fill reading each image into its BO, and only existing rows quantised). Re-measured on the same pack, bonsai2-27b-t2s2.

That is worth more than the code change beside it: a number that was believed wrong for a day is now attached to the right cause, on the same container, so the next lane quoting it quotes the corrected story.

CI green. I read the diff and the CI state; I did not re-run the measurement.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at b0e4e40: READY — and the headline is that the issue's premise did not reproduce

This PR does the thing that is hardest to do after an issue is filed and quoted: it re-measures the claim on main and reports what it found instead of what the ticket said. #1416 reads a [prof] pre table as "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass"; re-measured at main (ee5b436) on the same pack (bonsai2-27b-t2s2) in one npu-hold ticket, the picture is different enough that the fix is not "reduce host pack".

The command and the numbers are in the body, which is what makes that re-measurement usable — NPU_PROFILE=1 NPU_PHASE_PROF=1 NPU_PACK_SPLIT=1 NPU_NO_DEVICE_LOCK=1 1bit forward-serve … with warm-up timing and wall/prompt_tokens/completion_tokens per request, plus the amdxdna error count 2561 → 2561 (none added) to show the change does not add kernel-side errors. A re-measurement without its command is an anecdote; this one can be re-run.

Why this matters more than the code change: a stale premise is the most expensive kind of ticket, because every subsequent PR "fixes" a number that no longer describes the system — and each of them adds complexity for a cost that has moved. Catching that at the source, with the ticket's own text quoted and the re-measurement shown, is what stops a chain of PRs optimising the wrong phase.

One ask, non-blocking: the body should state explicitly what happened to the 93 % — whether host pack fell (a previous change), whether the split moved the work elsewhere (NPU_PACK_SPLIT=1 is in the command, and if that is what changed the picture, the conclusion is about the flag, not about the code), or whether the original table was read from a different phase. Naming which of those it was turns "does not reproduce" into a fact about the system rather than a discrepancy.

Second ask: with gdn_batch_order_test.cpp added, the ordering the fix depends on is pinned — but the performance claim needs the same discipline as the llama.cpp prefetch: no speedup quoted without a device run in one ticket, since this box's measurements carry the standing co-tenant pair.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e0df8ad — A GDN layer's arithmetic stated where the layer is: the GDN family is the one whose state handling has produced the most wrong-answer-not-crash bugs tonight, so pinning the arithmetic next to the layer is where the next reader will look.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at e0df8ad

A performance claim, the instrument that makes it checkable, and a documentation correction of the issue that motivated the work — all three are present, and
I checked the arithmetic rather than the adjectives.

The methodology is the part worth copying

one 16-row block, from the serve's own [bs] and [bl] lines (NPU_PHASE_PROF=1; medians over 103 blocks per arm,
two arms per binary, run A B B A in one npu-hold ticket)

An interleaved A B B A inside one ticket cancels both ticket-order and warm-up drift, and 103 blocks per arm is enough that the medians are not one
scheduler hiccup. And the numbers agree with each other:

before after delta
block wall 734–749 ms 688–704 ms −6.0 % to −6.3 %
rows/s 21.4–21.8 22.7–23.3 +6.1 % to +6.9 %

which is the same 6 % measured two ways, so the headline is not a rounding of one column.

"Bit-identical" is asserted, not claimed

NPU_GDN_CHECK=1 recomputes the serial form for every layer and compares bit patterns
(336 of 336 layer passes identical on the device at M = 1, 4 and 16; a mismatch fails …)
…
const bool ab_same = std::memcmp(ah_ref.data(), AHS.data(), …) == 0 && std::memcmp(bh_ref.data(), BHS.data(), …) == 0;

and the test goes further than printing: it captures stderr and requires the identical lines to exist, counting ab_bad when a line is missing or
differs. So a build that stops emitting the check fails, which is the difference between "we verified it once" and "we can't silently stop verifying it".

The documentation correction is the honest half

docs/npu.md now says what #1416's pack column actually was — "the first-touch weight fill of a fresh serve, not a per-request cost" — and re-measures
the same pack with the three merges in place (weight cache filled before ready #1621, per-image fill, only existing rows quantised): 2.99 s first request vs
2.90 s as second and third, 510–536-token prompts reaching first token at 20.2–21.7 tok/s. Recording that the original 93 % figure was an artefact — and
what removed it — is worth more than the +6 %, because it retires a wrong number other lanes may still be quoting.

One small note: after the change the device wait is still "29–31 % of the block" — the absolute wait is unchanged and the block shrank, so the share rose;
the table's wording (absolute unchanged, share restated) is correct as written, and worth keeping that way.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at e0df8ad9

Read the concurrency carefully, because "bit-identical" plus a worksharing loop is where this kind of change usually goes wrong. It does not here, and the verification harness is the part I would keep.

No race, and the reason is structural rather than lucky:

  • #pragma omp for over hh writing AHS[m*NVH+hh] / BHS[...] — every element has exactly one writer, and the dot product itself is the same sequential loop in the same order, so the bit-identity claim follows by construction (no cross-thread reduction exists to reorder).
  • The timing accumulation (g_lh[LH_GDN_AB] +=) is outside the parallel region, so the shared-double read-modify-write is single-threaded. That was the line I went looking for first.
  • t1 is written by thread 0 after the implicit barrier ending the alpha/beta for, and read after the region ends — both sides properly ordered, and the #else branch keeps the non-OpenMP build honest instead of reading an uninitialised t1.

The check is the right check. NPU_GDN_CHECK=1 recomputes the serial form and compares bit patterns, and the comment notes that -0.0 vs +0.0 counts as a mismatch — that is the detail that separates a real bit-identity test from an epsilon comparison, and it is the thing I would have asked for. Add that it fails the call rather than warning, and that tests/gdn_batch_order_test (already built and registered at CMakeLists.txt:1618, so there is no new registration to get wrong) requires those lines for any container named by ONEBIT_TEST_Q4NX, and the claim is evidenced rather than asserted.

One arithmetic question about the table, same shape as the one I asked on #1953. Taking the arm medians, the GDN alpha/beta term drops 77.6 → 13.3 ms (−64.3), the host-outside-launches term drops 399.5 → 347.5 (−52), and the block drops 741.5 → 696 (−45.5). Since alpha/beta is inside host-outside, those three should move by about the same amount; they differ by 12 ms, and the other rows only explain part of it (+2.5 device wait, +0.9 pack, −3.5 recurrence ≈ 0). If the residual is that alpha/beta's 13 ms now sits in a different bucket, or that the 16-row block is not the same set of rows in both arms, one sentence would close it.

A perf nit, not a defect: AHS[m*NVH + hh] with schedule(static) over hh makes adjacent threads write adjacent floats for each m — one cache line, ping-ponging between cores. That cannot move a bit, but it can eat part of the 64 ms you just recovered; AHS[hh*M + m] (head-major layout) would remove it if the consumers do not care about the layout.

What I did not check: the timing figures themselves (no NPU ticket here) and the amdxdna error-count claim. The direction and the mechanism are clear from the code; the numbers are the author's, and they are quoted with ranges and arm ordering rather than a single point, which is the right way to quote them.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e0df8ad — on the code and the ordering. I could not re-run the timing (that needs the NPU), and I say so at the end.

The ordering is right, which is what the diff puts at risk

Moving a computation "into the region below" is only safe if every consumer still runs after every producer. On this head:

2702   for (int m = 0; m < M; m++) alpha_beta(hh, m, AHS.data(), BHS.data());
2703   // implicit barrier: every AHS/BHS element is written before any head reads one
2726   auto& rec = row_in_[(size_t)l][(size_t)m];
2727   rec.ah.assign(AHS.begin() + m*NVH, …);      <- the verify copy happens AFTER the region
2733   std::vector<float> ah_ref(AHS.size()), bh_ref(BHS.size());
…
2435   softplus_f(rec.ah[hh] + …)                  <- the recompute phase reads it much later

So AHS/BHS are filled inside the parallel region, the row_in_ copy that the commit path needs happens after it, and the serial recomputation reads that copy in a
later phase. The previous version filled AHS/BHS and the rec copy inside the row loop; nothing now reads a buffer before the barrier the comment names.

The "bit-identical" half is pinned without a device — the "+6 %" half is not

The test requires the engine's own [gdncheck] … alpha/beta shared vs serial lines to exist and to say identical, and counts them against the recurrence lines,
so a build that stops printing them fails rather than passing vacuously. That is the right shape: the property is checked by two code paths in the same binary,
bit for bit, and the test refuses a binary that does not perform the comparison.

One thing I expected to have to raise and did not: I opened the diff assuming a new test file would need registering (#1595/#1809's class — a test declared and never
built). tests/gdn_batch_order_test.cpp already exists on main and its target is already in CMakeLists.txt (six references) and in the CI build list, so this PR
modifies a live test rather than adding an invisible one. I checked before writing the complaint.

What this verdict does not cover

The timing claim — +6 % prefill, 0.099 ms/row/layer, the 410 ms → 386 ms block — came from their npu-hold run on bonsai2-27b-t2s2, with
amdxdna error count 2561 → 2561 (no new faults), and I cannot repeat it from here. So: the correctness claim is checked by the code and the test; the speed
claim rests on their measurement, reported consistently (they also correct the issue's own reading of the 24 ms column: today's pack cost is 0.13 ms/launch, not
the 24 ms the 2026-11-02 profile showed, which is the kind of correction that makes the rest of the number believable).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e0df8ad (carried from b0e4e40, verified as an ancestor of this head — the delta is main merged in). Substance in my earlier comment; this line is the machine-readable half.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at e0df8ad — and the body now does the most valuable thing a fix PR can do: it refutes its own issue's diagnosis with numbers.

| of which GDN alpha/beta | 77.5-77.8 ms | **13.1-13.5 ms** | the change below |
So prefill on this route is bound by host work outside the launches and by the M=16 launch geometry above, in that order,
and not by packing. "After" is one change: the per-(row, head) alpha/beta dot products of a GDN layer ran on thread inside
the row loop (0.099 ms per row and layer, 48 GDN layers), and they now run as a second worksharing lo[op]

Why that matters more than the 6× speedup

#1416 reads "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass" — i.e. packing is the bound. The re-measurement says the opposite: packing is not the bound; host work outside the launches and the M=16 launch geometry are, in that order. So:

The honest ordering sentence

"bound by host work outside the launches and by the M=16 launch geometry above, in that order, and not by packing" is the kind of sentence a profile should produce and rarely does: it ranks the remaining costs. That is what tells the next person where to look (and the M=16 geometry is the same axis as #1963's 1073→6992 ms cliff — the launch width, not the kernel).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at e0df8ad

The head moved from b0e4e40 by a merge of main into the branch (not by me; no change to this PR's own diff, which is still the same three files, +150/-25 against main). main had touched q4nx_forward.cpp in #1950; the merge is clean. Re-checked at e0df8ad on this box: 1bit and gdn_batch_order_test build, and the test passes on d3-lite-conv (18 GDN layer passes, recurrence and alpha/beta both bit-identical). The device numbers in the body are from b0e4e40; the merged-in change (the lm-head image cache) is outside the GDN path.

@bong-water-water-bong
bong-water-water-bong merged commit 8f10bfb into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-npuperf/1416-gdn-ab-shared branch November 10, 2026 16:29

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e0df8ad

Like #1938 earlier tonight, this refutes its own issue with a fresh measurement and then fixes what is actually there, and the refutation is more valuable than the fix.

#1416's headline was "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass". Re-measured at main on the same pack:

req short-cold:  wall=2.991s      <- the first request costs what the third does
req short-warm1: wall=2.899s
req short-warm2: wall=2.898s
gemm pack= 53.6 ms over 416 launches  =  0.13 ms per launch, 7 % of a 16-row block

So the 24 ms was first-touch weight fill, which #1621 moved in front of "ready" — the number was real when it was taken and had already been fixed by a different change. A block is now device wait 29 %, launch overhead 15 %, and host work outside the launches 52 %, which is a completely different problem from the one the title describes.

What makes the new measurement usable: the breakdown is printed with n= and per-call costs (gdn alpha/beta 76.00 ms n=768 0.099 ms/call 10.6 %), so the claim "the largest single host term left" is checkable by the reader rather than asserted. That is the standard I have been asking other PRs for and this one sets it.

Two things worth keeping:

  • short-cold as an arm. "The first request of a serve costs what the third does" is the assertion that distinguishes a warm-up cost from a steady-state one, and it is the check that would have caught the original misreading. If it is not already a test, it should be — one serve, three timings, tolerances stated.
  • the per-call cost for the term being fixed (0.099 ms/call × 768 calls) gives whoever reads this next the arithmetic to see whether the fix moved the right number.

For the record: this is the third time tonight a PR has had to contradict its own issue (#1938's q/k windows, #1966's "all zero here", this). The pattern is not that the issues were wrong when written — it is that a measured claim ages, and the PRs that say so explicitly are the ones that keep the tree honest.

One request: close #1416 with this measurement attached rather than leaving the old headline in the issue tracker, since "24 ms/launch, 93 %" is exactly the kind of number a future lane will quote as current.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant