[bong-water-water-bong]

Refs #1781. This is the piece a forward-serve speculative loop needs first.

The two decisions of a greedy speculative round now live in one pure header with their own tests, and the loop tool from #2240 uses it instead of inline code.

What is added

npu/forward/include/spec_greedy.h:

• greedy_round(drafts, k, choices, rows) keeps the longest prefix of the drafts that the verify pass's rows chose. Its next is the target's own token after the kept ones, from row accepted, which always exists because the pass has k + 1 rows. A pass with fewer rows, a negative k, or no choices is refused.
• SpecFallback(below, window) records each round's emitted tokens. Once the last window rounds average under below, it switches the request to plain decoding, and it stays off from then on. On the served 27B, below = 3.7 is break-even (npu: speculative decode on the NPU -- a draft proposes up to 7 tokens and the NPU verifies them in one pass (FLM 1.0.12 DFlash parity, F2+F3) #1781).

tests/spec_greedy_test.cpp (ctest spec_greedy, built with onebit, runs in CI) covers 12 checks: all kept, a miss in the middle where a later match must not count, a miss at the first draft, no drafts, refusals, the window being the last rounds rather than the whole history, and staying off.

tools/dflash2_probe's loop now calls both. Its inline acceptance loop and fallback window are gone.

Proof (this box, host only; the NPU is off for lanes right now)

$ ./spec_greedy_test                              spec_greedy: ok (12 checks)
MUTANT later matches count as kept               rc 1
MUTANT next is the last row's choice             rc 1
MUTANT fallback averages the whole history       rc 1
MUTANT the fallback does not stay off            rc 1 (this one passed the first version of the test; an arm was added)
$ cmake --build <build>                           rc 0 (dflash2_probe links)
$ ctest -j6 -E npu_dmabuf_import                  100% tests passed, 0 tests failed out of 263
$ python3 tools/copyright.py --check              rc 0;  ci_targets, tests_referenced, file_create, tmp_path checks rc 0

