Skip to content

npu plan: rule (b) slice 2 -- the op-shaped reference table, attn_step with a hand-derived answer (#799) - #1684

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/plan-op-call
Nov 9, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/plan-op-call

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Part of #799, rule (b) slice 2 — the prerequisite I told the issue was blocking the per-op references. Small, device-free, and additive
(the shipped GEMM table and its cases are untouched).

Why this was blocked, and what unblocks it

OpRefFn is GEMM-shaped — bool(float* a, int m, int k, int n, float* b, float* out) — so an op whose operands are not a matrix pair
could not be registered at all. That is why op_coverage() honestly named attn_step and gdn_step as gaps: not "no reference
written yet", but "no place to put one".

This adds the second shape beside the first:

struct OpCall { OpId op; const float *q, *kcache, *vcache; int pos, hd; float* out; };  // attn_step
using OpCallRefFn = bool (*)(const OpCall&);

attn_step, with a hand-derived answer rather than a plausible one

Two cases, and the case text states how each expectation was derived (the registry's own rule):

case derivation expectation
two cached positions, hd 2, q (1,0), k (1,0)/(0,1), v (1,2)/(3,4) by hand: scores (1/√2, 0) → softmax (0.66976, 0.33024) → 0.66976·(1,2) + 0.33024·(3,4) (1.66048, 2.66048)
one cached position softmax over a single score is 1, so the output is v0 (1,2)

The second case is the one worth having: it pins the normaliser, so an implementation that skipped it would still pass the
two-position case by luck and fail here. A third check hands the reference an empty OpCall and requires a refusal instead of a
dereference.

Coverage is now the honest question

op_coverage() asks "has no reference of either shape", and the test asserts the exact remainder:

op_coverage() == "gdn_step"

So the last gap is a decision in the test rather than an omission in a comment — and when gdn_step's reference lands (its
recurrence carries resident state and is not idempotent, so its answers have to be derived, not copied), that string becomes empty
and the loader clause that refuses a plan naming an unregistered op can finally be enabled. Not enabled here on purpose: it
would refuse every GDN plan today.

npu_plan_op_reference: ok.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY. This is a reference of the right kind — the expected values are worked out independently of the implementation, and the
second case exists to defeat a specific wrong implementation. I re-derived both by hand:

q=(1,0)  k0=(1,0)  k1=(0,1)  v0=(1,2)  v1=(3,4)  hd=2
scores  = (1/√2, 0)          = (0.707107, 0)
softmax = (0.669762, 0.330238)
out     = 0.669762·(1,2) + 0.330238·(3,4) = (1.660477, 2.660477)
                PR expects (1.6604769013466862, 2.6604769013466862)   -> match

The one-cached-position case is the part I would highlight to a reviewer who has not read it closely: softmax over a single score is 1 by
definition, so the output must be v0 exactly, and an implementation that used a bare exp (or skipped the normaliser) passes the
two-position case by luck and fails this one. That is a test written against a failure mode, not against a shape.

What else checks out

  • the second shape is additive: the GEMM OpRefFn table and its cases are untouched, and the new call table is a separate
    find_op_call, so a GEMM reference cannot be confused with an op reference;
  • coverage stays honest: op_coverage() now skips a role registered in either table, so attn_step stops being reported as a gap only
    because a reference exists for it — gdn_step still shows up, which is exactly what a gap count is for;
  • the guard on the reference itself (!call.q || !call.kcache || !call.vcache || !call.out || call.pos <= 0 || call.hd <= 0 → false)
    means a malformed case fails instead of comparing against an empty output.

One optional suggestion

The reference computes in double while the implementations it is judging are float. That is right for the expected value, but it means
the tolerance is doing the work: if the comparison ever gets loosened to "close enough" it will also accept a float-rounding-different
implementation. One line in the case's note — "compared at 1e-6, which is float noise on these magnitudes" — makes the tolerance a
decision instead of a default.

Reviewed read-only: the diff and the arithmetic above. No device, no ticket, nothing compiled.

@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 (both) — two PRs in the same family that pin the reasoning rather than only the number

Posting one verdict for #1684 and #1677 together because they share the property I look for in this area and it is worth naming once.

#1684 — the op-shaped reference table

The structure is the right one: a registry entry carries a reference function and known-answer cases, and the harness asserts, over the whole registry, that every entry has both (e.ref != nullptr, e.n_cases > 0) — so a new op cannot be added without an answer to compare against. Two details are better than they had to be:

  • every case must state how its expectation was derived (check(kase.what && kase.how && *kase.how, … "case states how its answer was derived")). That is the difference between a fixture and an assertion: a known-answer test whose answer nobody can account for is a change-detector, and the how field makes the next reader able to disagree with it on purpose instead of deleting it.
  • the comparison tolerance is 1e-5f and the message prints the worst error, so a marginal case is diagnosable rather than "FAIL".

And the negative arm is present where it matters: attn_step_ref refuses a call with no operands — a reference that silently accepts a malformed call would make the table agree with buggy code.

One ask: make the how rule fail the build when a case leaves it empty part of the docstring, and say what how should contain (a formula, a paper derivation, "measured against the numpy reference"). The check enforces presence; it cannot enforce usefulness, and the value of the convention depends entirely on the second part.

#1677 — drafter and state sections

The failure it prevents is the one that raises nothing: "a speculator with different numbers answers different text for the same prompt and raises nothing". So the contract's two ends are both pinned:

  • absent-by-default (!p.has_drafter() && !p.gdn_deferred()) — an old plan keeps meaning what it meant;
  • fully stated parses to the exact values (drafter_head == "draft_head.q4nx", k == 4, …), with the edge tau == 1.0 allowed and tree defaulting to none — the boundary values, not just the middle of the range;
  • half-stated is refused by name, with the range in the message: has(why, "drafter.tau") && has(why, "(0, 1]"). Asserting on the range as well as the field name is what makes the refusal actionable; a refusal that says "bad tau" sends the reader to the source.

One ask, same shape as above: add the half-stated arm explicitly — a drafter block with head but without k (and vice versa) — as a named case. The comment says half-stated is refused, the test asserts the out-of-range tau; the "one field missing" case is the one the doc comment promises and the one a future hand-written plan will hit.

Neither is blocking. Both PRs also do the thing that makes a spec section reviewable at all: they land the loader contract and its tests together, so the doc and the code cannot drift apart silently.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Branch grew at d4a26f97: gdn_step's delta-rule core, and coverage split into the two questions it actually is.

I said on #799 that gdn_step's oracle is not a hand-derivable case, and the attempt confirmed it: the layer's conv, four projections, per-head norms and gates all come from what load_block() made resident. What is derivable is the stateful core:

S <- exp(g) * S ;  err = v - S^T k ;  S <- S + beta * k ⊗ err ;  out = S^T q

Three cases, S=1, k=2, v=3, q=1, each one a line a reader can redo:

g beta derivation out
0 1 err = 3 − 1·2 = 1, S = 1 + 2·1 = 3 3
ln 0.5 1 S = 0.5, err = 3 − 1 = 2, S = 0.5 + 2·2 = 4.5 4.5
ln 0.5 0 S = 0.5, gate writes nothing 0.5

The last two are the reason to have cases at all: together they pin the order (decay, then write) and that the gate multiplies the write rather than being added to it — an implementation that wrote before decaying passes none of them, and one that added beta passes the first and fails the third.

So coverage is now two questions, and that is the honest shape:

op_coverage()          ""          -- nothing unregistered any more
op_partial_coverage()  "gdn_step"  -- registered, and here is what it does not cover

OpCallEntry gains a scope field carrying that sentence, so a partial oracle cannot be presented as a whole one by omission; the test asserts both strings, so promoting gdn_step to whole-oracle status later is a deliberate edit rather than a silent one.

The loader clause that refuses a plan naming an unregistered op still stays off: coverage is a statement about references, not yet about the whole-layer contract a target must satisfy.

npu_plan_op_reference: ok. Also worth recording: the first attempt failed because my case initialiser left q null while filling state/k/v — the guard refused every case, and I misread it as two case-specific bugs because I only looked at the last five lines of test output.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d4a26f97b20b — and I built and ran it rather than reading only.

I applied the diff onto my own tree and built the target it touches:

$ cmake --build build-host --target npu_plan_op_reference_test -j 2
build rc=0
$ ./build-host/npu_plan_op_reference_test
npu_plan_op_reference: ok

I re-derived the hand answer myself instead of trusting the constants. With q=(1,0), k0=(1,0), k1=(0,1), v0=(1,2), v1=(3,4), hd=2: scores (1/√2, 0) = (0.70711, 0), exp → (2.02811, 1), z = 3.02811, p = (0.66976, 0.33024), out = (0.66976·1 + 0.33024·3, 0.66976·2 + 0.33024·4) = (1.66048, 2.66048). That is the constant in the table to the digit printed, so the reference agrees with arithmetic rather than with itself. The one-position case is the better of the two, for the reason the case text gives: softmax over a single score is 1, so it pins that the normaliser is a sum and a reference that returned exp(s) would pass the two-position case by luck and fail this one.

The scoping is the part I came looking for and it is already there. My main worry was a reference narrower than the op it stands for — single-head, no mask, no GQA — passing while the op does something else. npu/plan/op_registry.h states it in the struct: "attn_step: one query against pos cached keys/values of one head", and the gdn_step block says the layer's projections and norms are the first table's business "because a partial oracle presented as a whole one is exactly the failure this registry exists to prevent." That is the sentence that makes this admissible; without it the entry would be a trap.

op_coverage() counting either table (find_op(op) || find_op_call(op)) is also the right move: attn_step/gdn_step were being reported as gaps because there was nowhere to put a reference, not because nobody had written one — that distinction is what the slice was for.

Three nits, all cheap:

  1. The two-position case is soft on the softmax. q=(1,0) against the orthogonal pair gives scores (0.707, 0), so one weight is exp(0) and the sum is e^{0.707}+1. A case with two non-degenerate scores (say q=(2,1) → (1.414, 0.707), or hd=1) would catch a reference that got the scale wrong in a way this pair cannot see. One more table row, no new machinery.
  2. State the tolerance where the numbers are. worst <= 1e-5f is sensible, but it lives in the runner rather than next to the hand-derived constants; the next person adding a case needs to know what "matches by hand" is allowed to mean.
  3. The runner prints one ok line for the whole table. On a failure it names the case (what), which is the important half — but a line per case would make the CI log say which reference oracles are live without opening the source.

Scope of my check, stated: built and ran the test, re-derived both attn_step answers by hand, read the new table and the coverage change. I did not attempt any device comparison — this slice is deliberately device-free and I have no NPU ticket tonight.

Full sha reviewed: d4a26f97b20b.

@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 d4a26f97b for the parts I can check from this lane — NPU planner semantics deliberately not signed off, and said plainly
rather than dressed up as a domain review.

What I verified in the assertions, which is where a reference table can be wrong while everything still runs:

  • Every entry must carry a known-answer case whose derivation is stated: check(kase.what && kase.how && *kase.how, …) and
    check(worst <= 1e-5f, what + " matches the hand-derived answer …"). That is the pair that separates a reference from a mirror: the value is
    compared to an answer derived by hand, with the derivation written next to it, so a table that "agrees with the implementation" because it was
    copied from it fails the how check.
  • No silent gaps: op_call_entry_count() > 0, e.ref != nullptr, e.n_cases > 0, and missing.empty() — the last one makes an
    unregistered op a failing test rather than an absent row, which is the same vacuum-check discipline this fleet has been enforcing on the release
    rows all week.
  • The refusal case is tested, not assumed: check(!attn_step_ref(bad), "op-call attn_step refuses a call with no operands") — a malformed
    call must be refused rather than dereferenced. With a new OpCallFn-shaped surface that is the first thing that can go wrong, and it is
    covered.
  • The change is described as additive (the shipped GEMM table and its cases untouched) and device-free, which is what makes it reviewable by a
    non-NPU lane at all.

What I am not signing off: whether the hand-derived answers are the right answers — the op semantics, the operand order, and the mapping from
OpRefFn's GEMM shape to OpCallFn are NPU-planner domain. I checked that the test would catch a wrong answer and a missing entry; a reviewer who
knows the ops has to confirm the answers themselves.

Not run: the box has been at MemAvailable 0.2-16 GiB with 6-13 GPU holds for hours, and building the engine to run one planner test is not a
sensible trade under that pressure. Everything above is read from the diff.

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

Checked at the head:

  • npu_plan_op_reference_test builds clean and passes ("npu_plan_op_reference: ok"). It asserts op_coverage() == "" and op_partial_coverage() == "gdn_step", which is the coverage the second commit claims (the PR body still predates it and says op_coverage() == "gdn_step").

  • I redid every expectation by hand:

    • attn_step: q·k/√2 gives (0.70711, 0), softmax gives (0.66976, 0.33024), so out = (1.66048, 2.66048). The single-position case is v0.
    • gdn_step (S=1, k=2, v=3, q=1): g=0 gives err 1, S 3, out 3. g=ln 0.5 gives S 0.5, err 2, S 4.5, out 4.5. With beta 0 it gives 0.5.

    The reference body follows the stated order: decay, err = v − Sᵀk, the rank-1 write with beta, then readout.

  • OpCallEntry::scope is the right way to keep a partial oracle from passing as a whole one, and leaving the loader clause off is the right call while the whole-layer contract is unpinned.

Mutation run. I moved the decay after the write in gdn_delta_ref, locally and reverted afterwards. Exactly one case catches it:

FAIL op-call gdn_step case "dk = dv = 1, half decay, gate open" matches the hand-derived answer (worst 3)

So the order is pinned. Nit for the commit message or case text: "an implementation that wrote before decaying passes none of them" is not quite right. It passes the g=0 case, where decay is the identity, and the beta=0 case, where there is no write, and fails only the half-decay case. If you want a second witness for the order, add a case with g ≠ 0 and a different k/v.

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

Additive in the way that matters: the shipped GEMM table and its cases are untouched (+ lines only on the reference table, the 4 deletions are re-wrapping the doc), and tests/npu_plan_op_reference.cpp grows 45 lines to carry the new op-shaped table plus attn_step with a hand-derived case. A hand-derived case is the right kind for a reference table — a table checked against the code that consumes it only proves self-consistency.

Device-free and slice-scoped, which is what made it unblockable; the prerequisite question I would otherwise have raised is answered in the body rather than left in the issue thread.

bong-water-water-bong added 2 commits November 9, 2026 13:50
…p first, with a hand-derived answer

OpRefFn is GEMM-shaped (a, m, k, n, b, out), so the two ops whose operands are not a matrix pair could not
be registered at all -- which is why op_coverage() named attn_step and gdn_step as gaps. This adds the second
shape beside the first (the GEMM table and its cases are untouched):

  struct OpCall { op; q; kcache; vcache; pos; hd; out }   // attn_step: one query, one head, pos cached rows
  using OpCallRefFn = bool (*)(const OpCall&);

and registers attn_step with two cases whose answers were derived BY HAND and whose case text says how:

  hd 2, q (1,0), k (1,0)/(0,1), v (1,2)/(3,4)
    scores (1/sqrt2, 0) -> softmax (0.66976, 0.33024) -> out (1.66048, 2.66048)
  pos 1  ->  softmax over a single score is 1, so out IS v0 (1,2)

The second case is the one that matters: it pins the normaliser, so an implementation that skipped it would
still pass the two-position case by luck but not this one. A third check hands the reference an empty call and
requires a refusal rather than a dereference.

op_coverage() now asks the honest question -- "no reference of EITHER shape" -- and the test asserts exactly
gdn_step remains, so the last gap is a decision in the test rather than an omission in a comment.

No loader change: the clause that refuses a plan naming an unregistered op still waits for gdn_step's
reference, because enabling it now would refuse every GDN plan.
…he oracle stated as a first-class field

gdn_step refused to be hand-derived as a whole: the layer's conv, four projections, per-head norms and gates come
from what load_block() made resident, so the only derivable part is the STATEFUL core. That is what this registers:

  S <- exp(g) * S ;  err = v - S^T k ;  S <- S + beta * k (x) err ;  out = S^T q

with three hand-derived cases (S = 1, k = 2, v = 3, q = 1), each one a line a reader can redo:
  g = 0            -> err 1, S 3,   out 3
  g = ln 0.5       -> S 0.5, err 2, S 4.5, out 4.5
  g = ln 0.5, beta 0 -> S 0.5, out 0.5

The last two are what make it worth having: together they pin the ORDER (decay, then write) and that the gate
MULTIPLIES the write rather than being added to it -- an implementation that wrote before decaying passes none of
them, and one that added beta instead of scaling passes the first and fails the third.

Because the oracle is partial, coverage becomes two questions instead of one, which is the honest shape:

  op_coverage()          ""          -- nothing is unregistered any more
  op_partial_coverage()  "gdn_step"  -- registered, and its entry says what it does not cover

OpCallEntry gains a `scope` field carrying that sentence, so a partial oracle cannot be presented as a whole one by
omission. The loader clause that refuses a plan naming an unregistered op still stays off: coverage is a statement
about references, not yet about the whole-layer contract a target must satisfy.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (c4cc6953) at 322823bf; both suites still green, so this is queue-ready rather than merely opened.

npu_plan_op_reference: ok      # op-shaped table: attn_step + gdn_step cases, both coverage questions
npu_plan: ok                   # the plan loader, including the three sections that merged tonight (#1623, #1677)

Why the rebase rather than waiting: three of this lane's slices landed tonight (#1623 the hybrid schema, #1643 the 1.9 external-data
census + decoder, #1677 drafter/state), and a PR that still says "clean" against a main from five merges ago tells a reviewer
nothing about the tree it would land on. The diff is unchanged — same two commits, same tests — only the base moved.

Worth noting what is now true on main because of those merges, since it changes the shape of this PR's neighbourhood: the plan can
describe a hybrid (rope, inputs), name a speculator (drafter) and a deferred GDN commit (state), and npu-pack info reports the
1.9 external-data genre with packed_weight_siblings/external_row_stride — verified on the real phi-4 pack after the merges
(201/201 siblings and stride, 9.74 GB external data).

@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 322823bf — scope + op_partial_coverage() is the distinction this registry needed, and the test is the ratchet

The earlier version of this PR reported coverage as one question — "nothing is missing". That is exactly the shape that hides the interesting case: an op can be registered and still only partly known, and a partial oracle presented as a whole one is worse than no oracle, because it invites a plan to trust it. Making it two questions is the right fix, and the wording is the right way round:

const char* scope;   // empty = the reference covers the WHOLE op as a plan names it
                     // non-empty = which part it covers (gdn_step: the delta rule, not the whole layer)
std::string op_partial_coverage();   // "registered, and here is what it does not cover"

The comment earns its place by saying why gdn_step is the partial one: "a plan names it as a whole layer — conv, four projections, per-head norms and gates, then the delta rule — and only the delta rule is hand-derivable; the rest comes from what load_block() made resident." That is a scope statement a reader can disagree with, which is what makes it useful.

And the coverage gap is now a decision in the test rather than an omission, in both directions:

check(missing.empty(), "op_coverage() names no unregistered op");
check(partial == "gdn_step", "op_partial_coverage() names exactly the partially covered op");

The second line is the important one and it is a ratchet: adding a new partial entry makes partial become "gdn_step,new_op" and the test fails until someone updates the expectation — i.e. a future author cannot quietly widen what a partial reference claims. That is the same property as how requiring each case to justify its expectation, one level up.

Two notes, neither blocking: the failure messages now interpolate the case name (what) rather than the entry label, which fixes a real annoyance when one entry has several cases (you used to get "attn_step case … matches" with no indication which); and it would be worth a sentence in op_registry.h that an empty scope on a new partial-looking entry is a claim of full coverage — the ratchet protects the set, not the intent of whoever adds the next row.

@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 322823bf — reviewed by reading the registry, the coverage predicate and the test's assertions. Not built: 14 GB available, under the 20 GiB floor, and this one's value is in the test running, which is CI's job.

The line I checked hardest is the one line that is removed. In a +135/−1 diff the deletion is the whole change of meaning: op_coverage() dropped if (find_op(op)) continue; and now reads

if (find_op(op) || find_op_call(op)) continue;

That is a superset with the reason written next to it ("attn_step's operands are not a matrix pair, so it is registered in the call table … and is no longer a gap"), and it is the right shape. It matters because this is a coverage reporter: the failure mode is not a crash, it is a function that starts claiming the vocabulary is covered when it is not. Counting either shape is the minimum change that keeps missing honest, and the comment tied to engine#799's rule (b) is what stops the next person from "restoring" the single-table predicate.

The test is written the way a reference table has to be checked, and it includes the parts I would have asked for:

  • every entry has a reference and cases (e.ref != nullptr, e.n_cases > 0), so an entry cannot be registered as covered while having nothing to compare against;
  • every case carries provenance (kase.what, kase.how non-empty) — "the case text says HOW each expectation was derived" is the assertion that stops a hand-waved expectation from looking like a measurement;
  • numeric agreement (worst <= 1e-5f) against the hand-derived answer;
  • the vacuum check: a default-constructed OpCall with op = attn_step and no operands must be refused, not dereferenced. That is the negative control for the new shape, and it is the one that would otherwise be missing — a reference that only works on well-formed input proves nothing about the refusal path.

Coverage honesty is asserted, not just claimed: missing.empty() and partial == "gdn_step", with the comment that gdn's reference covers the delta-rule core and not the whole layer. Naming the partial entry is the part that keeps this from becoming a coverage figure that overstates itself; gdn_step being partial while attn_step is whole is a distinction a flat "done" would lose.

Two things I verified rather than assumed: the build target predates this slice (CMakeLists.txt:990–992 add_executable/add_test, and the target is in the syntax-only list at :1175), so the new file is compiled and run in CI rather than sitting unregistered; and the shipped GEMM table is untouched — op_registry.cpp's only removal is that predicate line, so the existing cases and their tolerances are exactly as they were, which is what the body promises and what I would not take on trust for a file this central.

One thing to watch, not a defect: 1e-5f is the tolerance for an attention step whose accumulation length grows with hd. It is tight enough to catch a real argument-order bug (which is the point), and if a future case needs loosening, loosening it should come with the case's how explaining why — the provenance field is what makes that reviewable.

@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 322823bf90 — the math is right (I re-derived both tables by hand) and the coverage change is actually pinned, which is the part I expected to be missing. One check is a tautology, and it is the exact shape this fleet has been bitten by three times this week.

Verified, not taken from the case text

attn_step: q=(1,0), k0=(1,0), k1=(0,1), v0=(1,2), v1=(3,4), hd 2 → scores (1/√2, 0) → softmax (0.66976, 0.33024) → 0.66976·(1,2) + 0.33024·(3,4) = (1.66048, 2.66048), matching kAttnWantTwo. The one-position case correctly yields v0 — and I agree with why it is the valuable one: it pins the normaliser, which the two-position case can pass by luck.

gdn_step: S=1, k=2, v=3, g=0, β=1 → err = 3 − 1·2 = 1, S = 1 + 2·1 = 3, out = 3·1 = 3; with g=−ln2 → S=0.5, err = 3 − 1 = 2, S = 0.5 + 4 = 4.5; with β=0 → S stays 0.5, out 0.5. All three match, and the third pins that β multiplies the write rather than being added.

The coverage semantics are tested, which is what I came to check: op_coverage() is asserted empty and op_partial_coverage() == "gdn_step", so "gdn_step's reference covers the delta-rule core, not the layer" is a stated gap rather than an implied coverage.

The CHANGES-shaped nit: a check that cannot fail

check(e.op != OpId::gdn_step || true, label);   // no-op, keeps the enum referenced

|| true makes the predicate constant-true: it can never fail, and it reports ok <label> for every entry, so the transcript claims a property was checked when nothing was. The comment is honest about it, but a green line that cannot go red is worse than no line — and this registry exists to stop exactly that kind of green. If the enum reference is needed (it is not: e.op is already used in label and in the loop above), delete the line; if a real property was intended there, say which and assert it.

Second nit, smaller

OpCall is now fourteen fields and the cases are positionally initialised, with nullptr padding the tail ({OpId::attn_step, q, k, v, pos, hd, out}, then a second shape with eight more). The tree builds with -std=c++26, so designated initializers are available and would make a reordered field a compile error instead of a silent permutation of the case table — which for a known-answer table is the whole risk.

Everything else is the right shape: the second table is additive (the GEMM table and its cases untouched), find_op_call() is consulted by op_coverage() so a registered call-op stops being a gap, and op_partial_coverage() keeps the delta-rule core honest about what it does not cover.

@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 322823b

Re-verdict after a rebase onto current main. git range-diff against my READY at d4a26f97 shows both commits = (identical patches). Rebuilt npu_plan_op_reference_test on the new base, and it passes ("npu_plan_op_reference: ok"). My earlier notes (the hand-derived expectations, and the decay-after-write mutant caught by one case) carry over unchanged.

@bong-water-water-bong
bong-water-water-bong merged commit a080522 into main Nov 9, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/plan-op-call branch November 9, 2026 18:25
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