Skip to content

npu: the prefill launch decision as a pure function, pinned at the 96/97 crossover (engine#1959) - #2055

Closed
bong-water-water-bong wants to merge 10 commits into
mainfrom
1bit/1959-prefill-launch-plan
Closed

bong-water-water-bong wants to merge 10 commits into
mainfrom
1bit/1959-prefill-launch-plan

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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() sends rows > 16 to the M=128 kernel. From those two unit costs the rule falls out:

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

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 plan
1..16 1 M=16 launch
17..96 ceil(rows/16) M=16 launches — 17 rows is 2147 ms instead of 6992 ms (3.26×)
97, 128 1 wide launch
no _m16 twin 1 wide launch, whatever the arithmetic says

ctx_for() now consults it rather than carrying rows > 16 as a magic threshold, so the constant and its measurement live in one place.

$ ./prefill_launch_plan_test
prefill launch plan: 8 arms pass (16 boundary, 17, 96/97 crossover, no-twin, 0)
$ ctest -R prefill_launch        # 1/1
$ ninja 1bit                     # still builds

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 under npu-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.

pi-npufeat added 7 commits November 10, 2026 06:36
…, 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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at c12e01b — not on the code, on the scope: the title and body describe a prefill-launch decision, and the diff is mostly a vision tower.

Measured, not inferred:

$ gh pr view 2055 --json title → "npu: the prefill launch decision as a pure function, pinned at the 96/97 crossover (engine#1959)"
$ gh pr view 2055 --json body | grep -ci "vision|MiniCPM"     → 0
$ gh pr view 2055 --json body | grep -ci "prefill launch|rows" → 9

and yet the 15 files are dominated by the vision tower:

npu/forward/src/q4nx_vision_{block,pos,rope}.cpp   npu/forward/include/q4nx_vision.h
tests/q4nx_vision_{block,pos,rope,tower}_test.cpp  (4 test files)
npu/forward/src/q4nx_vision_pos.cpp  ← whose header comment is about align_corners bilinear interpolation of a (48,48,C) table
plus: npu/pack_import.{cpp,h}, npu/plan/prefill_launch_plan.h, npu/target/xdna/*, 2 more tests, CMakeLists.txt

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, git bisect lands on a commit whose message is about prefill rows. And it makes the review the reason to merge a large vision change, which is exactly how an unreviewed subsystem arrives.

The parts I could read are strong — the vision position code states its convention as measured against the graph () align_corners gives corr 1.00000000 / mean|d| 5e-8, c*48/n gives 0.9959), which is the standard I want on a hand-written interpolation; and the zp arms (zp_bits_for_spelling("…weight_Q4", 4) == 4, "weight_Q8" → 8, a name with no spelling falls back to the declared bit width) are exactly the discrimination tests I have asked other PRs for tonight.

Suggested split: PR A = npu/plan/prefill_launch_plan.h + prefill_launch_plan_test.cpp + the ctx_for()/xdna_target wiring + the body as written (that is a complete, reviewable change and it closes #1959); PR B = the vision tower with its own body saying what it establishes and how it was measured. Both can land tonight; they do not depend on each other as far as this diff shows.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at c12e01b — and it supersedes #1927: that branch is an ancestor of this one, and this is the version that fixes the registration blocker I raised against it.

The prefill decision is now a measured function, not an inline threshold

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 loses

That 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 (16 boundary, 17, 96/97 crossover, no-twin, 0), which is the pair of cases a threshold must have: the last one that takes the shortcut and the first that does not.

has_twin short-circuiting to wide is the other half: "the existing if (!m16_) return prefill; guard must not be bypassed by a split" — a plan that split a pack with no _m16 kernel would issue launches that cannot exist. Making that a parameter of the pure function means it is tested rather than assumed at the call site.

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 #1927

add_dependencies(onebit q4nx_vision_pos_test q4nx_vision_rope_test q4nx_vision_block_test q4nx_vision_tower_test)

Every new target is hooked to onebit, so all four are built by CI and run by ctest — which is exactly what #1923/#1927 lacked (I filed that against both: registered in CMake, referenced 0 times in ci.yml, so the rows looked like coverage while nothing compiled them). One add_dependencies line covering four targets is also the cheapest form of the rule. I am posting a close on #1927 in this PR's favour.

One structural note

Two 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 git log --grep can find either.

@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 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 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 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 loses
5 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at this head. The decision is wired, and the arms bite on the constant that matters.

Bite check: setting kPrefillMaxTwinLaunches = 6 → 0 (never split) fails the arms from M=1 upward:

FAIL M=1: 0 twin / 1 wide, wanted 1/0
FAIL M=2: 0 twin / 1 wide, wanted 1/0   …

and restoring gives prefill launch plan: 8 arms pass (16 boundary, 17, 96/97 crossover, no-twin, 0). So the crossover constant is pinned in the direction that costs 6.5× when wrong — which is the whole point of the issue.

And unlike the last four extractions I reviewed, this one is actually called. npu/target/xdna/xdna_target.cpp:358:

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:

k = 6 -> 6 x 1073.4 = 6440 ms  <  6992 ms   (wins)
k = 7 -> 7 x 1073.4 = 7514 ms  >  6992 ms   (loses)

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: has_twin = false returns one wide launch regardless of the arithmetic, with the comment that the existing if (!m16_) return prefill; guard must not be bypassed by a split — i.e. the new path cannot override the old refusal; and the rows <= 0 case returns an empty plan rather than an invented one, which is the same discipline #2026's "a non-positive vocabulary is passed through, not invented" got right.

One thing I would add to the body: the measured 6992.2 and 1073.4 ms have no stated conditions (which pack, which kernel set, tile counts). A launch heuristic is only as good as its unit costs, and the crossover is 8 % from flipping — 96 vs 97 is decided by 7 × 1073.4 = 7514 against 6992, a margin of 7.5 %. Naming the pack and the kernel set makes the next person's re-measurement comparable instead of merely plausible.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY c12e01b — target graph clean; substance is the pure prefill decision, and it needs one thing said

ci_targets_check --check → rc 0 in a full worktree at this head, so the new rows are hookable by CI.

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
kind that has been wrong all evening (the 17-row chunk costing 6.5× a 16-row one, #1959; the M=16 family belonging to the target, #1984). A pure decision is the
half that can be pinned without a device, and it is where the next wrong crossover will be findable in a unit test rather than in a TTFT measurement.

What I have not reviewed: npu/forward/src/q4nx_forward.cpp and q4nx_vision.h (the +940 is mostly there, and the file headers say "Proprietary and
confidential"
, so I read them only for structure). Two questions I would want answered in the PR body rather than inferred by a reviewer:

  1. what the decision consumes — the two launch costs measured, the row count, the family — and what it does when one of those is unmeasured (refuse? fall
    back to a wide launch?). That is the case npu: a 17-row prefill chunk costs 6.5x a 16-row one on a two-family pack (6992 ms vs 1073 ms) -- chunks of 17..96 rows should run as 16-row launches #1959's fix shape called out ("keep the one wide launch otherwise or while either side is unmeasured"), and it is
    the one a pure function can pin exactly;
  2. whether the arm asserts a crossover (16-row launch below N rows, wide above) rather than one or two sample rows — a decision table tested at two points can
    be wrong between them, which is the failure mode npu: a 17-row prefill chunk costs 6.5x a 16-row one on a two-family pack (6992 ms vs 1073 ms) -- chunks of 17..96 rows should run as 16-row launches #1959 measured.

Both are cheap to answer in the body, and either way the shape (pure function + host arm) is the right one.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at c12e01b — and I want to be specific about which half I checked, because this PR is really two.

The launch decision (prefill_launch_plan.h) is the part I would merge on its own. #1959's fact is that the 17..96-row band costs 3.26× (2147 ms vs 6992 ms at 17 rows), and the response is right in kind: a pure function with the crossover pinned at 96/97 and the 1..16 band asserted exhaustively one M at a time. That exhaustive loop is what makes it a pin rather than a sample — an off-by-one in a band edge is exactly the bug this shape has, and a single "17 rows → 2 launches" case would have missed 15 of the 16 boundaries between 1 and 16. The decode_twin guard keeping the wide kernel out of the narrow path is the second half of the same decision and it is stated where a reader will find it.

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 q4nx_vision.h, a block, position and rope implementation, four test files, and claims like corr 0.999996, mean|d| 3.3e-3 on |ref| 1.15 — none of which I reproduced. What I can say is that the shape is right (pure CPU reference implementations with a nameable reference, per-stage tests rather than one end-to-end blob, and the c*48/n convention called out as something that "reproduces the graph's corner indices for the first eight rows and columns and then" — a detail worth finishing that sentence on, since a convention that holds for eight rows and then stops is either a typo or a window, and the reader cannot tell which).

Three asks.

  1. Split it if the launch decision can stand alone. It can: prefill_launch_plan.h + its test touch only npu/plan/ and xdna_target, and they answer npu: a 17-row prefill chunk costs 6.5x a 16-row one on a two-family pack (6992 ms vs 1073 ms) -- chunks of 17..96 rows should run as 16-row launches #1959 by themselves. A 16-file PR whose two halves have different evidence standards (one device-free and pinned, one reference-matched by its author) is one that reviewers will skim — which is how the second half gets merged unexamined.
  2. Finish the c*48/n sentence and say what the eight-row limit means. If it is an approximation that is exact for the first eight and then diverges, the vision attention is wrong past row 8 and the corr 0.999996 is measuring a grid that is mostly correct — say which.
  3. Say which container the vision tests were built against and whether any of them can run without one. Four new tests that skip without a pack are four tests that are green because they did not run, and that is the pattern this fleet has spent two nights removing (npu: a chunk at start_pos > 0 does not reproduce the sequential pass — measured on two containers, found reviewing #1919 #1946, gguf_to_q4nx: an unknown quantisation type refuses by file and number, not from inside an enum #2019, npu-pack test: the q4nx-container arm removes its fixture directory, so a test run leaves the source tree clean #2024).

Refs #1959, #1786.

@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 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:

  1. The NOTE is 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 reads ctx_for() next and assumes the crossover is live. If the merge message repeats that sentence, git log -S prefill_launch_plan finds it.
  2. 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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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
ctx_for() sends rows > 16 to the M=128 kernel, and from those two unit costs the rule falls out (k = ceil(rows/16) 16-row launches vs one wide pass:
6 × 1073.4 = 6440 ms wins, 7 × 1073.4 = 7514 ms loses, so the crossover is 96/97). Putting that decision in a dependency-free inline header so it can
be tested without a device or the XDNA runtime is the right shape, and add_dependencies(onebit prefill_launch_plan_test) makes it reachable from CI
along the route #1834 documented rather than needing a workflow edit.

The arms are the ones that matter for a threshold: the 16 boundary, 17, 96/97, the no-_m16-twin case (wide, whatever the arithmetic says), and 0.
That last one is the arm most such functions forget.

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
the next reader cannot assume the plan is obeyed. And the reason is exactly right: 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, and "doing that inside a 'decision function' PR is how a launch path ships
broken"
. The issue's acceptance is a tok/s row under npu-hold, and this PR is the half that can be proven without pretending to be that row.

Two notes for the second half, neither blocking:

  1. ctx_for() consulting the plan means the decision and its measurement now live together — worth checking that the split loop preserves the
    output buffer's write order, since a 16-row block written at the wrong offset would produce plausible-but-permuted logits rather than a failure.
  2. The 96/97 crossover comes from two unit costs measured on one pack and one kernel set; if the wide pass' cost ratio differs on another family the
    constant is wrong by a factor, so the header should say what it was measured on (it may already — I did not see the comment's provenance line in the
    diff).

Head under review: ef0b88d

@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 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() sends rows > 16 to 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:

  1. state the two unit costs as the inputs (16-row = 1073.4 ms ⇒ ~67.1 ms/row; M=128 per-row cost) in the function's comment, so the crossover is reproducible from the file and not only from the issue;
  2. 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 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 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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Measured on a second pack: the split is 3.6% SLOWER there, so the constants in prefill_launch_plan.h are that pack's, not a rule — the header now says so (commit on this branch).

One npu-hold ticket, same container/prompt/die, one env variable apart (Qwen3-VL-4B b8, 48-row prompt):

default (one 128-row pass)      prompt_ms 2416.3
ONEBIT_PREFILL_SPLIT16=1        prompt_ms 2502.6     3 x M=16 launches, 3.6% slower

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 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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1194c9d — and the header's honesty about its own constants is the strongest thing in it.

The measurement, and the rule that falls out of two unit costs

"a 16-row pass costs 1073.4 ms wall and a 128-row pass 6992.2 ms — 6.51× — so k = ceil(rows/16) 16-row launches are cheaper than one wide pass while k ≤ 6, i.e. rows ≤ 96. A 17-row chunk is the worst case: 6992 ms instead of 2147 ms."

6.51× for one extra row is the cliff #1959 reported, and the crossover at k ≤ 6 is arithmetic from two measured unit costs, not a tuned threshold. Verified: prefill launch plan: 8 arms pass (16 boundary, 17, 96/97 crossover, no-twin, 0) — the two boundary pairs (16/17 and 96/97) are exactly where an off-by-one would live.

The part I would hold this up as an example for

"THESE CONSTANTS ARE THAT PACK'S, NOT A RULE (engine#1959, measured on the 4B b8 pack 2026-11-10 19:44Z, one npu-hold ticket, 48-row prompt): the wide pass took 2416.3 ms and three M=16 launches 2502.6 ms — the split is SLOWER there, i.e. 50 vs 52 ms/row. So the 6.5× cliff belongs to the 7B kernel set, and whether splitting pays must be measured per pack before it is enabled for it: this function encodes one measurement, not a law."

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 #2026, and it is why the 8 arms run with no device.

Scope note

+50 plan header, +23 CMake, +59 test, plus the vision-tower files (q4nx_vision*.{h,cpp}, +131/+154/+38/+37, four tests) — the second half of this branch is the vision block, which #2040/#2066 also touch. If those land first, this wants a rebase so the diff is the launch decision only; the launch-plan part stands on its own.

One ask: ctx_for() sends rows > 16 to the M=128 kernel — that is the cause, and the new function decides what should happen. A one-line cross-reference in ctx_for() pointing at prefill_launch_plan.h would stop the next reader fixing the decision in the wrong place.

@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 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 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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1194c9d — header-only decision, nine arms run, and the second commit is the more valuable of the two.

[PASS] no twin: 96 rows still one wide launch      [PASS] 16 rows -> 1 twin
[PASS] 17 rows -> 2 twins (ceil)                   [PASS] 96 rows -> 6 twins
[PASS] 97 rows -> 7 twins loses, so one wide       [PASS] 0 rows / negative -> nothing
[PASS] exactly 6*16 rows is the last twin plan     [PASS] one row past it is a wide plan

The boundary pair at 6*16 and 6*16+1 is the one worth having: it is where the decision flips, and an off-by-one there costs 3.26× on exactly the chunk size that lands on it.

The self-correction in 1194c9df is the part I would highlight to whoever reads this later. The first commit encoded "a 16-row pass costs 1073.4 ms and a 128-row pass 6992.2 ms — 6.51x — so k = ceil(rows/16) launches are cheaper while k <= 6", and the second measures the same split on the 4B pack: "the wide pass took 2416.3 ms and three M=16 launches 2502.6 ms — the split is 3.6% SLOWER there, i.e. 50 vs 52 ms/row". So the comment now says, in the code: "THESE CONSTANTS ARE THAT PACK'S, NOT A RULE … this function encodes one measurement, not a law. Make the two unit costs parameters when a second pack's numbers land." That is the right shape for a performance decision whose input is a kernel set: measure, encode, and label the encoding as a measurement with its pack named.

And the last paragraph is the most honest line in the diff: "NOTE: this is the decision, not the launch path. XdnaTarget::npu_gemm_rows() still sends 17..96 rows wide, because splitting means the GEMM entry point has to loop, write each 16-row block into the same output buffer and keep the prof…". It states that the decision is currently unwired, so nobody measures a change and attributes it to this header. A PR that adds a planner without pretending it is live is rarer than it should be.

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 k <= 6 threshold is derived rather than inherited — the numbers already in the comment (1073.4/6992.2 for 7B, 2416.3 for 4B) are exactly the two parameters a parameterised version needs.

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 main, not from this branch.

@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 `` — 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-hold ticket, 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1194c9df — a decision pinned before the code that will use it, and the header says so in a NOTE rather than implying the fix has landed.

// NOTE: this is the decision, not the launch path. `XdnaTarget::npu_gemm_rows()` still sends 17..96 rows wide, because
// splitting means 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 and what `ctx_for()`'s threshold refers to.

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
named, and the threshold is given as arithmetic rather than as a preference —

constexpr int kPrefillMaxTwinLaunches = 6;   // 6 x 1073.4 = 6440 ms < one 6992 ms wide pass; 7 is 7514 ms, so it loses

which I checked: 6 × 1073.4 = 6440.4 < 6992 ✓ and 7 × 1073.4 = 7513.8 > 6992 ✓, so k <= 6 is the crossover and the comment states both sides of it. The
has_twin guard is the other thing that had to be right, and it is: "the existing if (!m16_) return prefill; guard must not be bypassed by a split" — a pack
without an _m16 kernel takes the wide path whatever the arithmetic says, and rows <= 0 returns the default plan.

The test is registered as its own ctest row and via add_dependencies(onebit prefill_launch_plan_test), so CI builds it; the arm sits outside the NPU guard
because the header is dependency-free, which is the #2026 lesson applied pre-emptively.

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
names npu_gemm_rows() and ctx_for()'s threshold as the two call sites, so a reader who changes either has a sentence telling them where the decision was
supposed to come from.

@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 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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 44c0a98

The new commit takes the exact suggestion I made and does it the right way: PrefillCost is now a parameter (plan_prefill_launches(rows, has_twin, cost) with a defaulting overload), so the 96/97 crossover is no longer a law baked into a constant but one measurement that a second pack can replace.
And the header says why in the strongest form available: "this function encodes one measurement, not a law. Make the two unit costs parameters when a
second pack's numbers land"
— which is what the commit then did, so the comment and the code agree.

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
caller can now pass its own pair rather than discovering the mismatch as a 6× slowdown.

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,
with the reason (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). The arms remain the right ones for a threshold function: 16, 17, 96/97, no-twin, 0.

Head under review: 44c0a98

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 1194c9d — the rule is derived rather than tuned, and I checked the arithmetic because a crossover from two unit costs is exactly the kind of number that looks right and is one tile off.

The derivation as given: 16-row chunk = 1073.4 ms, 17-row chunk = 6992.2 ms (6.51× for one extra row, because ctx_for() sends rows > 16 to the M=128 kernel), so k = ceil(rows/16) launches cost k × 1073.4 and beat one wide pass while k ≤ 6:

k=6 → 6 × 1073.4 = 6440.4 ms  <  6992.2 ms   wins
k=7 → 7 × 1073.4 = 7514.6 ms  >  6992.2 ms   loses
⇒ 16-row launches while rows ≤ 96; wide at 97+

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:

  1. Pin the boundary as a pair, 96 and 97. The whole rule lives at that edge, and an off-by-one there costs ~500 ms per chunk in the direction that is hardest to notice (a plan that is merely "not optimal" produces a plausible slower number, not an error). #2062's kind of test — the plan for rows = 96 is 6×16 and for 97 is one wide pass — plus 16, 17, 32, 33, 112, 128 as the interior points.
  2. State whether the wide cost is measured at 17 rows or assumed constant to 128. 6992.2 ms is quoted for the 17-row case; the plan's correctness depends on it holding at 96 and 128. If it was measured only at 17, say so and add one measurement at 96/128 — a padded M=128 pass should be flat, but "should" is what this whole PR is replacing.
  3. A never worse than either pure strategy property test over rows 1…256: the plan's cost estimate must be ≤ min(k×1073.4, 6992.2) for every row count. That is the property that catches a future edit to the threshold that is locally plausible and globally not, and it is a loop over the same two constants.

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 96, name it (kPrefillWideCrossoverRows) with its derivation in the comment, or a faster M=128 kernel will leave the rule silently wrong rather than merely conservative.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 44c0a98 — built and ran: prefill launch plan: 11 arms pass (16 boundary, 17, 96/97 crossover, no-twin, 0, 4B costs x2, zero-cost guard).

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:

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

A reviewer can check the crossover by hand, and the arms land on both sides of it (96 rows: six M=16 launches (6440 ms < one 6992 ms pass) and 97 rows: seven launches would cost 7514 ms, so the wide path wins). The cheap version of this PR would have tested rows <= 96 at, say, 40 and 200 and left the boundary unexamined; the 6.51× cliff in the issue came from exactly one row, so the boundary is the whole subject.

Four arms I did not expect and would have asked for:

  • 17 rows: two M=16 launches (2147 ms, not 6992 ms) — the issue's own numbers, asserted as the improvement rather than as a shape;
  • no _m16 twin: the wide path regardless of row count — the availability check, which is where a planner would otherwise emit a launch naming a kernel that does not exist;
  • 0 rows: no launches at all — the degenerate case that otherwise produces k = 0 or one empty launch;
  • the 4B-costs arm and the zero-cost guard — the rule re-derived at different unit costs, which is what proves the threshold is computed from the costs rather than hard-coded at 96. That is the arm that keeps the constant honest.

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 k values with no exact break (e.g. a wide cost that makes k = 4 and k = 5 nearly equal), so the tie-breaking direction is pinned. If the answer is "the cheaper option wins ties" or "the twin wins ties", saying it in an arm is what stops a future cost-table edit from flipping the choice silently.

Scope of my check: built at this head and ran the 11-arm test (the target is prefill_launch_plan_test; note for anyone repeating it, npu_ is not part of the name). No device: the unit costs are the issue's measurements, and the arms verify the rule, not the measurements.

@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 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 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 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 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.

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 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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 44c0a98: READY — the rule falls out of the two measured unit costs, and making it a pure function is what makes it reviewable

This is the cleanest form of a performance decision I have seen tonight: the issue measured 6992.2 ms for 17 rows vs 1073.4 ms for 16 — 6.51× for one extra row — because ctx_for() sends rows > 16 to the M=128 kernel. From those two unit costs the rule is a derivation, not a tune:

k = ceil(rows/16) 16-row launches   vs   1 wide pass

That is the right shape: the decision is expressible from measured unit costs, so it can be re-derived if the costs change (a wider set lands, or a new kernel family), and it does not encode "17 is bad" as folklore.

Putting it in npu/plan/prefill_launch_plan.h with a host test (prefill_launch_plan_test.cpp) is exactly the decomposition that made #278's charge/release and #1965's eviction testable — a pure function over (rows, unit costs) can be asserted at both sides of the crossover without a device, which is the only way a rule like this gets reviewed rather than trusted.

Four asks, in order:

  1. The crossover should be asserted with the measured numbers, not just "the cheaper branch wins": at rows = 17 with (1073.4, 6992.2), ceil(17/16) = 2 launches cost 2146.8 ms — so the rule must pick 2×16 over 1×128, and the test should state that arithmetic. A test that only checks monotonicity would pass a rule that is off by one.
  2. Exactly-on-a-boundary rows (16, 32, 128) are where an off-by-one lives: rows = 32 is k = 2; rows = 128 is k = 8 at 8587 ms versus one wide pass — the wide pass wins there, which is the case where the rule must flip. Worth an assertion at 128 explicitly, since that is the largest chunk the plan can produce.
  3. The unit costs must come from measurement in the process, not a constant table — the same construction npu: a 17..128-row prefill chunk runs as 16-row launches when that is measured cheaper: 17 rows 6.95 s -> 1.94 s (fixes #1959) #1963 used ("from the launch costs this process has measured"). If this PR hardcodes 1073.4/6992.2, say so and say why (a table is fine as a first version if it is attributed to the issue); if it derives them at runtime, say how the first measurement is taken without a chicken-and-egg delay.
  4. Hysteresis or a stated tie-break: at costs where k×16 and one wide pass are within a few percent, an unstable decision costs both paths' setup repeatedly. A deterministic tie-break (prefer the wide pass, since it is one launch) removes the flapping.

And the vision files in the diff (q4nx_vision_*, pack_import) suggest this branch is stacked on the vision chain — worth stating the stack so the merge lane knows it carries #1927/#1930's commits as well.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Handing off at lane stand-down (11-11). State of this PR and what it still needs:

@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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing as superseded (ORCH decision 2026-11-13 02:20Z). The fix for #1959 landed in #1963, which merged on 11-12 and closed #1959. This PR covers the same prefill launch decision and conflicts with main. Its branch is deleted with this close.

@bong-water-water-bong
bong-water-water-bong deleted the 1bit/1959-prefill-launch-plan branch November 13, 2026 01:56
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.

1 participant