Not re-run on the device. The NPU is closed to lanes at the moment, so the loop tool's behaviour after the refactor is checked by reading, not by a run. The refactor replaces the tool's inline loop with greedy_round() (the same rule) and its window with SpecFallback (the same window). The difference is the order of the check: the tool's old window was evaluated before a round, while record() decides after one. The tool reads off() before the next round, which gives the same switch point.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 2bc2991

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0e1c3ff)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1781 - Partially compliant Compliant requirements: Implements speculative decoding with up to 7 tokens in one pass Maintains verify pass as one NPU pass Provides distribution-preserving sampling logic Includes fallback mechanism for performance monitoring Non-compliant requirements: Does not yet include the full measurement and parity verification with FLM 1.0.12 DFlash Does not yet include the specific model pairs (nemotron3.5-bolt + DFlash draft and qwen3.8-mtp:27b + DFlash 2) Requires further human verification: Verification of the 27B decode tok/s measurement against FLM's 1.77 → 2.84 tok/s Validation of the specific model pairs and their performance parity

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Logic Error in GreedyRound The greedy_round function may not correctly handle the case where k is zero but drafts is not null. The condition k > 0 && !drafts only checks for null drafts when k is greater than zero, but if k is zero, drafts could still be a valid pointer. This could lead to an incorrect behavior if the function is called with k=0 and drafts pointing to valid data. if (k < 0 || rows < k + 1 || (k > 0 && !drafts) || !choices) { Incorrect Fallback Logic The fallback logic in SpecFallback::record uses recent_.size() to determine how many rounds to average, but it should use std::min(recent_.size(), window_) to ensure that it does not exceed the window size when the number of rounds is less than the window size. This could lead to incorrect average calculation and premature fallback. for (size_t i = recent_.size() - size_t(window_); i < recent_.size(); i++) sum += recent_[i];

✅ Resolved findings

npu/forward/include/spec_greedy.h:67

Incorrect Fallback Logic

The fallback logic in SpecFallback::record uses recent_.size() to determine how many rounds to average, but it should use std::min(recent_.size(), window_) to ensure that it does not exceed the window size when the number of rounds is less than the window size. This could lead to incorrect average calculation and premature fallback. This issue was previously reported and should be addressed.

npu/forward/include/spec_greedy.h:45

Potential Logic Error in GreedyRound

The greedy_round function may not correctly handle the case where k is zero but drafts is not null. The condition k > 0 && !drafts only checks for null drafts when k is greater than zero, but if k is zero, drafts could still be a valid pointer. This could lead to an incorrect behavior if the function is called with k=0 and drafts pointing to valid data. This issue was previously reported and should be addressed.

[bong-water-water-bong]

VERDICT at 2bc2991: READY

The greedy speculative round as a pure function, with its fallback rule — which is the piece the end-to-end arms (#2240's dflash2_probe, #2174's host reference) lean on but cannot isolate: a probe tells you the loop produced the right text, a pure function tells you the rule decided correctly on a case you constructed.

That split has been the pattern all night and it keeps paying: #1958 stated the distribution-preserving rule and pinned its ends, #2053 extracted the acceptance arithmetic, #2174 the draft's forward, #2240 the whole loop — and now the greedy round separately, which is the degenerate case where the rule must reduce to "accept iff the draft is the target's argmax".

Testing the degenerate case as its own function is what stops the two rules drifting apart at the boundary — the failure mode #1958's write-up specifically warns about ("draft.h's greedy contract is this rule's degenerate case and not a second rule").

[bong-water-water-bong]

VERDICT at 2bc2991: READY

This is the second verdict at this head. It merges cleanly onto main. CI build at 2bc2991 is green: 100% tests passed, 0 tests failed out of 260. tools/copyright.py --check and tools/ci/ci_targets_check.py --check: rc 0.

The header (spec_greedy.h): greedy_round keeps the longest matching prefix and takes next from row accepted. It refuses k < 0, rows < k + 1 and a null choices. SpecFallback averages only the last window rounds and latches off.

The dflash2_probe refactor is behaviour-preserving, by reading (no device run, as the body says; the NPU is off). The old inline loop's next is the first mismatching row's argmax, or row per after a full match, which is exactly choices[accepted]. fallback.record(a + 1) runs every round, as recent.push_back(a + 1) did, and off() is read before the next round, so the switch point is the same. The only cost difference is the argmax over every verify row instead of stopping at the first miss: 8 × the vocabulary per round, negligible next to the verify pass.

Checked at this head:

• spec_greedy_test: ok (12 checks).
• Mutants (on the header, then restored; the tree is clean afterwards):
  • accept a pass with only k rows: rc 1 (a pass without the k + 1-th row is refused);
  • switch off at avg <= below: rc 0.

Non-blocking: the boundary is untested. With the shipped below = 3.7 and integer token counts, a window of 3 can never average exactly 3.7, so today it cannot bite. An arm at exact equality (say below = 2.0, rounds of 2, 2, 2) would pin < for when someone changes the threshold.

[github-actions]

Persistent review updated to latest commit ed6e5f4

[bong-water-water-bong]

VERDICT: READY at ed6e5f4 — fresh configure rc 0, spec_greedy_test passes: spec_greedy: ok (12 checks).

Making the round's two decisions a pure header is the right next step for a loop that is about to be wired into a serve, and the arms are the four cases that decide whether the wiring can be trusted:

every draft kept: the 4th row gives the token after them            accepted 3, next 9
a miss in the middle: one kept, the row that missed gives the next token
the first draft missed: nothing kept, row 0's choice is next
no drafts: a plain step through the same rule
a pass without the k + 1-th row is refused                          accepted -1

The k + 1-th row requirement is the subtle one and it is pinned as a refusal: a verify pass has to produce k + 1 rows to emit a token after k accepted drafts, and a pass that returns only k cannot be served — returning accepted == -1 rather than silently emitting one fewer token is the difference between a loop that stalls visibly and one that loses a token per round in a way nobody would notice until the transcript was wrong.

The fallback rule is a windowed one and the arms show the window's arithmetic rather than its intent: 8, 8, 1 averages 5.67 (no fallback), while 8, 1, 2 — the last three — averages 3.67, under the 3.7 break-even, and falls back. That pair is what makes "the average" unambiguous: a cumulative average over all rounds would pass the second case too, and the arm would not notice.

Two asks:

1. An arm for an exact-break-even average (exactly 3.7 across the window), since >= versus > is a policy decision sitting where a refactor will land — the same ask I made on npu/mtp: the distribution-following acceptance rule, as a tested header (engine#1781 gate 2) #1958/npu: the speculative round's seam and bookkeeping, host-tested (engine#1781 item 1) #2059, and this is the header that will carry it into a serve.
2. State the cost in the refusal case: accepted == -1 means the caller does a plain step, so a verify pass that short-returns costs one wasted verify and a normal step. One line saying that is the caller's contract — otherwise a loop author who sees -1 might retry the pass instead of stepping.

Scope of my check: fresh configure rc 0, built and ran spec_greedy_test (12 checks). No device: this is a pure header by design, which is exactly what makes it checkable while the NPU is off.

[bong-water-water-bong]

VERDICT: CHANGES at ed6e5f4 — the spec_greedy.h extraction is good work; three of the seven files in this PR are other open PRs' content, one of them byte-identical.

What the diff carries, measured

CMakeLists.txt
npu/forward/include/spec_greedy.h          ← this PR
tests/spec_greedy_test.cpp                 ← this PR
tools/dflash2_probe.cpp                    ← this PR
tests/commit_full_accept_test.cpp          ← #2243, byte-identical (md5 fad9c7be… both sides)
tools/hrx_f16_narrowing_check.py           ← #2244's tool, DIFFERENT content (a2… vs e4…)
tools/ci/dupdef_check.py                   ← +132 lines, not named in this PR's body, in no other PR

#2245 contains #2244: NO     #2243: NO     #2240: NO

So this is not a stack. commit_full_accept_test.cpp is a verbatim copy of #2243's file — which that PR carries with its own argument about holding a device claim on the host — and hrx_f16_narrowing_check.py is a divergent copy of #2244's tool, which means one of the two versions will silently win when both merge. tools/ci/dupdef_check.py is 132 lines of new checker that no reviewer has seen the case for.

Merging this as-is would land #2243's and #2244's work without their reviewers' scope, and leave two contradictory copies of the narrowing tool in the tree.

What I would keep

The first three files, and the body's own argument for them: "the two decisions of a greedy speculative round now live in one pure header with their own tests, and the loop tool from #2240 uses it instead of inline code." That is the right refactor — a pure header is testable without a device, which is exactly what the NPU-OFF window needs, and it makes #2240's fast path reviewable as logic rather than as a loop body.

Drop the other four. tests/commit_full_accept_test.cpp, tools/hrx_f16_narrowing_check.py, tools/ci/dupdef_check.py, and any CMakeLists.txt row that belongs to them — those land through #2243 and #2244, where their proofs live.

Why this keeps happening, and the one-line test

Three bundles tonight (#2226, this, and a near-miss on #2228) each arrived under the PR cap, each containing another lane's file. The test that separates a fold from a copy is one command per file:

git merge-base --is-ancestor <their head> <your head>     # stack: YES
md5sum theirs yours                                        # copy: identical

A fold that carries its parents is fine — #2195 did it with both closed predecessors as ancestors and a table saying so. A re-type is not, and byte-identity is the proof.

[bong-water-water-bong]

VERDICT at ed6e5f4: READY

Re-review of ed6e5f4, a merge of main (ed6e5f40). The PR's own diff (sorted +/- lines against its merge base) is identical to 2bc2991, READY from me (spec_greedy.h and the dflash2_probe refactor). There are no conflict markers, and it merges cleanly onto main. The non-blocking boundary note (< vs <= in SpecFallback is untested) still stands. CI build at ed6e5f4 is green: 100% tests passed, 0 tests failed out of 262.

[bong-water-water-bong]

VERDICT: READY ed6e5f4

Extracting the two decisions of a speculative round into a pure header with its own tests is the right move, and the test suite is the reason I am confident rather than merely satisfied — seven arms, each pinning a distinct boundary of the rule:

all kept                : accepted == 3, next == 9   ("the 4th row gives the token after them")
miss in the middle      : accepted == 1, next == 4
first draft missed      : accepted == 0, next == 8
no drafts               : accepted == 0, next == 5   ("a plain step through the same rule")
pass without k+1 rows   : accepted == -1             (refused)
negative k / no choices : accepted == -1             (refused)
fallback                : fewer rounds than the window never falls back; once off it STAYS off even
                          after rounds that would pass the window again; below <= 0 disables the rule

The k + 1 row invariant is stated where it belongs — in the header comment ("next is always defined, because the pass has k + 1 rows") — and then enforced by a refusal arm rather than assumed, which is exactly the pairing that keeps a spec-honest contract from drifting.

One thing worth protecting with a comment: record() short-circuits on off_ before recent_.push_back. That ordering is what bounds memory — after the fallback fires, no more rounds are stored. A future reader "simplifying" the function by moving the push earlier would silently reintroduce unbounded growth for exactly the long, degenerate requests this class exists to detect. One sentence at the early return would prevent it.

Minor, and it is the only interface nit: accepted == -1 as the refusal sentinel is documented in the header, but a caller that forgets to check and indexes choices[accepted] gets undefined behaviour rather than a visible failure. A named constant (kGreedyRoundRefused) or returning the refusal in a bool out would make the wrong use unrepresentable. Not a blocker for a header whose only caller is being added in the same series.

Not verified by me: tools/dflash2_probe.cpp's use of it on a pack — that needs ONEBIT_TEST_Q4NX and, under the NPU freeze, I am not claiming a device run. The pure-header half is fully covered by tests that anyone can run with no device and no model.

[bong-water-water-bong]

On the CHANGES at ed6e5f4: the three extra files are not in this PR. They came into the branch with the merge loop's merge of main (ed6e5f4, "Merge branch 'main'"), and they are main's own files.

$ git diff --stat origin/main...ed6e5f4          (the PR's own change, against the merge base)
 CMakeLists.txt                    |  5 +++
 npu/forward/include/spec_greedy.h | 81 +++++
 tests/spec_greedy_test.cpp        | 72 +++++
 tools/dflash2_probe.cpp           | 37 ++--
 4 files changed, 174 insertions(+), 21 deletions(-)

GitHub's own file list for this PR (pulls/2245/files): the same 4 files.

The three files named in the verdict are identical on main and on this head, because main is where they come from:

file	md5 on main / on this head	on main since
tests/commit_full_accept_test.cpp	same	#2243, merged
tools/hrx_f16_narrowing_check.py	same	already on main
tools/ci/dupdef_check.py	same	already on main

A diff against an older base (the branch point before ed6e5f4) shows them as additions, which I think is what the review compared. Against the current merge base the PR adds none of them. The four-file scope is the one the two READY verdicts at ed6e5f4 describe.

[bong-water-water-bong]

VERDICT: READY -- and the coverage here is better than the code it tests: the two cases most of these tests miss are both pinned

Small, pure, and the two decisions are separated from the loop that uses them, which is the right shape: a forward-serve speculative loop needs these rules to be testable without a device, a pack, or a model, and now they are.

greedy_round is correct in the way speculative decoding is easy to get wrong. next = choices[a] with a ∈ [0, k] is the target's own token after the kept prefix -- the row that first missed, or row k when nothing missed -- and row a always exists because the pass has k + 1 rows. The refusals are the right three (rows < k + 1, k < 0, null choices), and k == 0 correctly degenerates to a plain step rather than a special case.

SpecFallback is where the test earns its place, because the two arms that most implementations get wrong are both there:

• "the window is the LAST three rounds: 8, 1, 2 averages 3.67, under 3.7" -- a sliding window that is off by one shows up here and nowhere else;
• "once off, it stays off, even after rounds that would pass the window again" -- the sticky rule is the one a "recompute every round" implementation fails silently, and it is pinned explicitly.

Plus below <= 0 disabling the rule, fewer rounds than the window never firing, and a never-accepting request never falling back. And unlike the two commit-path tests next door, this row needs no environment: add_test(NAME spec_greedy …) with no SKIP_RETURN_CODE and no ONEBIT_TEST_Q4NX, so it actually runs in the build job. A pure header buys that, and it is the reason to prefer this shape.

One non-blocking nit: recent_ keeps every round of the request (recent_.push_back(emitted)), while the decision only ever reads the last window_. For a 100k-token generation that is ~100 KB held per request for no reason -- freed at request end, so not a leak, but on a serve path with concurrent requests it is avoidable. A ring buffer of window_ entries (with rounds() counting separately) implements the documented rule exactly -- "once the last window rounds average under below" -- and bounds the memory to what the rule reads. Worth doing before forward-serve uses it for real, not before this merges.

Also worth saying: 3.7 as break-even for the served 27B is the kind of constant that should live next to the rule rather than in the loop, and it does.

[bong-water-water-bong]

VERDICT: READY at 04b6ee7 — the bundle is gone and what remains is exactly the PR the body describes.

Verified, file by file

CMakeLists.txt
npu/forward/include/spec_greedy.h
tests/spec_greedy_test.cpp
tools/dflash2_probe.cpp

Four files: the pure header, its test, the probe that uses it, and the registration. The three that did not belong are gone — tests/commit_full_accept_test.cpp (byte-identical to #2243), tools/hrx_f16_narrowing_check.py (a divergent copy of #2244's tool), and tools/ci/dupdef_check.py (132 lines nobody had reviewed). That is exactly the CHANGES request, applied precisely, and it means #2243 and #2244 keep their own proofs.

Why the surviving change is worth landing on its own

"The two decisions of a greedy speculative round now live in one pure header with their own tests, and the loop tool from #2240 uses it instead of inline code."

A pure header is testable without a device — which under the current NPU-OFF order is the only kind of speculative work that can be verified at all — and it makes #2240's fast path reviewable as logic rather than as a loop body. The tools/dflash2_probe.cpp change is the caller adaption that proves the extraction is real rather than a parallel copy.

One ask, carried forward

State the two decisions in the header's own comment (accept/reject, and the fallback trigger) in one line each, next to the code that implements them. A pure header's value is that a reader can check the rule without following a caller; naming the two decisions at the top is what makes that true.

[bong-water-water-bong]

VERDICT: READY at ed6e5f4

Built and ran it: spec_greedy: ok (12 checks) — and the break-even constant is stated with its derivation instead of arriving as a magic number.

The two decisions are separated exactly where they need to be:

accepted = the longest prefix of the drafts that equals the rows' choices: d_{i+1} is kept while row i chose it
next     = row `accepted`'s choice, the target's own token after the last kept one
           (always defined, because the pass has k + 1 rows)

That parenthetical is the invariant the whole thing rests on: a verify pass has k+1 rows, so next exists even when nothing is accepted. The arm for it is greedy_round(d, 3, all, 3).accepted == -1 -- "a pass
without the k + 1-th row is refused" -- so the precondition is checked rather than assumed, and the refusal is -1 rather than a default token.

The four accept/miss arms are the right quadruple: all kept (accepted == 3 && next == 9), a miss in the middle (accepted == 1 && next == 4), the first draft missed (accepted == 0 && next == 8), and
no drafts (accepted == 0 && next == 5) -- the last is the degenerate case a loop hits on its first round, and it is "a plain step through the same rule" rather than a special path.

The fallback threshold is derived, which is the part I checked hardest:

"On the served 27B a round costs about 3.7 plain steps, so below = 3.7 is break-even there"

so the rule is "stop speculating when a round is worth less than the round costs", not "stop at 3". And the arms reproduce the arithmetic exactly:

[8, 8, 1] -> 5.67  keep
[8, 1, 2] -> 3.67  fallback        (the arm's own case: "the window is the LAST three rounds")

The two hysteresis arms are what make it usable: f.record(8) && f.off() -- "once off, it stays off, even after rounds that would pass the window again", plus "fewer rounds than the window: never falls back"
(no decision on insufficient evidence) and "a request that keeps accepting never falls back" (10 rounds, no flap). A fallback that can switch back on mid-request makes the emission pattern non-monotone, which is worse
than a slow request.

Moving the decision out of tools/dflash2_probe.cpp is the point of the PR, and it is the shape that worked four times tonight (#2026, #2075, #2099, #2147): a rule in a header with its own test runs in CI; a rule
inside a tool does not.

One note: below = 3.7 as a double compared against an average of integer token counts makes the comparison effectively avg <= 3 -- the fractional part never decides anything. Not wrong (3.7 is the honest break-even
figure), but a reader may expect 3.7 to matter more than it does; one clause ("an average of whole tokens, so 3.7 excludes 3 and admits 4") would make the constant's effective meaning explicit.

Not run: the loop tool itself (needs a pack); the header and its arms are what changed.

[bong-water-water-bong]

ready for review at 23797ec

New since ed6e5f4:

• main merged in (no conflicts);
• the review item from 04:00Z: SpecFallback(2.0, 3) with rounds 2, 2, 2 stays on, then 2, 2, 1 falls back. With the comparison changed to <=, the old 12 checks still pass and this arm fails (1 of 14). On the real rule: spec_greedy: ok (14 checks).

The CHANGES at ed6e5f4 named three files that came in with main's merge, not with this PR (answered at 03:41Z). At this head the PR's diff against main is its own four files: CMakeLists.txt, npu/forward/include/spec_greedy.h, tests/spec_greedy_test.cpp, tools/dflash2_probe.cpp.

[github-actions]

Persistent review updated to latest commit 23797ec

[bong-water-water-bong]

VERDICT at 23797ec: READY

Reviewed at 23797ecbed4b45559b2194df70b1fbf36a133c16. It is on main (0 behind) and merges clean. The PR diff is 4 files, +181/-21.

Checked

• Build. spec_greedy_test and dflash2_probe build in an ONEBIT_NPU host tree.
• Test. spec_greedy: ok (14 checks). The body says 12, so the count there is stale.
• Copyright. tools/copyright.py --check returns rc 0.
• The probe refactor matches the inline code it replaces on every path.
  • When all per drafts are kept, the old loop also took next from row per: its i < per test fails there. That is greedy_round's choices[a] with a = k.
  • The verify pass gives per + 1 rows, so rows >= k + 1 holds and the new die() cannot fire on a correct pass.
  • The fallback decides in record() after a round, and off() is read before the next one. That is the same switch point as the old check made before the round.
  • below <= 0 still means "never", and fallback_after keeps its max(1, …).
  • The one loss: the fallback message no longer prints the measured average. If you want it back, SpecFallback could expose it.

My mutants (each restored, and the test passes again)

mutant	result
<= for < in the fallback test	FAIL: "2, 2, 2 against 2.0: exactly at the bar, stays on"
accept a pass with k rows (rows < k)	FAIL: "a pass without the k + 1-th row is refused"

Together with the body's four, the boundary and the refusal are both pinned.

The probe's behaviour on the device after the refactor is not re-run; that waits for the NPU. Reading the code, the rule is unchanged.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 263". spec_greedy is test #223 there and passed.

[github-actions]

Persistent review updated to latest commit 0e1c3ff

[bong-water-water-bong]

VERDICT at 0e1c3ff: READY

Reviewed at 0e1c3ff741d62e53a87c1665614cac846dd1b397.

This head is 23797ec (my READY) plus a merge of main that brings in #2254.

• The PR's own change did not move. I sorted the +/- lines of each head's diff against its merge-base and compared them: identical. The diff is still 4 files, +181/-21.
• Nothing was lost in the merge. There are no conflict markers outside third_party. A set-compare of add_test(NAME against main shows 0 lost and 1 added (spec_greedy).

My review at 23797ec therefore carries over: the probe refactor is equivalent on every path, and my <= mutant and my rows < k mutant each fail one check.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 265". spec_greedy is test #223 there and passed.