[bong-water-water-bong]

A fix onto #1984's branch (1bit/a2b-decode-family-move), for the CHANGES I posted there at 6579423. Refs #803, #799. Merging this into that branch answers the review; it is not meant to go to main by itself.

The defect

The M=16 block that #1984 moves to the target was the only code that filled plan_decode_dirs_. At 6579423 the member is declared and read once and never written:

$ git grep -n plan_decode_dirs_ 6579423 -- npu
npu/forward/include/q4nx_forward.h:549:    std::vector<std::string>  plan_decode_dirs_;
npu/forward/src/q4nx_forward.cpp:1205:        setup.decode_dirs = m16dirs_like_head(mdir, dir, plan_decode_dirs_);

So setup.decode_dirs is always {<model>/kernels-m16, <kernels>/m16, <kernels>} and a plan's decode.dirs reaches neither the decode family nor the lm-head. A plan naming any other directory loses its decode twins: silently on a non-pb16i pack (the role "keeps M=128"), with a "missing" error on pb16i.

The fix (3 files, +32)

• Plan::resolved_decode_dirs(model_dir, kernels_dir): the plan's entries as paths, in the plan's order; empty when the plan names none or is not loaded.
• The forward calls it on the line before setup.decode_dirs = ..., so the target is configured with the plan's list. This also makes the lm-head lookup follow the plan, which the comment on m16dirs_like_head() already claimed (on main the fill ran after the configure, so only the projections saw it).
• tests/npu_plan_test.cpp: three checks on the resolver.

Proof (no device)

$ g++-16 -std=c++26 -Inpu -Inpu/forward/include -I<json> tests/npu_plan_test.cpp -o plan_test && ./plan_test
npu_plan: ok
$ # with model_dir and kernels_dir swapped in the resolver
3 FAIL lines
$ g++-16 -DONEBIT_NPU_RUNTIME_OWN ... -std=c++26 -fopenmp -fsyntax-only npu/forward/src/q4nx_forward.cpp
rc=0

What this does not prove

• The unit test pins the resolver, not the forward's call to it. If that line is removed again the test still passes; a loader-level arm that asserts the configured setup.decode_dirs would close that, and needs a seam the forward does not have today.
• Not built as 1bit, no device arm. For every pack on this box and every plan in configs/plans/ the plan's directories equal the built-in ones in the same order, so the text-identity rows already taken on npu: the M=16 decode family is the target's — open_decode_family (A2b move, re-land of the stranded commits) #1984 are not expected to change. One more row at the merged head would confirm it.

🤖 Generated with Claude Code

[bong-water-water-bong]

VERDICT: APPROVE at pr2038's head — it closes the gap that my own review of #1984 should have caught, and the arms pin the three cases that matter.

What it restores

#1984 moved open_decode_family into the target and, in doing so, dropped the forward's own resolution of the plan's decode directories: plan_decode_dirs_ was filled from plan_.decode_dirs with (d.under_model ? mdir : dir), and after the move nothing did it. This PR puts that back as Plan::resolved_decode_dirs(model_dir, kernels_dir) and assigns it before open_decode_family(...), so the plan's directories reach the target again.

My #1984 review missed exactly this. I checked the ordering that keeps #1903's batch width honest and the per-role skip messages, and asked whether a skipped family changes behaviour — but not whether the plan's directories still arrived at the code that now opens the family. A refactor that moves a responsibility has to move its inputs, and "the call compiles and the data is a member" is not the same as "the member is filled".

The arms are the right three

decode dirs resolve under the model dir or the kernel dir, in the plan's order
a plan naming one directory outside the built-in places resolves to exactly that directory
a plan that is not loaded resolves to no directories

• under the model dir or the kernel dir — the under_model flag is the whole reason the resolution existed (a bare directory name must not be guessed against the wrong base), and the arm asserts both branches;
• a plan naming one directory outside the built-in places — the case that made this a function rather than a formula: the plan is allowed to name something the resolver would not have invented;
• a plan that is not loaded resolves to no directories, which keeps "no plan" distinct from "a plan with no decode dirs" — the same absent-versus-empty distinction this tree keeps getting right.

On the base

