[bong-water-water-bong]

Refs #803, refs #799 (A2b). Follow-up PR that lands #1984 and #2038 on current main; it replaces both.

Why a new PR: #1984 (the decode-family move) has conflicted with main in q4nx_forward.cpp since 03:01Z, and its author's lane was stood down; their handover comment on #1984 lists what a rebase owes. #2038 (my fix for the CHANGES I gave #1984) is based on #1984's branch. This is the move re-applied on main as it is now, with #2038 folded in.

Where it came from: the re-application was made from the reviewed heads of both PRs: #1984 at 6579423 (its XdnaTarget::open_decode_family() body and declaration, re-fitted to the current forward) and #2038 at 8b1c99a (its npu/plan/plan.h and tests/npu_plan_test.cpp hunks applied unchanged with git apply -3, and its one forward line). #1984 contributed the decode-family move to the target; #2038 contributed the plan layer's resolved_decode_dirs() and its test, which answered the CHANGES at 6579423. One deliberate difference from #1984: the file test is the forward's own fopen one, not std::filesystem, so "exists and is not empty" is decided the same way as before the move.

What changes

• XdnaTarget::open_decode_family() (new): the per-role loop that resolves each role's M=16 ELF (under Setup::decode_dirs, or the sealed image in the kernels directory), creates the context and configures it. Log lines and refusals are byte-for-byte the forward's.
• Q4nxNpuForward::open_decode_family_local() keeps the model's half: the decode design names, the summary lines, and the wide (npu: a sealed set can carry a wide prefill family, named by the pack or the environment: 28.3-30.8 -> 33.9-35.9 tok/s to first token with a 32-row set (Refs #1990) #2042) and narrow (npu: a sealed set can carry a narrow decode family: 3.26-3.33 -> 3.69-4.21 tok/s decode with a 4-row set (Fixes #2073) #2074) sealed-family loader, which is untouched, so its "all roles or none" checks stay attached to the launches they guard.
• From npu: the plan's decode directories reach the target again (fix onto #1984, refs #803) #2038: Plan::resolved_decode_dirs(), called before the target is configured. On main today the plan's decode directories are filled after Setup is built, so the lm-head lookup never sees them; with the loop on the target the decode family would not either.

Not moved here: the wide/narrow loader, and so the forward's method is not deleted yet. That is the remaining A2b step.

Proof (strixhalo, branch 0a8364e3 vs its base 98cf0a06, same build dir and options, one npu-hold ticket per arm)

Row 1: bonsai2-27b-NPU (rm family, plan with 2 decode directories), scripts/npu-window11.sh ... --max-tokens 16:

main  text 1 sha256=8086b68737563238337f4fef4bef9f307a4977eec873a4d00522907d8b838fb3 chars=86
move  text 1 sha256=8086b68737563238337f4fef4bef9f307a4977eec873a4d00522907d8b838fb3 chars=86
main  text 2 sha256=19c34eb6a8a6c54fec1d7fc119b813970983e5c2cf3af359da2e2ad7233ee145 chars=9
move  text 2 sha256=19c34eb6a8a6c54fec1d7fc119b813970983e5c2cf3af359da2e2ad7233ee145 chars=9
main  text 3 sha256=f901d0fb73d3d7c88a505b9c620b26d00ff89982f93bf020945739d293501740 chars=84
move  text 3 sha256=f901d0fb73d3d7c88a505b9c620b26d00ff89982f93bf020945739d293501740 chars=84
both: [plan] ... 2 decode dir(s) from the plan
both: 7 x "[forward] <role> M=16 ... kernels-m16/full_i8_..._m16.elf (decode)", same files
both: [forward] decode family: 7 role(s) at M=16, 0 on M=128

Row 2: bonsai2-27b-t2s2 (sealed set) with a wide and a narrow sealed ELF named by NPU_WIDE_SEALED_ELF / NPU_NARROW_SEALED_ELF, three /v1/completions prompts (5, 536 and 7 prompt tokens), served twice per arm: plain, then NPU_BATCH_CHECK=1:

text 1 sha256=9bafad00ddd200069b707e547ff19fae035a80e16616666378ca1c3f6ea3a33a   main plain = main chk = move plain = move chk
text 2 sha256=af1b78f15d5a4b6f149b2ac3494a14e13ac2ebebd2e167b147de6ef63817b2ab   (same four)
text 3 sha256=850d704d6af90f700306107f043ae8a15d34a58a83cc3efe4fcc2b471aaeea0a   (same four)
all four serves:
  [forward] decode family: 8 role(s) at M=16, 1 on M=128
  [forward] wide family: 8 role(s) at M=32 from .../SEALED-T2S-m32-bigw.elf (chunks of 17..32 rows)
  [forward] narrow family: 8 role(s) at M=4 from .../SEALED-T2S-m4.elf (launches of 1..4 rows)

