Repository navigation
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
Conversation
… (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>
|
ready for review at b0e4e40 |
|
VERDICT: READY at 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
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 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 And a boundary worth stating for my own reasons: this is Not run: I did not run |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
- Say which column of the
[prof] pretable #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. - 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.
|
VERDICT: READY at Why this is the right opening for a fix PRAn 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 ( That is What I would keep in the merged recordBoth 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. |
|
VERDICT: APPROVE at The measurement is the argument10.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 resultThat is the correct condition for parallelising without changing bits — parallelise across independent outputs, never within a reduction. The arm follows from it directly: 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 rowThe 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), 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
left a comment
There was a problem hiding this comment.
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.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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.
|
VERDICT: READY at 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 The methodology is the part worth copyingAn interleaved
which is the same 6 % measured two ways, so the headline is not a rounding of one column. "Bit-identical" is asserted, not claimedand the test goes further than printing: it captures stderr and requires the The documentation correction is the honest half
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; |
VERDICT: READY at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
VERDICT: READY at 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). |
|
ready for review at e0df8ad The head moved from |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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-coldas 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.
Fixes #1416
What #1416 says, and what main measures today
The issue reads a
[prof] pretable as "24 ms of host pack per launch against 1.3 ms of device wait, 93 % of the pass". Re-measured atmain(ee5b436) on the same pack (bonsai2-27b-t2s2), onenpu-holdticket,amdxdnaerror count 2561 -> 2561 (none added):gemm packis 53.6 ms over 416 launches: 0.13 ms per launch, 7 % of a 16-row block.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=1now also recomputes alpha/beta the serial way for every layer and compares bit patterns; a mismatch fails the call.gdn alpha/betaandgdn recurrenceprofile columns stay disjoint (timed on thread 0 at the two barriers).gdn_layer()) is untouched.Measured (device)
Baseline
mainbinary (A) and this branch (B), each a fresh serve, run A B B A in one ticket (457 s,amdxdna2561 -> 2561), same pack, same three distinct prompts. Medians over 103 16-row blocks per arm, from the serve's own[bs]/[bl]lines: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:
Test
tests/gdn_batch_order_testnow captures the engine's check lines and requires onealpha/beta shared vs serial ... identicalline per GDN layer pass.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, whichstep_batch_all()refuses by name, so the test failed before any GDN layer ran. It now drives a noncausal pack throughstep_batch(), which reaches the samegdn_layer_batch(). The test still skips (77) in CI, which has no container; it is a dev-box test, as before.Docs
docs/npu.mdgains "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)
gdn_layer(conv + SiLU + normalise, about 60 ms), the Hadamard rotation (about 44 ms), attention (35-38 ms) and the MLP activation (28-30 ms).🤖 Generated with Claude Code