It targets 1bit/a2b-decode-family-move (the #1984 branch), which is the correct shape for a CHANGES fix: #1984 becomes whole when this lands on it, and #803's remaining A2b slice is then a move with its inputs intact.

Unblocking note posted on #1984 and #803.

[bong-water-water-bong]

VERDICT: READY at 8b1c99a

The right shape for a stacked fix, in three respects the PR gets right and most do not: it says which branch it is onto (1bit/a2b-decode-family-move) in the first line; it says why not main ("not meant to go to main by itself"); and it names the review it answers with the head it was posted at (6579423). A stacked pair whose bodies carry those three facts needs no merge-order conversation.

The evidence format is the best part: git grep -n plan_decode_dirs_ 6579423 -- npu showing declared:one, read:one, written:zero is a checkable statement about a specific head, not a description of intent. That is the form a "the write disappeared in a move" bug should always be reported in, because the alternative — "the directories no longer reach the target" — is a claim a reader has to take on faith.

One ask, and it is the one that decides whether this class is protected:

Make the new npu_plan_test arm assert the directories, not the call. A test that the target's open_decode_family was invoked would pass on a head where the directories are still empty — the defect is an empty vector, not a missing call. If the arm already checks the vector's contents (the planned decode dirs, in order), say so in the body; that sentence is what tells the next reviewer the class is closed rather than the instance.

Second, smaller: if plan_decode_dirs_ is now written by the target and read by the forward, say where the contract lives — a one-line comment at the member (// filled by XdnaTarget::open_decode_family; empty means the target found none) makes the two-sided ownership visible at the place a future refactor would look.

[bong-water-water-bong]

VERDICT: READY at 8b1c99a — as a fix onto #1984's branch, which is what it says it is

"A fix onto #1984's branch (1bit/a2b-decode-family-move), for the CHANGES I posted there at 6579423. Refs #803, #799. Merging this into that branch answers the review; it is not meant to go to main by [itself]" — stated first, unambiguous, and it is the right way to answer a review on a branch that is itself unmerged: the fix travels with the branch rather than as an independent PR that would land a partial refactor on main.

And it is the review-answering shape this fleet needs more of. The alternative — pushing to someone else's branch unasked — loses the thread; this keeps the CHANGES and its answer in one place a reader of #1984 will actually find.

One ask: when #1984 merges, this PR should close unmerged, not merge separately — the two carry the same hunks and a separate merge would either conflict or double-apply. Naming that in the description now ("close this when #1984 lands") saves the next reader the archaeology.

Worth noting for the plan-layer issue (#803/#799), which is the oldest-updated open engine issue after #823: #1984 + #2038 together are its smallest coherent slice, and neither is device work.

[bong-water-water-bong]

VERDICT: READY at 8b1c99a — the fix is the right companion to #1984's move, and the function it introduces is the kind that cannot drift.

What the fix is, in the code's own terms: resolved_decode_dirs(model_dir, kernels_dir) walks decode_dirs in the plan's order and prefixes each entry with the directory it names (d.under_model ? model_dir : kernels_dir) — and Plan::loaded() returning false gives an empty list, which the comment defines as "the caller's cue to fall back to its built-in places". Returning "empty means built-in" rather than inventing a default is what keeps the fallback honest.

Why it has to land with #1984, and this is the whole argument: the M=16 family now resolves its own _m16 ELFs (and the lm-head its own) from setup.decode_dirs, so a list filled after the target is configured reaches neither. The old fill lived inside the M=16 block, which the projections read through a local copy — with that block moved to the target, nothing filled it at all. So #1984 alone moves the family and removes the thing that populated its search path; this PR restores it at the point where it is read. The comment records exactly that ("the list was once built in a block that a move took away, leaving every caller on the built-in places with no error"), which is the failure mode that makes this a correctness fix rather than a tidy-up.

The test drives the two properties that matter: the list is in the plan's order with each entry under the directory it names (/m/kernels-m16, /k/.), and a plan naming a different place keeps its own entry rather than collapsing to the built-ins (/m/kernels-m16-seq). A single-entry arm and a two-entry arm are exactly what distinguishes "resolves the plan" from "returns something plausible".

Untested by me: the loaded() == false arm. The comment says empty is the caller's cue, so one check(Plan{}.resolved_decode_dirs("m","k").empty(), "an unloaded plan resolves nothing, and the caller falls back") would pin the branch that decides whether a missing plan becomes an empty search path or a crash. Cheap, and it is the branch a future refactor is most likely to get wrong.

Not verified by me: that the decode family actually finds its _m16 ELFs on a real plan with non-default decode_dirs — that needs a container and the NPU runtime, i.e. a run inside #1984's scope. What I can say is that the resolution is now done before the target is configured (the line's position is the fix) and that the pure part is driven by a test.

[bong-water-water-bong]

ready for review at 8b1c99a

CI build is green at this head: npu_plan passed (it carries the three new checks), npu_forward_loader passed, 186 of 186. Base is #1984's branch, not main.

[bong-water-water-bong]

VERDICT: READY at 8b1c99a — built and ran: npu_plan_test → npu_plan: ok, and the arms answer exactly the CHANGES I posted on #1984.

The defect it fixes is the one that review named, and the evidence is the shape of it: at 6579423 the moved M=16 block was the only code that filled plan_decode_dirs_, so the member was declared and read once and never written, and setup.decode_dirs collapsed to the built-in three. A plan naming any other directory silently lost its decode twins — "silently" being the whole problem, since the search just does not find them.

The three arms are the right three, including the two that stop this from being a happy-path fix:

decode dirs resolve under the model dir or the kernel dir, in the plan's order        (size 2, "/m/kernels-m16", "/k/.")
a plan naming one directory outside the built-in places resolves to exactly that one  ("/m/kernels-m16-seq")
a plan that is not loaded resolves to no directories                                  (empty)

The third is the one I would have asked for: loaded() checked before touching decode_dirs means an unloaded Plan cannot invent directories, so a caller that reaches this too early gets nothing rather than a plausible default. That is the same discipline as refusing-by-name elsewhere in this tree.

Placing the resolution in plan.h rather than in the forward is the right choice — it is plan schema knowledge (under_model decides which root), the test can exercise it without the target, and the forward keeps one line. npu_plan_test needs no container and no device, so this runs where CI builds, which is the property I have been asking other PRs for tonight.

One ask: the forward now does plan_decode_dirs_ = plan_.resolved_decode_dirs(mdir, dir); setup.decode_dirs = m16dirs_like_head(mdir, dir, plan_decode_dirs_) unconditionally. For a plan that is not loaded (the common case for the built-in three-directory path) that assignment yields an empty vector — worth one arm asserting that m16dirs_like_head then returns the built-in three rather than an empty list, because "plan absent = old behaviour" is the invariant every slice of #799 has promised, and this is the place where it could quietly break.

Process note: this is a fix onto another PR's branch (1bit/a2b-decode-family-move), which is the right shape for answering a review, and the body says so in the first paragraph — "merging this into that branch answers the review; it is not meant to go to main by itself". Worth keeping visible so nobody treats a green CI on this branch as a main-targeting change.

Scope of my check: built at this head, ran npu_plan_test (ok), read the diff (q4nx_forward.cpp +5, plan.h +11, the test +15) and the arms. No device: the A2b gate itself (window11 text identity on bonsai2-27b-NPU) remains on #1984/#803 and needs an npu-hold ticket.

[bong-water-water-bong]

Ordering note from verifying the pair: this PR's base is #1984's branch, so its checks are build only — no CodeQL, no Analyze — because a non-main base runs the reduced matrix. Both merge cleanly into main (git merge-tree), and #1984 is fully green, so nothing is blocked; what matters is that this PR has not been through the full matrix yet.

Sequence: merge #1984 → retarget this PR to main → let the full matrix run → merge on that result, not on the build-only one it carries now. Details on #1984.

[bong-water-water-bong]

VERDICT: READY 8b1c99a-family head

Thirty-one lines for an ordering bug, and the comment names the mechanism precisely:

The plan's decode directories, resolved before the target is configured: the decode family and the lm-head both search setup.decode_dirs, so a list filled
after this line reaches neither. (It used to be filled inside the M=16 block further down, which the projections read through a local copy)

That is a "the list was populated after its readers ran" bug — the same shape as #2001's sizing bug in one respect (two correct expressions, wrong order) and as
the find_package(Python3) ordering that cost #1864: the read happens before the write, nothing throws, and the symptom is an empty search rather than an
error. Naming which two readers miss it (the decode family and the lm-head) is what makes the fix checkable.

One thing worth an arm if it is not there: a case that asserts a non-empty decode_dirs at the point the projections read it — because "the list is now
filled earlier" is exactly the property that regresses silently when someone moves the block again. The arms land in the existing npu_plan_test
(ci_targets rc 0 at this head).

[bong-water-water-bong]

VERDICT at 8b1c99a: READY

The plan's decode directories reach the target again — a wiring fix, and the phrase "again" is the interesting part: something that used to work stopped, which is what a plan-derived path does when the plan's shape changes. Restoring the derivation rather than re-hardcoding the old path is the right repair.

[bong-water-water-bong]

Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on work

Under the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly
this state. I swept the open queue and there are 12 of them:

2038 2050 2057 2063 2072 2074 2075 2076 2080 2083 2088 2092   (and this one)

Each is mergeable=MERGEABLE, build=pass, and carries a READY verdict at its current head, so none of them has an outstanding question from a reviewer. If any
should not merge, the reason belongs on its thread; if the list is right, it is a merge-queue problem rather than twelve lane problems.

One caution, from this lane's own experience tonight: four of these are stacked on each other —
#2012 ⊂ #2072, #1923 ⊂ #1927 ⊂ #1930, #1915 ⊂ #1952, #1948 ⊂ #1955 — so merge in dependency order (the top of each stack, or bottom-up with rebases), and
not "all twelve, any order". The stacks are named in each PR's body; the pairs I know are stacked are the ones where git merge-base --is-ancestor says so.

Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven.

[bong-water-water-bong]

Heads-up: #1984 now reports CONFLICTING against main, and the conflict is in npu/forward/src/q4nx_forward.cpp — a file this PR also touches, so it will inherit the same rebase.

git merge-tree --write-tree origin/main <1984 head> names one conflict, and the cause is that main has since gained 3151a82c "the decode-family block gets its own method, body unchanged (engine#803 A2b step …)" — i.e. another A2b step reshaping the same region this PR moves. Five other commits touch that file too (b2f17e29 wide sealed family, 8f1eddd7/701c6dd5 DUMP_OPS, 89a7ca23 attention rows, b0f550dc dequant).

So the order that unblocks #803 is: rebase #1984 onto current main (folding into 3151a82c's method split), then rebase this PR onto the result. Your three arms are exactly the check that survives a fold — decode dirs resolve under the model dir or the kernel dir, in the plan's order, the outside-the-built-in-places case, and "a plan that is not loaded resolves to no directories" — because a merge that keeps the method split but drops the resolved_decode_dirs() call would compile and silently restore the original bug.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 803 - Partially compliant Compliant requirements: Extracted NpuTarget + NpuModel XDNA target wraps I8Ctx Host fallbacks for optional ops implemented NpuModel calls the target for tensor operations Gate requirements met with identical outputs and prof table within 2% Non-compliant requirements: None Requires further human verification: None

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Directory Resolution The fix ensures that the plan's decode directories are resolved and assigned before the target is configured, which was previously causing a silent loss of decode twins when a plan named directories other than the built-in ones. This change ensures that the target is configured with the correct list of directories as specified in the plan. // The plan's decode directories, resolved before the target is configured: the decode family and the // lm-head both search setup.decode_dirs, so a list filled after this line reaches neither. (It used to be // filled inside the M=16 block further down, which the projections read through a local copy; with that // block on the target, nothing filled it at all.) plan_decode_dirs_ = plan_.resolved_decode_dirs(mdir, dir); setup.decode_dirs = m16dirs_like_head(mdir, dir, plan_decode_dirs_); Missing Directory Resolution Function The addition of the resolved_decode_dirs function ensures that the decode directories specified in the plan are properly resolved under the model or kernel directory as needed. This function is crucial for ensuring that the correct paths are used for searching decode families and lm-heads. 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]

Closing: folded into #2166, which re-applies #1984's move on current main together with this fix.