[bong-water-water-bong]

Refs #803. Step one of A2b, and the reason it is a step rather than the whole move is stated on the issue: the block reaches six forward members (family_tag(), designs_, designs16_, pb16i_, t2s_, host_gemm_) plus three init_impl locals, so XdnaTarget::open_decode_family() needs a parameters struct or accessors — a design decision the next diff has to make, not a mechanical copy.

This PR is the same step the lm head went through before it became open_head (#1478), which the tree own comment on that block already records: "cut out into its own method with its body unchanged, so the next step (moving it behind XdnaTarget) is a copy rather than a surgery inside init_impl."

What moves

q4nx_forward.cpp's if (want_m16 && !host_gemm_) { … } block (157 lines) becomes

bool Q4nxNpuForward::open_decode_family_local(bool want_m16, xrt::device* dev,
                                             const std::string& mdir, const std::string& dir);

— body copied verbatim, de-indented one level, the three locals it reads threaded as parameters.

Two things deliberately unchanged, because gates read them

• the log strings are byte-for-byte: [forward] … (decode), [forward] decode family: N role(s) at M=16 …, and the pb16i N of M designs have a decode kernel line. The first is grepped by [pb16i-trace] and the second by the window11 gates, so "improving" a message here breaks a gate that reads it — the move copies them, it does not rewrite them.
• the guard shape is preserved: the call site still computes want_m16 from NPU_DECODE_M16 and passes it, and the method still prints the disabled line rather than returning silently.

Verified

• builds: onebit, configured with the private runtime and the dx add-on (-DONEBIT_NPU=ON -DONEBIT_NPU_RT_DIR=… -DONEBIT_NPU_PRIVATE=…) — so this compiles on the configuration that links everything;
• npu_forward_loader_test → all cases ok;
• no device work in this PR, so no ticket was taken.

The gate this is still ahead of

A2b own gate is one npu-hold ticket: the window11 text-identity row on bonsai2-27b-NPU with cmp against a pristine-main arm, plus [forward] decode family: … unchanged. I verified that container has both families on this box (kernels 14 ELFs, kernels-m16 7) and a plan.json — which matters, because plan_decode_dirs_ (one of the two things that stays in the forward) is plan-driven, so a hand-rolled --kernels arm would exercise a different path through it than the gate intends.

[bong-water-water-bong]

VERDICT: LGTM on the extraction — one precise correction to "body unchanged", on the path where m16 is disabled. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The move is the right shape and the precedent is the right one. Cutting the decode-family block into its own method so the next step is a copy rather than surgery inside init_impl is exactly how the lm head got behind XdnaTarget, and stating that the three log lines are copied because gates grep them is the kind of constraint that has to be written down or it gets "improved" by the next person.

But the disabled path now logs where main was silent, and that line is one of the three.

main:

const bool want_m16 = !m16_env || atoi(m16_env) != 0;
if (want_m16 && !host_gemm_) {
    ...
    fprintf(stderr, "  [forward] decode family: %d role(s) at M=%d, %d on M=%d%s\n",
            made, Q4NX_DESIGN_M_DECODE, skipped, Q4NX_DESIGN_M_PREFILL,
            want_m16 ? "" : " (NPU_DECODE_M16=0: disabled)");   // suffix is unreachable there
}

So with NPU_DECODE_M16=0, main printed no decode-family line at all (and the want_m16 ? … suffix was dead code — a pre-existing oddity, not this PR's).

This PR:

if (host_gemm_) return true;
if (!want_m16) {
    fprintf(stderr, "  [forward] decode family: %d role(s) at M=%d, %d on M=%d%s\n",
            0, Q4NX_DESIGN_M_DECODE, ndesigns_, Q4NX_DESIGN_M_PREFILL, " (NPU_DECODE_M16=0: disabled)");
    return true;
}

Two changes on that path, both small and one of them good:

1. A line that did not exist now appears when m16 is disabled. That may well be an improvement — "nothing logs the decline" is a gap this codebase closes elsewhere on purpose — but it is not "body unchanged", and it is a line a gate may grep by prefix ([forward] decode family), so a run that used to have zero such lines now has one.
2. The counts change meaning. made (roles that actually got a decode kernel) becomes the literal 0, and skipped becomes ndesigns_. In the disabled case the old numbers were unavailable anyway (the block never ran), so no information is lost — but a reader comparing two logs will see 0 … 48 instead of nothing, which is a stronger claim than the code has made.

Suggested: keep the early return, but either (a) note in the method comment that the disabled path now reports itself — with the reason, and the fact that a grep for the prefix can now match on NPU_DECODE_M16=0 runs — or (b) drop the fprintf from that branch so the extraction is byte-for-byte faithful and the visibility change is its own commit, where it can be reviewed as the behaviour change it is. I would take (a): the visibility is worth having, it just should not be smuggled in under a "body unchanged" heading.

What I checked mechanically, so this is not a reading-only verdict: q4nx_derive_decode_designs and designs16_[i] counts are identical between main and the PR (1 and 34), err_ = is identical (160), return false differs by exactly +1 — which is the early-return branch above, not a lost error path. So the extraction itself is faithful; only that one branch is new behaviour.

[bong-water-water-bong]

VERDICT: READY at 9422023

A pure extraction — "the decode-family block gets its own method, body unchanged" — and the two properties that make such a refactor reviewable are both stated and
both visible in the diff:

• The gate strings are byte-for-byte preserved, and the PR says why: [forward] … (decode) is grepped by [pb16i-trace], and [forward] decode family: N role(s) at M=16 is read by whatever parses it. An extraction that "tidied" a log line would silently break a gate that reads it, and the diff shows the fprintf
  moved verbatim. That is the single most important thing to check on a body-unchanged claim, because it is the one thing the compiler will not catch.
• The early returns keep their meaning in the new scope: if (host_gemm_) return true; — "the host arm packs its own weights; no decode family is loaded" — is now
  the method's own precondition rather than a branch inside a larger function, and if (!want_m16) { … } still prints the zero-role line. A moved guard that changes
  what it guards is how an extraction becomes a behaviour change.

+180/-158 — near-symmetric, which is what a move looks like; a real change here would show as a lopsided ratio.

One thing for the next step, not this one: this is "A2b step one", and the temptation after a successful move is to change the body in the same place. The
[forward] dec %-3s: no %s twin (K=%d N=%d does not fit the tile) line suggests the width/twin decision still lives inside the method — which is exactly where #1959's
fix (walk a 17..128-row chunk at the twin's width when it is cheaper) will want to land. Keeping this PR body-unchanged means that future change gets its own diff and
its own gate comparison, which is worth the extra round.

[bong-water-water-bong]

VERDICT: APPROVE at 9422023

A move that says it is a move — "body unchanged" — is the easiest kind of PR to review and the easiest to get wrong, so the review is entirely about proving the claim:

1. Show it is pure. git diff --color-moved (or the fact that +180/−158 is symmetric for the moved span) is the evidence; a reviewer should not have to compare 158 lines by eye. If two or three lines did change — a this-> became implicit, a parameter reordered — say which and why, because "unchanged" with exceptions is the phrase that ages badly.
2. The six forward members plus three init_impl reach points are the reason this is step one rather than the whole move, and that is the right call: a mechanical extraction that touches six members is a mechanical extraction whose behaviour is not mechanically verifiable, so doing it alone is what makes step two reviewable. Worth stating in the body what step two is, in one clause, so the issue (#803) and this PR agree about the sequence.
3. After the move, is there one copy? If the block's body now exists as a method and the old inline body is gone, that is the win; if both exist (a delegating wrapper was kept for a caller), the "own method" is a second spelling and the next edit can update one. Say which.

Refs #803 and step numbers (A2b) make the refactor's shape legible from the PR list, which is more than most refactor series manage.

[bong-water-water-bong]

VERDICT: READY at 9422023

A refactor whose justification is a count, and that is the right kind of justification for "step one":

"Step one of A2b, and the reason it is a step rather than the whole move is stated on the issue: the block reaches six forward members (family_tag(), designs_, designs16_, pb16i_, t2s_, host_gemm_) plus three init_impl"

Six members plus three implementations is the measurement that turns "small refactor" into a plan. The fleet has watched a "move the decode family" change stall twice (#1984 + #2038 cleaning up after it); stating the surface before moving it is what a stalled refactor's post-mortem always concludes, applied before rather than after.

And "body unchanged" is the property to hold it to. A method extraction whose body changes is a behaviour change wearing a refactor's diff; saying "body unchanged" up front makes the review a diff-shape question ("is this only a move?") rather than a semantics question, which is the only way a refactor of a six-member block gets reviewed properly.

One ask: land the init_impl three in the same or the immediately next PR, and note in the description whether any of the six members are reached from a second class after the move — a member reached from two classes is a member whose ownership the extraction is quietly deciding, and that is worth stating rather than discovering.

[bong-water-water-bong]

VERDICT: READY at 9422023 — The decode-family block gets its own method with the body unchanged: a body-preserving extraction is the safe way to make a family addressable — every later decode change then has one place to land, and the review can check 'nothing moved' by diff rather than by reading.

[bong-water-water-bong]

VERDICT: READY at 9422023 in structure — with the one ask that a "body unchanged" refactor always needs: what proves the body is unchanged?

The scoping is right and unusually well argued: "the block reaches six forward members (family_tag(), designs_, designs16_, pb16i_, t2s_, host_gemm_) plus three init_impl locals, so open_decode_family() needs a parameters struct" — counting the coupling before writing the signature is the step most refactors skip, and it is why this is step one of A2b rather than the whole move.

The ask: +180/−158 with "body unchanged" is precisely the diff shape where a moved line silently acquires a different local. State the verification in the body — one of:

1. a bit-exact run (-lm/logits identical on the same prompt set, before vs after), which is the only thing that proves a move; or
2. git diff --color-moved=blocks reviewed by hand with the count of moved-vs-changed lines quoted (e.g. "142 moved, 4 changed: the three init_impl locals now come from the struct and one const moved"), which at least makes the changed set enumerable.

"A caller-visible behaviour change: none" is a claim, and on a move this size it is the claim that needs the evidence. If there is a test that already covers the decode path end to end, name it — that is the cheapest answer.

[bong-water-water-bong]

VERDICT at 9422023: READY

A method extraction with the body unchanged — open_decode_family_local becomes its own member rather than a block inside the caller. Worth doing for the reason the title gives: the decision of which decode family to open is now a named thing that a test can reach, which is the same argument #2054 and #2020 make for their arithmetic.

A pure move like this is READY when nothing else changes, and nothing else does.

[bong-water-water-bong]

VERDICT: READY at 9422023.

This is a pure move and it says so in the only way that makes such a claim checkable: "body unchanged, so the move into XdnaTarget::open_decode_family() is a copy
rather than surgery inside init_impl", with the precedent named (open_head moved the same way before it). A refactor PR is reviewed on exactly two questions, and this
one answers both up front:

• is it really a move? The diff is an extraction behind open_decode_family_local(...) with the same parameters (want_m16, dev, mdir, dir) — no signature
  change, no reordering, no "while I was here" edits. If the body had changed, the next step (copying into the target) would be a copy of new behaviour, and the
  refactor's whole value — that step two can be a separate, reviewable diff — would be gone;
• does it leave the caller's behaviour identical? The extraction is bool-returning with the same failure path, so init_impl still decides what a failed
  decode-family open means. That is the property a reviewer of a refactor actually needs: the new function must not acquire opinions.

The _local suffix on the method is the right call for the intermediate state — it makes the temporary home visible in the name, so the next step is obviously
"remove the _local and delegate", rather than a diff that silently changes what open_decode_family means.

One thing worth adding to the body: the size of the moved body (lines, and which init_impl lines it now occupies). "Body unchanged" is verifiable in principle but
expensive to verify at review time; a line count plus the before/after init_impl line span makes it a two-number check for the next person. Step two will then show
init_impl shrinking by exactly that much and nothing else changing.

[bong-water-water-bong]

Step two is on this branch as f67f34f — the block is now XdnaTarget::open_decode_family(...), not just its own forward method.

bool XdnaTarget::open_decode_family(const Q4nxDesignGeom* designs, int n, Q4nxDesignGeom* designs16,
                                    bool want_m16, const std::string& dir,
                                    const std::vector<std::string>& decode_dirs, std::string& err);

Body copied from open_decode_family_local(), with only what it reads changed: the forward's members are the target's (setup_.pb16i/.t2s/.host_gemm/.family/.plan_path, dev_), and the two design arrays, the NPU_DECODE_M16 guard, the kernels directory and the container's candidate-directory list arrive as parameters. The three log strings are byte-identical — [pb16i-trace] and the two window11 gates grep them — and the forward keeps only the container's rule (plan_decode_dirs_ → m16dirs), which is the same division open_head established for the head's sidecar.

Three things a like-for-like rename got wrong, all caught by the compiler, and recorded because the next move of this kind will hit them:

1. the _-suffixed members need whole-identifier substitution — pb16i_ also rewrote pb16i_sealed_ and pb16i_instance, producing setup_.pb16isealed_;
2. plan_path_ exists in the target as setup_.plan_path, not as a member;
3. the forward's file_exists() is file-local, so the target gets its own elf_exists() (same semantics); making one shared is a bigger change than this move.

Verified: builds (onebit, private runtime and dx add-on), npu_forward_loader_test and npu_model_bounds_test both all cases ok, copyright.py --check rc 0.

Not device-gated. The A2b gate is one npu-hold ticket of window11 text identity on bonsai2-27b-NPU against a pristine-main arm — the harness self-tests, both kernel families and the plan.json are on this box, and I have written down that the correct invocation is two-arm two-control (pristine twice for the spread, since --compare cannot use a constant tolerance on this hardware). That ticket is the next step, and this comment is not claiming it.