Skip to content

npu target: write out the optional ops' contracts, because a reference cannot be written against a signature that omits its state - #1427

Open
bong-water-water-bong wants to merge 1 commit into
mainfrom
1bit/npu-op-contract
Open

bong-water-water-bong wants to merge 1 commit into
mainfrom
1bit/npu-op-contract

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What, and why it is not the references themselves

Rule (b) (#799) says every op a plan may name ships a host reference checked against known answers. gemv and lm_head now have one (#1409). Writing the missing two — gdn_step, attn_step — ran into the actual obstruction, and it is in the interface rather than in the arithmetic:

their arguments are (layer, M, x|q, y|o) and nothing else, while both ops consume the layer's parameters and its recurrence state — GDN's conv kernel, wa/wb, ssm_a, dt, norm and scan state; attention's key/value cache. A host reference has to take those explicitly, so the first thing that has to change is the contract saying where they come from, not a guessed oracle.

So this PR states it, without touching a signature:

  • what x/y (and q/o) are, with shapes, row-major;
  • that the parameters and state are the layer's resident ones (load_block()), not arguments — which is exactly why a reference needs them passed;
  • that gdn_step is not idempotent (it advances the state) while lm_head is;
  • and one sentence saying the registry entries for the first two wait on a reference that takes the parameters explicitly, so the next reader does not repeat the attempt.

Why comments-only is the right size here

An oracle guessed from a signature that omits its inputs is worse than a missing one: it would be indistinguishable from a correct reference until a device row failed, which is the failure mode the rule exists to prevent. The alternative — pointing the reference at a copy of the model path's own code — satisfies the table and defeats the purpose, because it can share the mistake.

No behaviour change (comments only), and the signatures are untouched, which also keeps pi-hexagon's P5 target able to implement them as they read today.

row verdict
tools/copyright.py --check rc 0
build not run: comments only, no compiled line changed

Refs #799.

…e cannot be written against a signature that omits its state

Rule (b) says every op a plan may name ships a host reference checked against known answers.  Trying to
write the missing two -- gdn_step, attn_step -- ran into the actual obstruction, and it is in the
interface rather than in the arithmetic: their arguments are (layer, M, x|q, y|o) and nothing else, while
both ops consume the layer's parameters AND its recurrence state (GDN's conv/wa/wb/ssm_a/dt/norm and the
scan state; attention's key/value cache).  A reference has to take those explicitly, so the signature is
the first thing that has to say where they come from.

So this states it, without changing a signature: what x and y mean and their shapes, that the parameters
and state are the layer's resident ones rather than arguments, that gdn_step is NOT idempotent (it advances
the state) while lm_head is, and therefore that the registry entries for the first two wait on a reference
that takes the parameters explicitly.  The comment says that in one sentence so the next reader does not
repeat the attempt.

No behaviour change: comments only, and the signatures are untouched (which is also what keeps
pi-hexagon's P5 target able to implement them as they read today).
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

#1427 ready for review at 01d2370b — comments only, and it exists because the rule could not be satisfied without it.

I tried to write the two missing references (rule (b) slice 2) and hit the real obstruction: gdn_step(layer, M, x, y) and attn_step(layer, M, q, o) carry none of the layer's parameters or state, both of which the ops consume (GDN's conv/wa/wb/ssm_a/dt/norm + scan state; attention's KV cache). A reference has to take those explicitly, so the contract — not the arithmetic — is the first thing that must say where they come from.

So the comment now states: operand shapes, that parameters/state are the layer's resident ones from load_block() rather than arguments, that gdn_step is not idempotent while lm_head is, and one sentence saying the registry entries for those two wait on a reference that takes the parameters explicitly.

Why not guess an oracle anyway: one built against a signature that omits its inputs would be indistinguishable from a correct reference until a device row failed — the failure mode the rule exists to prevent — and pointing it at a copy of the model path's code would satisfy the table while defeating the point.

Signatures untouched (comments only), so pi-hexagon's P5 target can implement them exactly as they read today.

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

Writing the contracts out is the right next step after #1409's registry, and the part that makes it more than prose is that each one says what a
host reference would need:

  • gdn_step(layer, M, x, y) — x [M][H] in, y [M][H] out, and the layer's parameters (conv kernel, wa/wb, ssm_a, dt bias, norm) and
    recurrence state come from what load_block() made resident, not from the arguments — with the consequence spelled out: a host reference
    must take them explicitly, and the step is not idempotent because it advances state. That last clause is the one a reference implementer
    would otherwise get wrong silently, since every other op in this file is a pure function of its inputs.
  • attn_step(layer, M, q, o) — q/o [M][NH*HD], keys/values from the layer's own residency, same shape of contract.
  • the third (lm_head) in the same form.

This is what my #1409 review was missing when I asked for references for gdn_step/attn_step: a reference cannot exist until the signature
carries the operands, so the two PRs are the two halves of rule (b) — the registry says which ops, these contracts say what a reference has to
be given
. The layer-parameter clause also explains why the registry's OpCase is a known-answer triple per op rather than a generic fuzz shape.

Nothing to check beyond internal consistency, which holds: the three signatures match the Op/OpId names #1409 registers (gemv, gdn_step,
attn_step, lm_head), and the non-idempotence note is exactly why gdn_step will need a state fixture in its reference test rather than a single
call.

@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 01d2370b95eb94c8cae752c940d27b68f548fbdc

Comments only, in npu/target/target.h (+22 lines, above the three optional ops). No signature or compiled line changes, so not building is the right call. tools/copyright.py --check is rc 0 at this head.

What it says checks out against the tree:

  • load_block(int first_layer, int n_layers, std::string&) is the pure-virtual at target.h:115, the "resident per load_block()" the contract points at;
  • npu/plan/op_registry.cpp exists (#1409), and gemv/lm_head are the ops registered there;
  • no target in npu/ implements gdn_step or attn_step yet. That makes this the prescriptive contract a first implementation (pi-hexagon's P5 included) will be held to, which is the useful moment to write it.

The reasoning is the valuable part: "a reference cannot be written against an argument list that omits the state and parameters the op consumes", and a guessed oracle is worse than none. Stating the non-idempotence of gdn_step next to the idempotent lm_head is the detail a reference author would otherwise get wrong.

One thought for the follow-up, not a change request: attn_step says keys/values come from "the layer's own cache state (built by the forward …)". When the reference slice lands, the contract should also say who appends this step's K/V (the target or the caller), since that's the other half of attention's state and decides whether attn_step is idempotent too.

This branch has not been deployed

No deployments
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