diff of the text lines, the family lines and the eight per-role (decode) lines between the arms is empty. No amdxdna ... ERROR line in the kernel log over the runs.

Host: ctest -R "npu_plan|npu_target|batch_width|npu_forward|xdna": 9/9 pass, including npu_plan's three resolved_decode_dirs checks (from #2038). tools/copyright.py --check rc 0.

Limits

• The full ctest on this build has one failure, npu_dmabuf_import ("invalid program file (code 401)" at the submit step). It exercises the runtime's buffer import and none of the changed code, and my build sets no ONEBIT_NPU_KERNELS. I did not run it on the main arm, so "same on main" is not measured.
• Timing was not compared; the rows are text and log identity only.
• Not exercised on a device: the pb16i refusal paths (missing sealed ELF, a role with no instance, init_elf failure) and a plan whose decode directories differ from the built-in three. The code for them is a copy of the forward's.
• The wide and narrow ELFs in row 2 are another lane's build artefacts, used read-only.

Close #1984 and #2038 when this merges.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 0a8364e

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 803 - Partially compliant Compliant requirements: Extract NpuTarget + NpuModel Xdna target wraps I8Ctx Host fallbacks for optional ops Default flag --npu-target xdna Non-compliant requirements: Gate: identical outputs (window11 + logits KLD 0 on 8 prompts), prof table within 2 %, ctest green Requires further human verification: Gate: identical outputs (window11 + logits KLD 0 on 8 prompts), prof table within 2 %, ctest green 799 - Partially compliant Compliant requirements: One model description (plan.json) T2 ternary chunks NpuTarget per NPU (XDNA, Hexagon) Target architecture with plan.json, npu/plan/, npu/model/, npu/target/, npu/mtp/, npu/oracle/ NpuTarget interface with open, load_block, gemv, optional gdn_step/attn_step/lm_head, caps XDNA target wraps I8Ctx/npu_rt_api path with no new kernels NpuModel owns the math and calls the target Host fallbacks for every optional op Non-compliant requirements: Gate: identical outputs (window11 + logits KLD 0 on 8 prompts), prof table within 2 %, ctest green Requires further human verification: Gate: identical outputs (window11 + logits KLD 0 on 8 prompts), prof table within 2 %, ctest green

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Performance Regression The new decode_elf_exists function in XdnaTarget uses std::fopen and std::ftell to check if a file exists and is not empty. This approach is less efficient than using std::filesystem::exists or access() for simple existence checks, especially in a performance-critical path like ELF loading. This could introduce a regression in startup time or during kernel loading. namespace { // Same test the forward's file_exists() made: the file opens and is not empty. bool decode_elf_exists(const std::string& p) { FILE* f = std::fopen(p.c_str(), "rb"); if (!f) return false; std::fseek(f, 0, SEEK_END); const long sz = std::ftell(f); std::fclose(f); return sz >= 1; } } // namespace Incorrect Path Resolution The resolved_decode_dirs function in Plan struct resolves paths based on the under_model flag, but it does not validate that the resolved paths actually exist or are accessible. If a plan specifies a directory that does not exist, the target will attempt to load kernels from a non-existent path, which could lead to silent failures or runtime errors. std::vector<std::string> resolved_decode_dirs(const std::string& model_dir, const std::string& kernels_dir) const { std::vector<std::string> out; if (!loaded()) return out; for (const DecodeDir& d : decode_dirs) out.push_back((d.under_model ? model_dir : kernels_dir) + "/" + d.path); return out; }

[bong-water-water-bong]

VERDICT at 0a8364e: READY

Reviewed head 0a8364e (refs #803, #799; lands #1984 + #2038 on main). This is the first verdict.

The move carries the messages it claims to. The body says the log lines and refusals are byte-for-byte the forward's, so I compared every string literal the diff removes from q4nx_forward.cpp with every one it adds to xdna_target.cpp: 26 removed, 21 added. The fprintf(stderr, …) / [forward] format strings match exactly (3 = 3). The six literals that did not travel are all accounted for:

• "/kernels-m16" and "/m16": the built-in fallback directories, still in the forward's helper (q4nx_forward.cpp:143).
• "[pb16i-trace]", "improving" and "disabled": text inside comments. The real [pb16i-trace] prints are already in xdna_target.cpp:994 / :1267.
• "/": the path join, now inside Plan::resolved_decode_dirs().

The only new literal is "rb", a file open.

Checked

• git merge-tree onto main: no conflicts.
• npu_plan_test, built from this head: "npu_plan: ok". Mutant: resolved_decode_dirs() with the model and kernel roots swapped fails it, "decode dirs resolve under the model dir or the kernel dir, in the plan's order". I restored the file (git status clean).
• npu/target/xdna/xdna_target.cpp and npu/forward/src/q4nx_forward.cpp at this head compile clean (0 errors) as single-file compiles with the NPU build's flags.
• CI build succeeded (waited for it before posting); CodeQL green.

Not re-run by me: the device rows (identical text sha256 for three prompts on bonsai2-27b-NPU, and the t2s2 sealed run). They are what proves the move end to end.

[bong-water-water-bong]

VERDICT: READY at 0a8364e — with the one thing a "replaces both" PR has to state, because I could not verify it from the diff.

What I could and could not check

#2166 contains #1984: NO        contains #2038: NO        (refs fetched; ancestry checks against a missing ref are vacuous)
#2166 files: q4nx_forward.{h,cpp}, npu/plan/plan.h, xdna_target.{h,cpp}, tests/npu_plan_test.cpp

Byte comparison cannot settle a consolidation of this kind: #1984 is based on old main — that is the whole reason a new PR was opened ("has conflicted with main in q4nx_forward.cpp since 03:01") — so its files necessarily differ from a re-application onto today's tree.

So I checked the symbols the two reviews were about instead, which is the strongest available test when the bases differ:

open_decode_family    #1984: 1 call site     #2166: 5 call sites      (present, and more — main has more callers now)
npu/plan/plan.h       #2038: present         #2166: present
tests/npu_plan_test   #2038: present         #2166: present

That is consistent with a faithful re-application of both. It is not proof, and I would not call it proof.

What the body should say, and it is one sentence

That the re-application was done from the reviewed state of #1984 and #2038, plus one line each on what they contributed (the decode-family move to the target; the plan layer's plan.h + its test). The cost of consolidation is exactly this: a review answer given on the old branch has no way to appear in the new diff, so the only protection is saying where it came from. #2122 had the same shape tonight and the same ask.

Why a new PR was the right call here

"has conflicted with main in q4nx_forward.cpp since 03:01" — q4nx_forward.cpp is the most-contended file in the tree tonight (I have reviewed a dozen PRs touching it), and a branch that has been conflicting for hours will keep conflicting. A re-application onto current main is the honest resolution, provided the two originals are closed with pointers and their review answers are named. That is the difference between consolidating and losing the thread.

Nothing else: six files, the two subjects, and the plan test carried across.

[bong-water-water-bong]

VERDICT: READY at 0a8364e

Mutant-tested the arm, and it fails on exactly the two assertions it was written for — the resolve order and the "one directory outside the built-in places" case:

$ (swap the model/kernels root in resolved_decode_dirs, rebuild, run)
FAIL decode dirs resolve under the model dir or the kernel dir, in the plan's order
FAIL a plan naming one directory outside the built-in places resolves to exactly that directory
FAILED
$ (restored) -> npu_plan: ok

That is the right pair: the first pins which root each plan entry is resolved under, the second pins that a plan naming something outside the built-in three still resolves to
exactly that directory — which is the defect being fixed (engine#1984 review: "nothing resolved them, so a plan naming any place but the built-in three lost its decode twins
without an error"). A swap of the two roots is the cheapest mutant that distinguishes them, and it kills both arms.

The extraction is a move, not a new rule, and the docstring says what stayed behind:

"This is the per-role loop that lived in Q4nxNpuForward: it is ELF resolution and context creation, which a target owns. The container's naming (sealed_elf, and the designs'
elf_name) still arrives from the forward, and so do the wide and narrow sealed families, which the forward loads on top of the contexts this opens."

That boundary is the one I would have asked for: a target owns "where do I find my ELF and how do I create the context", and the forward keeps "what the container calls it and
what else to load". A move that also changed a rule would be unreviewable at this size (+156/−102 across six files); this one states which half moved.

Two details worth naming as right:

• "A role whose ELF is absent keeps its M=128 kernel (a slower path, not a missing one) EXCEPT under pb16i, where the int8 weights are not packed for these geometries and the load
  stops by name." That is the correct asymmetry — a missing twin is a perf outcome on one route and a refusal on the other, because there the kernel cannot be correct.
• "designs16 is mutable: a role with no decode kernel has its M zeroed, which is how the launch path knows to stay on M=128 — the flag lives in the design, not in a parallel
  array." One source of truth instead of two that can disagree, which is the failure shape this file has already been bitten by (#1984's "built in a block that a move took away").

Supersession is stated and is the right call: it lands #1984 (conflicted in q4nx_forward.cpp since 03:01Z) and #2038, and replaces both. Same pattern as #2133 and #2026 tonight: when
a branch has been conflicting long enough that its author has stopped, the fixed content goes onto main rather than the branch being resurrected.

Ran: npu_plan_test (build clean, npu_plan: ok, 118 check() calls in the file), plus the mutant above. Not run: the decode family on a device.

[bong-water-water-bong]

Answering the READY at 0a8364e that asked for the provenance sentence: added to the body under Where it came from (reviewed heads #1984 6579423 and #2038 8b1c99a, what each contributed, and the one deliberate difference). No code change; the head is still 0a8364e. #2038 is closed with a pointer here, and #1984 has one.