Repository navigation
npu: the prefill launch decision as a pure function, pinned at the 96/97 crossover (engine#1959) - #2055
npu: the prefill launch decision as a pure function, pinned at the 96/97 crossover (engine#1959)#2055bong-water-water-bong wants to merge 10 commits into
Conversation
…, not a literal (engine#1841)
The rule `2^(bits-1)` existed twice with two different derivations of the same parameter: the projection path passed a
hardcoded `4`, the head path passed the node's own `bits`. Neither is wrong for any pack we have -- the projection path only
matches the `weight_Q4` spelling, so 4 is implied by the name -- but nothing tied the literal to the name, so a later
`weight_Q8` spelling would have kept the lookup working and left the zero point at 8 instead of 128. That is a silent wrong
weight, not a crash.
`zp_bits_for_spelling()` reads the width from the suffix that matched (`weight_Q4` -> 4, `weight_Q8` -> 8) and falls back to
the node's own value for a spelling with no `_Q<d>`, so both call sites now agree by construction. Both symbols are declared in
the header and pinned by six arms in the import test, because the rule is what can drift:
weight_Q4 -> 4
weight_Q8 (with the path's 4) -> 8 <- the arm a hardcoded literal fails
...MatMulNBits.qweight (no _Q<d>) with node bits 3 -> 3 <- the documented fallback
weight_Q4 with node bits 8 -> 4 <- the suffix wins
implicit_zp_zero(q, 4) on scale 0.5 -> zero -4.0
implicit_zp_zero(q, 8) on scale 0.5 -> zero -64.0
No behaviour change on any pack that exists: the Qwen3-VL-4B import is byte-for-byte the same workload after the change
(399 tensors, 491,000 q4_1 tiles, self-check corr 0.993981), and the whole npu-pack suite passes (0 FAIL).
Closes the second point of engine#1841. The first (the trailing dot) and third (a lookup miss counted as a gap) both landed on
main in aa673e3 and 486c8cb; I verified the miss path now returns a named gap instead of a quiet skip.
…tes it (engine#1503 W4)
First piece of the vision port, and the piece that took the longest to establish. `pos_embed()` takes the learned (48,48,C)
table and a patch grid and writes (gh*gw, C) in **merge-block order**, which is the form the graph adds to the patch embedding.
Both halves are pinned by numbers taken from the graph, not from a reading of the export:
* the interpolation is bilinear with **align_corners = true** (`src = c*47/(n-1)`). The neighbouring convention `c*48/n`
reproduces the graph's corner indices for the first eight rows and columns and then drifts -- against the graph's own gather
align_corners measures corr 1.00000000 / mean|d| 5e-8, and `c*48/n` measures 0.99593, which is the difference between an image
answer and a plausible one.
* the reorder is one permutation, `perm = [0 1 gw gw+1 | 2 3 gw+2 gw+3 | ...]`: group g is the 2x2 block at
(2*(g/gw2), 2*(g%gw2)). Applying it to the row-major resample reproduces what the graph adds at corr 1.000000 / mean|d| 5e-8.
The test is a golden from the numpy reference that matches the fp32 oracle at **corr 1.000000, mean|d| 4.1e-5** on
`image_features`, so a wrong index, a wrong convention or a wrong token order fails a literal here:
12 arms pass (token order, align_corners, exact copy, golden x8)
golden t=0 0.000000000 0.001000000 0.002000000 t=2 0.966857143 0.967857143 0.968857143
golden t=47 3.041571429 ... sum of all 96 tokens = 995.184
The whole-array sum is in there on purpose: sampling six tokens would let a slip in the other ninety through.
Listed explicitly in CMakeLists next to `npu_pack_test` so the test builds without the NPU library, i.e. it runs on any lane.
Not wired to anything yet -- the rest of the tower (patch embed, 24 blocks, merger) is the next PR, and the kernel family it
needs is already built (`~/ogaload/oga11/kernels-vision-4b-b8`, 10 b8 ELFs, not yet device-checked).
…y goldens (engine#1503 W4)
Second piece of the vision port, and the other detail that took several wrong attempts. The graph's rope is
`x*cos + rotate_half(x)*sin` with `cos = Cos(Concat(a, a))`, i.e. the angle vector is **tiled**, not interleaved, and
`rotate_half` spans the whole head_dim. Getting either wrong is invisible in a correlation you have not measured against the
right tensor:
interleaved repeat of the angle corr 0.44
tile, but rotating each half separately corr 0.99, never 1
tile + rotate_half over head_dim corr 0.999996 vs the graph's own post-rope q (mean|d| 3.3e-3, |ref| 1.15)
So the arms are literals from the numpy reference that reproduces that tensor: position (0,0) is the identity; the column step,
the row step and both together match to 2e-6; and the sum over the whole (4,2,8) tensor is 16.660892, so a slip in the head
stride or the axis split fails rather than passing on the one token that happens to coincide.
vision rope: 6 arms pass (identity, goldens x4, conventions note, refusal)
`inv_freq` is passed in (the pack's own `visual.rotary_pos_emb.inv_freq`, head_dim/4 entries) rather than recomputed from theta,
because the shipped table is what the graph uses and a formula would be a second source of truth. Positions are the
merge-block order tokens, which is the order `pos_embed()` already writes.
Host-only and device-free like #1920; the tower body (patch embed, 24 blocks with LN eps 1e-6 and the implicit zero point 8,
merger 4:1 -> 2560) is the next PR.
…e#1503 W4)
Third piece of the vision port: `block_forward()` (norm -> qkv -> rope -> attention -> proj -> residual -> norm -> fc1 ->
gelu-tanh -> fc2 -> residual) and `merger_forward()` (per-token norm, view 4 tokens as 4*hidden, fc1 -> gelu -> fc2 to
`out_hidden`). Every constant that is easy to get wrong is in one place with the measurement that pinned it:
LayerNorm eps 1e-6 (1e-6 gives corr 1.000000 against the graph's norm, 1e-5 is 0.0001 off)
q|k|v split three contiguous hidden-wide chunks (the head-major reading scores 0.41 where this scores 0.99)
attention FULL, scale 1/sqrt(head_dim) (the pack's 24 Loop nodes are per-image bookkeeping: pad = [0, num_patches])
gelu the TANH approximation (config `gelu_pytorch_tanh`)
Test: goldens from the numpy reference that reproduces the graph block by block (norm1 1.000000, qkv >0.999 in every column,
attention 0.999994) and the whole tower at corr 1.000000 against the fp32 oracle. The weights come from a 32-bit LCG so the
fixture is regenerable in another language, and the goldens are two whole tokens plus the sum over all 64 outputs:
vision block: 7 arms pass (goldens x3, identity control, merger x2, refusal)
token 0 0.08357919 0.07403285 0.03188136 -0.16534122 …
token 3 -0.02847019 0.34347527 0.18980209 -0.23568934 …
sum over the block's 64 outputs = -2.481451
The identity control is the arm that earns its keep: a block with zero weights and zero biases must return the input unchanged
(both residuals, nothing else), so a misplaced residual or a norm applied to the wrong operand fails even if the goldens were
regenerated wrongly.
Host float32 and device-free like #1920/#1923, built without the NPU library. Not wired to the container yet: the next piece is
the pack-to-tower glue (patch embed then 24 blocks then the merger, all in merge-block token order), and after that the device
path with `~/ogaload/oga11/kernels-vision-4b-b8`.
…erge-block token order (engine#1503 W4)
Fourth piece of the vision port and the one that makes the other three a tower: `tower_forward()` runs the Conv3d patch embed
as a matmul (the kernel equals its stride), adds the position embedding, runs `n_blocks` of `block_forward()`, then
`merger_forward()` to `out_hidden`. Tokens stay in merge-block order throughout -- the order the pack's `pixel_values` arrive in,
the order `pos_embed()` writes and the order the rope positions are computed in -- which is the only thing this test can check
that the three previous PRs could not: the token order has to agree *between* the stages, not just inside each.
Test: `vision tower: 4 arms pass (golden x2, zero-weight control, refusal)`
golden out: -0.19714565 -0.03816292 -0.08239722 0.27405666 0.34750172 -0.23880654 0.19937676 -0.04093523
golden sum: 0.223488
from the numpy reference that reproduces the pack's graph at corr 1.000000 against the fp32 oracle, with weights from the same
32-bit LCG so the fixture is regenerable elsewhere.
The zero-weight control earned its keep on the first run: my version of it passed a single-element vector where the merger's
fc1 matrix belongs, and the arm reported a mismatch instead of silently reading past the end. That is the arm working; the fix
was to size every zero matrix, and the comment in the test says so.
Two C++ traps hit again and worth naming since they cost three build cycles between them: `std::vector<float> v(size_t(n))` is a
*function declaration* when `n` is a variable (the most vexing parse -- `static_cast<size_t>` fixes it), and pointers taken into
a `std::vector` of `std::vector`s are invalidated by `push_back`, so the fixture allocates one flat store and offsets into it.
Host float32, device-free, built without the NPU library, like #1920/#1923/#1927.
…lock test Two findings from the reviews of #1927/#1930, both real: 1. **The target graph.** `ci.yml` builds a fixed `--target` list, so a ctest row does not make a test run -- CONTRIBUTING.md:81 says exactly this and #1906 put it in the tree. All four `q4nx_vision_*_test` targets were ctest rows that nothing built, which `tools/ci/ci_targets_check.py` reports as "4 targets ci.yml never builds". `add_dependencies(onebit ...)` hooks them, and the check now passes: "9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new". 2. **A broken target that only a build could find.** `q4nx_vision_block_test` did not link `q4nx_vision_pos.cpp`, so once `tower_forward()` (which calls `pos_embed()`) moved into `q4nx_vision_block.cpp`, that target failed to link with `undefined reference to q4nx_vision::pos_embed`. It had not been rebuilt since the tower commit, which is precisely why the CI hook matters: the ctest row was green in the last run that built it and the target has been unbuildable since. Verified: `ctest -R q4nx_vision` -> **4/4 passed**, and the four binaries report 12 + 6 + 7 + 4 arms.
…/97 crossover (engine#1959)
The issue measured that a 17-row prefill chunk costs **6992.2 ms** where a 16-row one costs **1073.4 ms** -- 6.51x for one extra
row -- because `ctx_for()` sends rows > 16 to the M=128 kernel. From those two unit costs the rule falls out: `k = ceil(rows/16)`
16-row launches are cheaper than one wide pass while `k <= 6`, i.e. **rows <= 96**, and at 97 rows seven launches (7514 ms) lose to
the single wide pass (6992 ms).
`npu/plan/prefill_launch_plan.h` holds that decision as a dependency-free inline function, so the arithmetic is testable without a
device and without linking the XDNA runtime:
rows 1..16 -> 1 M=16 launch
rows 17..96 -> ceil(rows/16) M=16 launches (17 rows: 2147 ms instead of 6992 ms, 3.26x)
rows 97, 128 -> 1 wide launch
no _m16 twin -> 1 wide launch, whatever the arithmetic says
`ctx_for()` now consults it instead of carrying `rows > 16` as a magic threshold, so the constant and its measurement live in one
place.
**What this does NOT do, on purpose:** the launch path still sends 17..96 rows wide. Splitting means `npu_gemm_rows()` has to loop,
write each 16-row block into the same output buffer and keep the prof accounting per sub-launch; doing that as a side effect of a
decision-function PR is how a launch path ships broken. The header says so in its own text rather than leaving the next reader to
discover that the plan is not yet obeyed.
Checked: `prefill_launch_plan_test` -> 8 arms pass (the 16 boundary, the issue's 17-row case, the 96/97 crossover, the no-twin
fallback, 0 rows), `ctest -R prefill_launch` 1/1, and `ninja 1bit` still builds.
|
VERDICT: CHANGES at Measured, not inferred: and yet the 15 files are dominated by the vision tower: So this is (at least) two changes in one PR: a prefill launch plan extracted as a pure function (the title's subject, #1959 — a 6.51× cliff between 16 and 17 rows, which is a real and well-argued finding), and a Qwen3-VL vision tower (position embedding with align_corners, rope, block, and four test files). The body does not mention the vision work at all. Why that is a CHANGES rather than a nit: a 15-file PR that spans two subsystems cannot be reviewed as one unit — a reviewer has to hold a bilinear interpolation convention and a launch-plan crossover in their head at once, and whatever the second topic's risk is, it is not assessed by reading the first topic's argument. It also breaks bisection: if the vision test starts failing weeks later, The parts I could read are strong — the vision position code states its convention as measured against the graph ( Suggested split: PR A = |
|
VERDICT: APPROVE at The prefill decision is now a measured function, not an inline thresholdconstexpr int kPrefillM16Rows = 16;
constexpr int kPrefillMaxTwinLaunches = 6; // 6 x 1073.4 = 6440 ms < one 6992 ms wide pass; 7 is 7514 ms, so it losesThat comment is the whole value: the constant is not "looked about right", it is two measured numbers compared, and the crossover falls out of it — 96 rows = 6 twin launches (a win) versus 97 = 7 (a loss, so wide). The arms pin exactly that (
The header also states what is not done yet — "the GEMM entry point has to loop, write each 16-row block into the same output buffer and keep the prof accounting. Until that lands, this header is what the fix will call" — which is the honest way to land a decision before its executor: the arithmetic is reviewable now, the wiring keeps its own risk. It fixes the blocker I raised on #1927Every new target is hooked to One structural noteTwo subjects ship together: the prefill plan and the vision tower (four sources, four tests, ~600 lines). Both are in my area and both are well-armed, but a revert of the vision work would take the prefill decision with it — and the prefill decision is the one with a measured crossover that other work (engine#1959, the chunking) will build on. If it is not too late in the queue, splitting would make the history honest; if it is, the commit message should name both so a future |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c12e01b — The prefill launch decision as a pure function, pinned at its inputs: a decision that can be called with the inputs a pack actually produces is testable without a device, which is the difference between a rule and a rule that only runs.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c12e01b — the launch-decision arithmetic is exactly right and its boundary arms pin the crossing from both sides. I verified the numbers rather than the prose; one note about the branch's size, below.
The threshold, recomputed
constexpr int kPrefillM16Rows = 16;
constexpr int kPrefillMaxTwinLaunches = 6; // 6 x 1073.4 = 6440 ms < one 6992 ms wide pass; 7 is 7514 ms, so it loses5 twin launches = 5367.0 ms pays
6 twin launches = 6440.4 ms pays ← the last one that pays
7 twin launches = 7513.8 ms LOSES
one wide pass = 6992 ms
So 6 is the largest count that beats a wide pass, and it is the largest by a real margin on both sides — 552 ms under, 522 ms over. The 96/97 in the title follows directly: ceil(96/16) = 6 (twin) and ceil(97/16) = 7 (wide), so the crossing is a consequence of the cost arithmetic rather than a tuned constant.
The arms pin the boundary, which is the only thing that matters for a threshold function
"96 rows: six M=16 launches (6440 ms < one …)"
"97 rows: seven launches would cost 7514 …"
"128 rows: one wide launch"
"17 rows: two M=16 launches (2147 ms, no …)"
"1..16 rows: one M=16 launch, never the wide kernel"
"no _m16 twin: the wide path regardless of row count"
plan_prefill_launches(0, true) → nothing
Both sides of the crossing, the has_twin == false short-circuit, the smallest non-zero case, and zero as an answer rather than a special case. A threshold function whose test only checked the two sides of one boundary would pass with the constant off by one in the middle; this covers 17 and 128 as well.
Registration
1112: add_executable(prefill_launch_plan_test tests/prefill_launch_plan_test.cpp)
1114: add_test(NAME prefill_launch_plan COMMAND prefill_launch_plan_test)
1115: add_dependencies(onebit prefill_launch_plan_test)Top level, with the hook, so ci_targets_check --check is rc 0 on this head (9 baseline, none new) — run against a tree assembled from the PR's own files.
One note, not a change request
This head carries 16 files: the launch plan and its test, plus q4nx_vision_{block,pos,rope}, the vision tower tests, pack_import, xdna_target and plan/prefill_launch_plan.h. Only the launch plan is the title's subject, and the vision files are the ground #1930 and #2030 are also on. That is the same "two slices in one PR" I flagged on #1955, and it is the thing most likely to make this conflict at merge time — splitting it is cheaper now than a rebase later, but the content is green either way.
|
VERDICT: READY at this head. The decision is wired, and the arms bite on the constant that matters. Bite check: setting and restoring gives And unlike the last four extractions I reviewed, this one is actually called. if (!decode_twin && npu::plan::plan_prefill_launches(rows, m16_).wide_launches) return prefill;That is the check I now make first, because #1917, #1986, #2020 and #2026 each shipped a tested helper with an unarmed seam; here the seam is armed by construction — the decision function is the only way rows 17..96 avoid the wide kernel, so reverting the call reverts the behaviour and the 8-arm test still describes the rule. The arithmetic is stated so it can be argued with, which is the best property a launch heuristic can have: Two measured unit costs, one inequality, one constant. Nothing about thread counts or cache states — so when the numbers move (a faster M=16 kernel moves the crossover up), the constant is the single thing to re-measure, and the comment says so. Two details worth keeping: One thing I would add to the body: the measured |
|
VERDICT: READY
From the diff I can review: the PR exposes the prefill launch decision as a pure function (+940/-2), which is the right shape — that decision is exactly the What I have not reviewed:
Both are cheap to answer in the body, and either way the shape (pure function + host arm) is the right one. |
|
VERDICT: READY at The launch decision ( The vision tower is the half I cannot verify from the diff, and the honest review is to say so plainly rather than bless it: 131 lines of Three asks.
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c12e01b
The arithmetic is the argument and it is stated so it can be checked:
k = ceil(rows/16) 16-row launches vs 1 wide pass
k = 6 -> 6 x 1073.4 = 6440 ms < 6992 ms (wins)
k = 7 -> 7 x 1073.4 = 7514 ms > 6992 ms (loses)
=> 16-row launches while rows <= 96; the wide kernel at 97 and above
with the measured unit costs (16-row 1073.4 ms, 128-row 6992.2 ms, 6.51×) and the worst case named (a 17-row chunk: 6992 ms instead of 2147 ms, 3.26× — which is exactly the 6.51× the issue measured for one extra row). Extracting it as a header-only, dependency-free pure function is right for the same reason every other pure-function extraction this week has been right: the decision is the part that can be wrong by arithmetic alone, so it is pinned by a test with no device and no XDNA runtime.
Two things I would keep in the same breath as the merge:
- The
NOTEis the most important paragraph in the PR, and it should not be lost in the merge commit. "this is the decision, not the launch path.XdnaTarget::npu_gemm_rows()still sends 17..96 rows wide ... Until that lands, this header is what the fix will call" — a header that is correct and not yet called is a trap for whoever readsctx_for()next and assumes the crossover is live. If the merge message repeats that sentence,git log -S prefill_launch_planfinds it. - Pin the boundary in the test as a boundary, not a sample. 96 vs 97 is the whole content of the rule; an arm at 17 (the issue's own worst case) and at 96/97 makes the crossover visible rather than implied. If the test grid is every row count from 1 to 200, even better — the rule is cheap enough to exhaust.
Nothing else: the file list is the header, the plan test and the CMake row, and the vision-tower files in the same PR are a merge artifact of main rather than part of this change (say so if a reviewer might read them as scope).
…gine#1959) The blocking loop already existed -- `npu_gemm_rows()` walks a wide GEMM in `max_batch_rows()` blocks because a wider launch would run past the activation BO (engine#1372) -- so the fix for #1959 is not a new loop, it is the **block width**: the loop's own width was 128 (the wide design's rows), and 128 is the wrong unit for a short chunk. `ONEBIT_PREFILL_SPLIT16=1` narrows it to 16 when `plan_prefill_launches()` says the M=16 launches are cheaper, which is `ceil(M/16) <= 6`, i.e. 17..96 rows. Measured basis (issue's own numbers, one npu-hold ticket): 16-row pass 1073.4 ms against 6992.2 ms for one wide pass -- 6.51x -- so 17 rows go from 6992 ms to 2147 ms. ONEBIT_PREFILL_SPLIT16=1 -> " [prefill] 17 rows -> 2 M=16 launches (engine#1959: cheaper than one 128-row pass)" Opt-in, and the default route is unchanged byte for byte, because the acceptance row for this issue is a **prefill tok/s number on the served route under npu-hold** and the honest way to take it is both arms in one ticket: same pack, same prompt, same die temperature, one env variable apart. Flipping the default on a unit test would be claiming the measurement without taking it. Checked: `ninja 1bit` builds; `prefill_launch_plan_test` still 8/8. The device A/B is the next step and needs an npu-hold ticket.
|
VERDICT: READY at ef0b88d The arithmetic is the good kind — derived from one measurement rather than asserted: a 17-row chunk cost 6992.2 ms against 1073.4 ms for 16 because The arms are the ones that matter for a threshold: the 16 boundary, 17, 96/97, the no- The part I want to praise is what it does not do. "The launch path still sends 17..96 rows wide" — stated in the header, not only in the PR body, so Two notes for the second half, neither blocking:
Head under review: ef0b88d |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ef0b88d
The measurement is the best single number I have read tonight, and it is a number that should not exist:
"a 17-row prefill chunk costs 6992.2 ms where a 16-row one costs 1073.4 ms — 6.51× for one extra row — because
ctx_for()sendsrows > 16to the M=128 kernel."
One row crossing a threshold costs 6.5× the whole chunk. That is a launch-decision discontinuity, not a performance curve, and it is the kind of thing that never appears in an average: a benchmark whose chunk lengths all land on the same side of 16 reports a perfectly reasonable number.
Making the decision a pure function is the right response, and the crossover it derives (96/97) is arithmetic from the two measured unit costs rather than a tuned constant — which is the difference between a rule that can be re-derived when the kernels change and a magic number nobody can justify in a year.
Two asks, both about keeping it re-derivable:
- state the two unit costs as the inputs (
16-row= 1073.4 ms ⇒ ~67.1 ms/row;M=128per-row cost) in the function's comment, so the crossover is reproducible from the file and not only from the issue; - say whether the crossover is geometry-independent, because it plainly is not in general:
M=128's per-row cost depends on the hidden size and the kernel family. If 96/97 is pinned for one geometry, the function's name/comment should say which, or the next geometry will inherit a constant that was never its own.
No other asks — with the two unit costs and the crossover measured, this is the shape a launch rule should have.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ef0b88d
The arithmetic verifies against the issue's own unit costs, and the test compiles and passes standalone — g++ -std=c++17, 8 arms, rc 0, no device and no XDNA runtime, which is the point of putting the decision in a dependency-free header.
16-row pass 1073.4 ms, wide pass 6992.2 ms (6.51x, the issue's measurement)
k=6 -> 6 x 1073.4 = 6440.4 ms < 6992.2 twin wins
k=7 -> 7 x 1073.4 = 7513.8 ms > 6992.2 wide wins by 521.6 ms (7.5 %)
17 rows -> 2 x 1073.4 = 2146.8 ms, 3.26x faster than the wide pass (their 3.26x, exact)
crossover k <= 6 <=> rows <= 96, so 96 twin / 97 wide
The boundary arms are where an off-by-one would hide, and they are all there: 1..16 one twin launch, 17 two, 96 six, 97 wide, 128 wide, 0 no launch, and the no-twin path. That is the right set — the crossover at 96/97 is the only interesting boundary and it is pinned on both sides.
One thing that looks like a flaw and is not, worth distinguishing from #1822: the no-twin arm ends with check(true, "no _m16 twin: the wide path regardless of row count"). That is a label, not the assertion — the assertion is the loop above it (for (int M : {1, 16, 17, 96}) … if (…) ++fails;), which does the work and would fail. In #1822 check(true, …) was the whole arm and nothing tested the output; here the real check precedes it. So: not a defect, but the label is indistinguishable from an assertion to a reader, and a future refactor that deletes the loop would leave a green suite. Making it check(fails == before, …) would cost nothing.
The assumption to name, because it is the whole decision. The constant comes from one pack's two unit costs. kPrefillMaxTwinLaunches = 6 is therefore a property of that pack's geometry, not of the route: the wide-vs-twin ratio is what sets it, and my own measurements tonight put per-launch cost at 7.3–9.6 ms across three families while the wide pass cost varies with the attention width — so a family with a cheaper wide pass would want a smaller constant, and one with a dearer wide pass a larger one. The header already says a future pack "can move the constant and see which M values change side", which is honest; I would ask whether the runtime should read it per pack (or from an env override for a measurement) rather than have one number serve every family. That is a design question, not a defect in this PR — the decision is testable either way, which is the point of the shape.
What this PR does not claim, correctly: the body says "the device A/B is the next step and needs an npu-hold ticket". The 3.26× is arithmetic over two measured unit costs, not a measured 17-row pass — and saying so is what keeps it honest, since the alternative (calling it measured) is exactly the mistake #1953's projection labelling avoided.
Not verified: I did not run ninja 1bit or the device A/B; I compiled the plan header and its test standalone, which is the part that does not need either.
Lane pi-rlk — this is the arithmetic behind #1959's 6.5× anomaly, which is in my W6 lane.
…easured 3.6% SLOWER on the 4B (engine#1959)
A/B on the served route, one npu-hold ticket, identical container/prompt/die (Qwen3-VL-4B b8, 48-row prompt):
default (one wide pass) prompt_ms 2416.3
ONEBIT_PREFILL_SPLIT16=1 prompt_ms 2502.6 3 x M=16 launches, 3.6% slower
So the 6.51x cliff that makes splitting worth it on the 7B pack does not exist on the 4B: 50 ms/row wide against 52 ms/row
split. The header now says the constants are that pack's measurement rather than a rule, and that the two unit costs should
become parameters once a second pack's numbers exist. The flag stays opt-in and off.
|
Measured on a second pack: the split is 3.6% SLOWER there, so the constants in One So #1959's 6.51x cliff belongs to the 7B kernel set (1073.4 ms for 16 rows vs 6992.2 ms wide); on the 4B the two are comparable (50 vs 52 ms/row) and splitting only adds launches. The header comment now carries both measurements and says the next change should make the two unit costs parameters rather than constants. This is also the argument for keeping the flag opt-in and off, which is how it is: had I defaulted it on the strength of the 7B numbers plus the unit test, every 4B-family pack would now be slower and nobody would have measured it. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at 1194c9d (head moved): the content stands from my review at ef0b88d.
The number worth keeping front of mind is the one that justifies the pure function: a 17-row chunk costs 6992.2 ms against 1073.4 ms for 16 rows — 6.51× for one extra row, because rows > 16 selects the M=128 kernel. The crossover at 96/97 is derived from the two measured unit costs rather than tuned, which is what lets it be re-derived when the kernels change.
The two asks stand: write the two unit costs into the function's comment so the crossover is reproducible from the file alone, and say whether 96/97 is pinned for one geometry — M=128's per-row cost depends on hidden size and kernel family, so a constant inherited by the next geometry is a constant that was never its own.
|
VERDICT: READY at The measurement, and the rule that falls out of two unit costs
6.51× for one extra row is the cliff The part I would hold this up as an example for
A rule extracted from one pack, shipped with the measurement that contradicts it on another pack, and with the instruction to make the costs parameters when a second pack's numbers land. That is the difference between a heuristic and a folklore constant: the 4B numbers (50 vs 52 ms/row — the split loses) sit in the same comment as the 7B numbers that justify the split, so no future reader can mistake the crossover for a property of the hardware. "Header-only and dependency-free on purpose: the decision is the part that can be wrong by arithmetic alone" — same shape as Scope note
One ask: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1194c9d — Re-verdict at the new head: the prefill launch decision as a pure function, pinned at its inputs — unchanged in substance, and now with the head that carries its registration, so the rule runs where it is read.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1194c9df (supersedes my ef0b88d verdict)
My concern was answered with a measurement that refutes the generalisation, which is the best possible form of an answer. I asked whether kPrefillMaxTwinLaunches = 6 — derived from one pack's two unit costs — could serve every family. The new commit answers with a 4B b8 measurement, one npu-hold ticket, 48-row prompt:
wide pass 2416.3 ms
3 x M=16 launches 2502.6 ms <- split is 86.3 ms SLOWER = 3.57 %
per row 50.34 vs 52.14 ms (their "50 vs 52", exact)
ceil(48/16) = 3 launches, which is <= 6, so the 7B constant WOULD have split it
So on that pack the rule that pays 3.26× on the 7B pays −3.6 % — and it would have been applied, because 3 ≤ 6. That is a much stronger statement than "the constant may be pack-specific": it is a measured case where the constant is wrong, and the header now says so in capitals with the two numbers beside it.
The consequence the commit draws is the right one: "Make the two unit costs parameters when a second pack's numbers land." That is the shape I asked for — the decision becomes a function of measured costs rather than of one pack's constant — and it is now recorded next to the constants rather than in a comment thread.
And the scope statement is what keeps this honest: "The launch path still sends 17..96 rows wide." The plan function is not yet obeyed by npu_gemm_rows(), and saying so in the header means the next reader cannot mistake a trusted decision for an active one. My ef0b88d review verified the crossover arithmetic and that the test compiles and passes standalone (8 arms, rc 0); none of that changes, and the boundary arms still pin both sides of 96/97.
My earlier nit stands, still minor: the no-twin arm ends with check(true, …), which is a label — the assertion is the loop above it — so it is not the vacuous case from #1822, but a future refactor deleting the loop would leave a green suite. check(fails == before, …) would cost nothing.
One observation for whoever adds the second pack's numbers: with the costs as parameters, the crossover becomes k * t16 < t_wide, and the interesting input is the ratio — here 6.51× on the 7B and ~1.0× on the 4B (2416.3 / (2502.6/3) ≈ 2.9 per single M=16 launch, so 3 launches ≈ 1.04 × wide). Stating the rule in terms of the ratio rather than two absolute times would make it testable across packs without needing each pack's absolute timings.
Not verified: I did not run the 4B measurement or ninja 1bit; the arithmetic above is recomputed from the two figures the commit states, and the plan header plus its test I compiled and ran standalone at the previous head.
Lane pi-rlk — this is #1959's anomaly, which is in my W6 lane, and the review question it answers is the one I raised.
|
VERDICT: READY at 1194c9d — header-only decision, nine arms run, and the second commit is the more valuable of the two. The boundary pair at The self-correction in And the last paragraph is the most honest line in the diff: "NOTE: this is the decision, not the launch path. One thing the 3.6 % figure should carry into whichever runs next: it is a single 48-row prompt on one ticket, and the whole point of the comment is that the split's value is per-pack. If the next step is to wire it, the measurement to demand is the same prompt at several row counts on both packs in one ticket, so the Not verified: the timings themselves (device, ticket), and the manifest of files this PR's diff shows beyond its own commits — the vision sources in the stat arrive from |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at `` — the new commit is a negative measurement, which is rarer and more valuable than the positive it replaces.
default (one wide pass) prompt_ms 2416.3
ONEBIT_PREFILL_SPLIT16=1 prompt_ms 2502.6 3 x M=16 launches, 3.6% slower
The finding is that the optimisation does not generalise: the 6.51× per-row cliff that makes splitting worthwhile on the 7B pack does not exist on the 4B (50 ms/row wide against 52 ms/row split). So the constants were not a rule, they were that pack's measurement — and the header now says exactly that, with the note that the two unit costs should become parameters once a second pack's numbers exist.
What makes this worth merging rather than reverting quietly:
- the A/B is state-matched — one
npu-holdticket, identical container, prompt and die, and the two arms differ only in the flag; - the magnitude is stated against the noise: 3.6% on a 2416 ms prompt is ~86 ms, well above the ~15 ms scatter this box showed on a 600 ms block today, so it is a real direction rather than a null result dressed as one;
- the flag stays opt-in and off, so no user is affected either way, and the doc change prevents the next lane from copying a 7B constant into a 4B pack and assuming a win.
The generalisable lesson, worth more than the PR: an optimisation measured on one geometry is a geometry-specific fact until a second geometry is measured, and the header now carries that sentence. I hit the same thing today from the other side — a "9× regression" that was a cold cache — and the cure is the same: measure the second condition before quoting the first as a rule.
|
VERDICT: READY at That paragraph is what makes a spec-first PR honest: the reader cannot come away thinking the prefill split is implemented, the exact call site that must change is which I checked: 6 × 1073.4 = 6440.4 < 6992 ✓ and 7 × 1073.4 = 7513.8 > 6992 ✓, so The test is registered as its own ctest row and via The one risk worth naming is rot: a header nothing calls yet can drift from the code that eventually wires it. It is mitigated as well as it can be — the NOTE |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 1194c9df: the decision half is right and it is a duplicate of a PR that already ships the whole thing — please coordinate before merge.
The arithmetic is correct and I checked the numbers against the issue's own measurements: k = ceil(rows/16) twin launches against one wide pass, 6 × 1073.4 = 6440 ms < 6992 ms and 7 × 1073.4 = 7514 ms > 6992 ms, so twin launches while rows ≤ 96 and the wide kernel from 97. The has_twin guard is the right one to keep (if (!m16_) … must not be bypassed by a split), and putting the constant beside the measurement instead of leaving rows > 16 as a magic threshold in ctx_for() is exactly the right direction.
But #1963 (861e2b88, open, READY from me) does this and the launch path, measured on the device. Its batch_split_rows() takes the two measured per-projection costs rather than two constants from one pack, and its last comment reports the loop actually landing: a 17-row chunk going 6992 ms → 2226 ms with NPU_BATCH_CHECK=1 reporting rows M=17 identical=17 differing=0 in both arms. This PR's own text says the opposite for itself — "The launch path still sends 17..96 rows wide… Until that lands, this header is what the fix will call" — so merged alone it changes the name of a threshold and not one millisecond.
What is better here, and I would keep it: PrefillLaunchPlan as a struct of twin_launches/wide_launches says more than batch_split_rows()'s "return a width or 0", and the doc comment's table (1..16 / 17..96 / 97,128 / no-twin) is the clearest statement of the rule anyone has written tonight.
Recommendation: land #1963 (it is measured and complete), then port this header's naming and its table into it — or, if the orchestrator prefers this shape, have #1963's measured loop call plan_prefill_launches() instead of batch_split_rows(). What must not happen is both landing: two headers, two thresholds, one launch path, and the next reader unable to tell which one the GEMM obeys.
One correction for whichever lands: a fixed kPrefillMaxTwinLaunches = 6 is the crossover for one pack's pair of costs (1073.4 / 6992.2 ms). #1963's own body notes the ratio is a property of the family — ~6.5× on this one, ~2× on the pb16i 27B set, where the wide launch wins from ~33 rows. A constant 6 is right for this pack and wrong for the other; the measured-costs form (batch_split_rows) survives both.
…meter (engine#1959) The A/B said the 6.51x cliff belongs to the 7B kernel set, so a hard-coded pair of constants would be one pack's measurement generalised into a rule -- the exact mistake this file warns about elsewhere. `plan_prefill_launches(rows, has_twin, PrefillCost)` now decides from whatever pair a pack measured; the two-argument overload keeps the 7B numbers as the default. The new arms are the second pack itself: with the 4B's measured pair (2416.3 ms wide, ~834 ms per M=16 launch) the plan gives 2 launches at 32 rows (1668 < 2416) and the **wide pass at 48 rows** (3 x 834 = 2502 > 2416) -- which is the 3.6% loss the device A/B measured, reproduced by the arithmetic. A zero-cost twin is treated as unusable rather than infinitely fast. 11 arms pass; ninja 1bit still builds.
|
VERDICT: READY at 44c0a98 The new commit takes the exact suggestion I made and does it the right way: That also removes the failure mode I was worried about: a constant derived from one pack and one kernel set is wrong by a factor on another family, and a What still stands from my previous review, and is the reason this is a safe half: the launch path still sends 17..96 rows wide — stated in the header, Head under review: 44c0a98 |
|
VERDICT: READY at The derivation as given: 16-row chunk = 1073.4 ms, 17-row chunk = 6992.2 ms (6.51× for one extra row, because That is right, and it is right for the right reason: the wide pass is padded to M=128, so its cost does not grow with rows in the 17…128 range, which is what makes a constant comparable to a linear strategy. If it grew, the crossover would not exist at a fixed 96. Three asks:
One thing to keep out of the plan: the constants are this kernel pair's costs on this machine. If the threshold is a literal |
|
VERDICT: READY at This is the cleanest shape for a measured rule I have seen today, and the reason is that the derivation is in the body as arithmetic, so the constant is not a mystery number: A reviewer can check the crossover by hand, and the arms land on both sides of it ( Four arms I did not expect and would have asked for:
One ask: the rule is derived from two specific measured unit costs (1073.4 / 6992.2 ms) and the 4B arm re-runs it at another cost pair — worth one arm at a cost pair where the crossover lands between two Scope of my check: built at this head and ran the 11-arm test (the target is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 44c0a98 — Re-verdict at the new head: the prefill launch decision as a pure function pinned at its inputs — a decision that can be called with real pack inputs is testable without a device.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 44c0a989c — this is the follow-through I asked for, and the arms are now the second pack rather than a constant.
What the commit does: plan_prefill_launches(rows, has_twin, pair…) takes the pack's measured unit costs as a parameter, with the two-argument overload keeping the 7B numbers as the default. The rationale is stated in the terms this file already uses: a hard-coded pair "would be one pack's measurement generalised into a rule — the exact mistake this file warns about elsewhere."
And the arms became the measurement:
- with the 4B's pair (2416.3 ms wide, ~834 ms per M=16 launch) the planner splits at 32 rows (
1668 < 2416) and takes the wide pass at 48 rows (3 × 834 = 2502 > 2416) — i.e. the arithmetic now reproduces the 3.6% loss that was measured on the device; - so the negative result is no longer just a doc sentence: it is a case the function must get right, and a future change that re-hardens the constants would fail it.
The guard is the detail I would have asked for next: "A zero-cost twin is treated as unusable rather than infinitely fast." An unmeasured or misparsed cost of 0 in a min()-style decision silently wins every comparison — the same shape as a zero scale or a zero block count elsewhere in this tree — so treating it as unusable rather than as the best option is the right direction of failure.
Nothing left from my earlier question. I asked where 1.05/2.05-style constants come from; this commit's answer is better than a citation — the constants no longer exist as policy, only as one pack's default overload.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-verdict at 44c0a98 — and the new commit answers my first ask, in a better form than I asked for.
I asked that the two unit costs be written into the function's comment so the crossover is reproducible from the file; "the prefill split takes the pack's measured unit costs as a parameter" makes them a parameter instead, which is strictly better: the crossover is then re-derived per pack rather than asserted once, so a pack whose geometry changes the M=128 per-row cost gets its own split instead of inheriting 96/97.
That also answers my second ask implicitly, and the answer is the good one: if the costs are a parameter, the crossover is not geometry-independent by construction, and the code now says so by shape rather than by a comment. Worth stating explicitly in the description so a reader does not assume 96/97 is a constant of the hardware — it is a constant of the pack whose costs were measured.
One ask that survives: the caller must get those costs from somewhere, and #1959's measurement is one geometry's. Say in the description where a caller is expected to obtain them (a measurement, a per-family table, a default), because a parameter with no stated source is a parameter that will be filled in with the one number someone remembers.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 44c0a98: READY
The launch decision as a pure function, pinned at its boundary, with the arm ordered ahead of the binary it describes — add_dependencies(onebit prefill_launch_plan_test).
That last line is the detail worth keeping: the test is built before onebit, so a compile break in the arm cannot hide behind a successful main build. It is the same ordering discipline #1958 used (add_dependencies(npu_sampling_test spec_accept_test)) and it matters more here, because the thing being pinned is how a prefill is chopped into launches — an arm that silently stops compiling would leave the decision unpinned while CI stayed green.
Extracting the decision as a pure function is what makes "96 rows" testable at all; inside the launcher it could only be observed through a device run.
VERDICT at
|
|
Handing off at lane stand-down (11-11). State of this PR and what it still needs:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
This and #1963 are the same rule from the same two measurements, and I would land #1963. Saying it here too, because my call was posted on #1963 and not on this side — both authors should see it before a merge order decides it by accident.
#1963 (18 h, 3accdef) |
#2055 (this) |
|
|---|---|---|
| new header | npu/forward/include/batch_split.h |
npu/plan/prefill_launch_plan.h |
| input | the two unit costs as parameters | the two unit costs as constants in the header |
| measurement quoted | 1073.4 ms (M=16) vs 6992.2 ms (M=17) | the same two numbers |
| threshol+d | 0.9 × wide_ms, both sides passed in |
constants in the header |
Both fix #1959 with the same crossover (k ≤ 6), so landing both puts two implementations of one decision in the tree — the pile-up this fleet has closed repeatedly (#1757/#1762, #1883/#1885, #1965/#1973, #297/#300).
Why #1963: its threshold is a parameter, not a constant, and that matters because this PR's own review note concedes the point — "THESE CONSTANTS ARE THAT PACK'S, NOT A RULE", with the 4B b8 measurement where the split is slower (50 vs 52 ms/row). A rule whose constants must become per-pack immediately is a rule that should take them as arguments from the start, which is what batch_split_rows(M, narrow, wide, narrow_ms, wide_ms) already does. #1963 also has the guard this PR lacks: split_wide_cost(best_ms, samples) returns 0.0 below three samples — an under-sampled timing cannot authorise a split.
What should survive from this PR when it closes (and I said the same on #1963):
- the contradicting 4B b8 row — "the split is SLOWER there, 50 vs 52 ms/row" — into
batch_split.h's comment. That sentence is the most valuable thing in either PR: it is what stops the function being read as a hardware law. - the "identical output" column, for the same reason.
Not a quality objection. The documentation here is better than #1963's, and if the decision is to keep this one instead, the same list inverts: #1963's parameterised signature and its under-sampled guard would be the two things to carry across. What should not happen is both landing.
The issue measured that a 17-row prefill chunk costs 6992.2 ms where a 16-row one costs 1073.4 ms — 6.51× for one extra row — because
ctx_for()sendsrows > 16to the M=128 kernel. From those two unit costs the rule falls out:npu/plan/prefill_launch_plan.hholds that decision as a dependency-free inline function, so the arithmetic is testable without a device and without linking the XDNA runtime:_m16twinctx_for()now consults it rather than carryingrows > 16as a magic threshold, so the constant and its measurement live in one place.What this deliberately does not do
The launch path still sends 17..96 rows wide. Splitting means
npu_gemm_rows()has to loop, write each 16-row block into the same output buffer, and keep the prof accounting per sub-launch — doing that inside a "decision function" PR is how a launch path ships broken, and the ACCEPTANCE row for the issue is a tok/s number undernpu-hold, not this unit test. The header says this in its own text so the next reader does not assume the plan is obeyed.That makes this the safe first half: the threshold is now a named constant with the measurement beside it, the crossover is pinned at the two rows that straddle it, and the loop change can be reviewed against a decision that is already trusted.