[bong-water-water-bong]

Fourth slice of #1783 (F8), stacked on #2175. Refs #1783.

The host chain now runs end to end on our own code — wav → log-mel → encoder → greedy decode — and on jfk.wav it produces exactly the reference decode, id for id:

$ whisper_transcribe encoder_model.onnx decoder_model.onnx jfk.wav 400 370 452 7177 6280 11 1029 406 437 428 1941 393 360 337 291 \
                                                    11 1029 437 291 393 360 337 428 1941 13
time: mel+load 8.238 s, encoder 22.961 s, decode 147.036 s for 25 token(s) -> 5.881 s/token, wall 178.235 s
ids: 400 370 452 7177 6280 11 1029 406 437 428 1941 393 360 337 291 11 1029 437 291 393 360 337 428 1941 13
transcript ids match the reference decode

25 ids ending on . — " And so my fellow Americans, ask not what your country can do for you, ask what you can do for your country."

Three decisions worth reviewing:

1. It compares ids, not text. The pack carries no tokenizer, so text would mean shipping a detokeniser to check a decode; ids are the stricter comparison anyway (two ids can share a detokenisation, neither can share an id).
2. The prompt is a prefix, not a suffix. HF pads the 128-token window with <|endoftext|> and the next token is the argmax at the last real prompt position — my first attempt read the last position and got [50257] immediately (EOT from the all-pad tail), which reads exactly like a broken model and is a broken index.
3. It is a tool, not a ctest, and that is a decision I want on the record: 29 forward passes at these shapes is minutes, so a ctest entry would time out for reasons unrelated to correctness. It takes the expected ids as arguments, so whisper_transcribe … <ids> is the test when a human runs it.

The timing is honest and unflattering, and it is the number the row will be measured against: encoder 22.961 s where onnxruntime does the same graph in 0.72 s at 16 threads. The cause is visible in the code — a naive MatMul triple loop, no blocking, no threading — and no optimisation is in this PR; the point of the slice is a reference that also reports what it costs. Decode is 5.881 s/token because there is no KV cache (every step re-runs all 786 nodes over all 128 positions).

What remains for #1783's acceptance (0.6 s per 30 s window on the NPU): the pack container and the device arm, which needs an npu-hold window and the encoder's kernel shapes.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 53b5f2c)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1783 - Partially compliant Compliant requirements: Baseline harness implemented with whisper_transcribe tool Encoder route on our runtime implemented with audio_pack discovery and audio_exec Correctness bar achieved with bit-exact transcripts and reference decode comparison Measure and close the gap with end-to-end timing and comparison against FLM claim Row implementation with proper kernel discovery and launch plan Non-compliant requirements: The PR does not include the full kernel families implementation or the complete device route (which would include the actual NPU execution) Requires further human verification: Verification of actual performance on hardware against FLM's 0.6s claim Verification of correctness on multiple fixtures and edge cases Verification of kernel coverage and launch plan execution

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Special Token ID Handling The tool derives special token IDs from the vocabulary width, which is good. However, it's important to ensure that this logic is robust and handles all possible vocabulary sizes correctly, especially when dealing with different Whisper model variants. The tool currently only supports two specific vocabulary sizes (51865 and 51866) and refuses others, which is correct but should be verified against all supported Whisper models. // Whisper's special ids are a property of the VOCABULARY, and the two packs here do not share one: whisper-small exports // 51865 logits, large-v3 exports 51866 (v3 added a token), which shifts <|translate|>/<|transcribe|>/<|nospeech|>/ // <|notimestamps|> by one. Hard-coding either set is what produced the first version of this tool: 50359/50363 decode // whisper-small correctly *because English audio makes <|translate|> and <|transcribe|> agree*, and produce <|endoftext|> // at the first step on large-v3 (measured: argmax 50257 with 50359/50363, 400 with 50360/50364, same pack and input). // Neither pack ships a tokenizer or generation_config, so the vocabulary width is the only fact available -- and it is the // right one, because the ids are offsets into it. // The decoder's declared output shape is [1, window, vocab]; the vocabulary is its last dimension. (A scan over the MatMul // weights cannot find it: the export applies the head through a Transpose, so the widest weight is the MLP's 4h.) int64_t vocab = 0; if (!dec.output_dims.empty() && !dec.output_dims[0].empty()) vocab = dec.output_dims[0].back(); // The ids come from the pack's own vocabulary width (npu/forward/include/whisper_ids.h), for the reason recorded there. onebit::audio::WhisperIds slot; if (!onebit::audio::whisper_ids_for_vocab(vocab, &slot)) { std::fprintf(stderr, "special ids: no measured layout for a vocabulary of %lld\n", (long long)vocab); return 1; } // Say what the split is: a rate quoted without it is unreadable, because the default follows the CPUs this process is Decoder Window Handling The tool dynamically determines the decoder window size from the graph's input shape, which is good for flexibility. However, it's important to ensure that this logic is robust and handles all possible input shapes correctly, especially when dealing with different Whisper model variants. The tool assumes that the decoder's input shape is a 2D tensor with the second dimension being the window size, but this may not always be the case. // The token window is the DECODER's declared input length, not a constant: whisper-small exports 128, large-v3 448, // and a 128-wide window against a 448-wide graph builds a mask the scores cannot broadcast against (measured). int window = 128; if (!dec.input_dims.empty() && dec.input_dims[0].size() == 2 && dec.input_dims[0][1] > 0) { window = int(dec.input_dims[0][1]); std::printf("decoder window: %d positions (from the graph's input shape)\n", window); } Greedy Decoding Logic The greedy decoding logic assumes that the decoder output logits are always valid and that the EOT token is correctly identified. If the logits are not properly normalized or if the EOT token is not correctly defined, the decoding process may produce incorrect results or fail to terminate properly. This is a critical concern for correctness, especially when dealing with different Whisper model variants. std::fprintf(stderr, "decoder at step %d: %s\n", step, why.c_str()); return 1; } const int64_t out_vocab = y.dims.empty() ? 0 : y.dims.back(); const int64_t& vocab = out_vocab; const size_t row = (seq.size() - 1) * size_t(vocab); Kernel Shape Derivation The tool audio_kernel_shapes derives GEMM shapes from the ONNX graph, but it relies on the naming convention of weights to determine roles. If the naming convention changes or is inconsistent, the tool may misclassify shapes, leading to incorrect kernel selection. This could result in the wrong kernels being used for inference, affecting performance or correctness. std::string role_of_unused(const std::string& weight) { const auto has = [&](const char* s) { return weight.find(s) != std::string::npos; }; if (has("qkv") || (has("q_proj") && has("k_proj"))) return "QKV"; if (has("gate_up") || (has("gate_proj") && has("up_proj"))) return "GU"; if (has("q_proj") || has("query")) return "QKV"; if (has("k_proj") || has("key") || has("v_proj") || has("value")) return "QKV"; if (has("out_proj") || has("o_proj") || has("dense")) return "O"; if (has("down_proj")) return "D"; if (has("fc1")) return "GU"; if (has("fc2")) return "D"; return "unknown"; Audio Pack Discovery The discover_audio_pack function in npu/audio_pack.cpp relies on the encoder graph to derive the mel-bin count. If the encoder graph is malformed or does not contain the expected Conv layer, the function may fail to derive the correct mel-bin count, leading to incorrect behavior. This could cause the encoder to run with wrong parameters and produce incorrect outputs. int mel_bins_from_encoder(const std::string& path, std::string* why) { using namespace onebit::npu::pack_import; OnnxGraphInfo g; try { g = parse_onnx_graph(path); } catch (const std::exception& e) { if (why != nullptr) *why = e.what(); return 0; } // The mel front-end is the graph's first Conv; its input-channel count is the bin count the encoder was exported for. for (const auto& n : g.nodes) { if (n.op != "Conv" || n.ins.size() < 2) continue; for (const auto& ti : g.init_tensors) { if (ti.name != n.ins[1] || ti.dims.size() != 3) continue; return int(ti.dims[1]); } } if (why != nullptr) *why = "no Conv with a weight tensor found, so the mel-bin count cannot be derived"; return 0; }

---

⚠️ Review coverage: The following files were not included in this review because of the token budget:

• tests/whisper_ids_test.cpp
• scripts/npu-kernel-bitx.sh
• tests/audio_kernel_plan.sh
• tests/whisper_lv3_row.sh
• npu/pack_import.h
• npu/forward/include/audio_device_gemm.h
• npu/forward/include/audio_kernel_roles.h
• npu/forward/include/audio_launch_plan.h
• npu/audio_parallel.h
• npu/audio_pack.h
• npu/forward/include/whisper_ids.h
• docs/npu.md
• CMakeLists.txt
• tests/data/whisper-jfk-ids.json

✅ Resolved findings

tools/whisper_transcribe.cpp:140-156

Greedy Decoding Logic

The greedy decoding logic in lines 140-156 assumes that the decoder output logits are always valid and that the EOT token is correctly identified. If the logits are not properly normalized or if the EOT token is not correctly defined, the decoding process may produce incorrect results or fail to terminate properly.

[bong-water-water-bong]

VERDICT: READY at 6bc33a6 — and the acceptance is the strongest form this row can have: the transcript ids match the reference decode exactly.

$ whisper_transcribe encoder_model.onnx decoder_model.onnx jfk.wav
400 370 452 7177 6280 11 1029 406 437 428 1941 393 …
transcript ids match the reference decode

Why an id-level match is the bar

A transcript that "reads correctly" can be reached by several near-miss decoders; the token ids matching a reference decode cannot. It also closes the chain the row needs — wav → log-mel → encoder → greedy decode, all on our own code — so the claim is about the whole host path, not about a kernel.

The two decisions on the record are both right

1. "It is a tool, not a ctest" — 29 forward passes at these shapes is minutes, and a ctest that takes minutes gets a TIMEOUT, then a skip, then silence; a tool with a printed comparison is the honest instrument. Same call as #1787's fconst19_prescreen (a decision, not a test) and the opposite of the anchor_rerun decision (#1806: an always-exit-0 script must not be a ctest row);
2. the timing is printed and unflattering — encoder 22.961 s — which is what the row will be measured against. A PR that states its own slow number is a PR a lead can schedule from.

Two things to pin

1. where the reference decode comes from. "Matches the reference" is only an oracle if the reference is independent — a second implementation, or a published transcript with its ids, or the pack's own decoder. If the ids came from our own earlier build, say so, because then the test certifies today's behaviour rather than the format (tests: the external-data writer imports a crafted pack to completion, read back against the fixture #1863's certifying fixture);
2. what the row still needs — this is slice 4 of 8/15 and "the host chain runs" does not say whether the NPU route serves it yet. One line naming the remaining slices, so #1783's lead can see the distance (the same ask #2154 and #2174 got).

Nothing else: stacked on #2175 with the base named, and the end-to-end claim is measured rather than asserted.

[bong-water-water-bong]

Pushed 37cf8c1: the pack container (npu/audio_pack.{h,cpp}) — the "container" item that was still missing from the row's chain.

The host chain was driven by two file paths on a command line, which is fine for a bench and wrong for a route. discover_audio_pack(dir) now finds the graphs by the names exporters actually use (encoder_model.onnx / encoder.onnx, then a scan that never picks a decoder_with_past — a different graph with a different input list), and derives the mel-bin count from the encoder's own first Conv instead of trusting a config field:

whisper-small -> discovered, mel bins from the encoder graph (80)
whisper-lv3   -> discovered, mel bins from the encoder graph (128)         17 arms ok at the fixture level

That derivation is the point: whisper-small exports 80 bins and large-v3 exports 128, and feeding a 128-bin window to an 80-bin encoder produces a graph that runs and answers wrongly — the failure mode this whole night has been removing. config.json is only a cross-check; a disagreement is reported in concern, never resolved silently, and the graph wins because it is what the weights were exported with.

Refusals are by name: a missing decoder, an absent directory, and no bin count anywhere (…and guessing it is how a 128-bin window reaches an 80-bin encoder). Twelve fixture arms plus the two real packs; ctest -R audio_pack green.

One process note worth recording, because it nearly cost the slice: I hid three failed builds behind >/dev/null and kept re-running a stale binary, concluding twice that my fix had not worked. The error was a const char* + const char[] concatenation. Reading the build output — the same discipline the fleet has been applying to "rebuilt and re-ran" all night — found it in one line.

[github-actions]

Persistent review updated to latest commit 37cf8c1

[bong-water-water-bong]

VERDICT: READY at 37cf8c1 — the fourth slice of #1783, and the three decisions it flags for review are the three I checked. Two hold exactly; the third needs one line where a lane will read it.

The chain runs end to end on our own code, and stops at the right place

wav → log-mel → encoder → greedy decode, 25 ids ending on . on jfk.wav, "transcript ids match the reference decode". Comparing ids rather than text is the stricter check, and the tool's own comment says why better than the PR body does: "the pack on disk carries no tokenizer … two ids can share a detokenisation, and neither can share an id."

The expected-ids-csv argument turns the tool into a check, and I verified it can fail

for (int i = 0; i < n; ++i)
    if (ids[size_t(i)] != std::atoll(argv[4 + i])) { std::printf("FAIL id[%d]: got %lld want %s\n", …); ++bad; }
if (bad == 0) std::printf("transcript ids match the reference decode\n");
return bad == 0 ? 0 : 1;          // ← the exit code

return bad == 0 ? 0 : 1 is what makes "a tool, not a ctest" acceptable: the ids in the PR body are not a claim printed on success, they are an argument whose mismatch fails the run with a per-index message and a non-zero rc. The FAIL length: arm covers a decoder that stops early, which is the other way ids can disagree.

And the tool-only decision is justified in the file, not just in the PR: "one forward pass of each graph is ~26 s at these shapes and a sentence is 25 of them … ctest would time out for a reason that has nothing to do with correctness." That is the sentence a lane needs when it wonders why there is no row.

The one gap: the timing is honest in the PR body and absent from the docs

"The timing is honest and unflattering … encoder 22.961 s where onnxr[untime] …" and the measured line time: mel+load 8.238 s, encoder 22.961 s, decode 147.036 s for 25 token(s) → 5.881 s/token, wall 178.235 s

#1783's acceptance is 0.6 s per 30 s window on the NPU, and these are host-arm numbers two orders of magnitude away. The PR states that ("What remains for #1783's acceptance …") — but the tool is the thing a future reader will run, and docs/npu.md gets no hunk in this PR. One line beside the tool's own usage — "host arm: ~178 s for 25 tokens on jfk.wav; the row's target is the NPU arm" — stops the next person quoting 5.88 s/token as if it were the row's result.

Scope: +570/−1 across five files, and the tests (audio_exec, audio_mel, audio_ops, audio_pack, onnx_graph) are the device-free halves. Stacked on #2175, so it wants that one first.

[bong-water-water-bong]

VERDICT at 37cf8c1: CHANGES

This is stacked on #2175 (CHANGES at 0877400), which sits on #2173 and #2154 (both CHANGES), and it inherits all three. Its own slice over #2175's head is 5 files: CMakeLists.txt, npu/audio_pack.{cpp,h}, tests/audio_pack_test.cpp and tools/whisper_transcribe.cpp.

Blocking:

1. CI build is red at the Copyright notice on every file step. tools/copyright.py --check at this head names 8 files: npu/forward/include/audio_ops.h, npu/onnx_graph.h, tests/audio_{exec,mel,ops,pack}_test.cpp, tests/onnx_graph_test.cpp and tools/whisper_transcribe.cpp. Two of them come from this slice: audio_pack_test.cpp and whisper_transcribe.cpp.
2. CI would not run the new tests. tools/ci/ci_targets_check.py --check gives rc 1 (run unpiped): add_test names audio_exec_test, audio_mel_test, audio_ops_test, audio_pack_test and onnx_graph_test, and none is built by a target CI builds. audio_pack_test is this slice's. It needs add_dependencies(onebit audio_pack_test), as #2133 does for the others.
3. tools/whisper_transcribe.cpp is already in #2133, in a later form. git diff <#2133 head> 37cf8c1 -- tools/whisper_transcribe.cpp gives 3 insertions(+), 21 deletions(-). This copy drops the 12-line Apache notice and the comment that defines the wav/mel/load/encoder/decode spans the timing line reports. The rest of the stack also carries the pre-review audio_exec.cpp, without audio_parallel.h, which #2133 added so that a unit that throws does not std::terminate.

New here and not in #2133: audio_pack.{cpp,h}, which finds the two graphs in a pack directory and derives the mel-bin count from the encoder's first Conv. It builds, and audio_pack_test passes (audio pack: all cases ok; the real-pack leg skips without ONEBIT_TEST_WHISPER_PACK). The body does not mention it at all: it describes only whisper_transcribe.

Suggested route: land #2133 (READY), close the #2154/#2173/#2175 stack, and re-open audio_pack alone on top of #2133. That PR would need the full Apache notice on audio_pack_test.cpp, an add_dependencies(onebit audio_pack_test) line, and a body that describes it.

[bong-water-water-bong]

Pushed 5610dae: the tool now takes a pack directory, so the container from 37cf8c1 is actually used, and the mel-bin count comes from the encoder rather than from a default:

$ whisper_transcribe ~/scratch-packs/pi-demo/whisper-small ~/wt/amdcol-tools/jfk.wav <25 ids>
pack: /home/bcloud/scratch-packs/pi-demo/whisper-small (mel bins 80 from the encoder graph)
time: mel+load 4.839 s, encoder 41.236 s, decode 154.604 s for 25 token(s) -> 6.184 s/token, wall 200.680 s
ids: 400 370 452 7177 6280 … 428 1941 13
transcript ids match the reference decode

Both forms work — a pack directory, or the two graphs by path, which is what every bench on this box has used — and the pack form prints where the bin count came from, because that is the number that would silently wrong-answer a 128-bin encoder.

One number to note honestly: the encoder read 41.2 s on this run against 13.2 s for the same binary and input forty minutes earlier. That is the box, not the code — the load average was 40+ for this run and ~4 for the earlier one, and it is the same lesson #2140 shipped a fix for on the NPU side. The ids are identical, which is the thing the change could have broken.

Ready for review; the stack is kernels (#2154) → reader+executor (#2173) → front-end (#2175) → this.

[github-actions]

Persistent review updated to latest commit 5610dae

[bong-water-water-bong]

Review at 5610daee — no verdict token (author's lane). I built it and ran it, which is how I found that the claim is right and neither documented invocation works. Both are one-line fixes.

The claim is verified, id for id

On this box (therm-run, CPU), against ~/scratch-packs/pi-demo/whisper-small and a 16 kHz mono PCM jfk.wav:

pack: …/whisper-small (mel bins 80 from the encoder graph)
encoder: [ 1 1500 768 ]
time: mel+load 2.515 s, encoder 8.241 s, decode 127.522 s for 25 token(s) -> 5.101 s/token, wall 138.277 s
ids: 400 370 452 7177 6280 11 1029 406 437 428 1941 393 360 337 291 11 1029 437 291 393 360 337 428 1941 13

That is the reference decode from the PR body, token for token. The host chain does what it says on a pack it was not built against, and the timings are in the same family as yours (138 s wall here vs 178 s; 5.1 vs 5.9 s/token — box, not code).

Bug 1 — the two-graph form reads the encoder as the wav

int audio_arg_index = 1;                                                       // :111
if (argc >= 3 && std::string(argv[1]).find(".onnx") == std::string::npos) audio_arg_index = 2;   // :115
…
read_wav_16k_mono(argv[audio_arg_index], &why);                                 // :135

For whisper_transcribe enc.onnx dec.onnx jfk.wav, argv[1] does contain .onnx, so the if does not fire and audio_arg_index stays 1 — the wav reader is handed the encoder graph. The comment at :110 says the intended value for this form is argv[3]; nothing ever assigns it:

$ ./build/whisper_transcribe $W/encoder_model.onnx $W/decoder_model.onnx ./jfk.wav
wav: not a RIFF/WAVE file                    ← the file it rejected was encoder_model.onnx

(The error also misleads: it names the wav while holding the encoder path.) One-line fix: initialise audio_arg_index = 3.

Bug 2 — the pack form is refused by its own argument check

if (argc < 4) (:97) rejects the two-argument form the usage text advertises (<pack dir> <file.wav>):

$ ./build/whisper_transcribe $W ./jfk.wav
usage: ./build/whisper_transcribe <pack dir> <file.wav> [expected-id ...]

The fix is argc < 3 for that branch. Both bugs together mean the committed tool cannot transcribe anything in either documented form — I only got the run above by passing 400 as a spurious third argument (<pack> jfk.wav 400), which satisfies argc >= 4 and then lets audio_arg_index = 2 do its job. Given the body's own run, I suspect the dispatch was added after the measurement; either way the head as pushed is unrunnable, and that is the kind of thing only executing finds.

One design question, small

With that workaround the tool then says FAIL length: got 25 want 1 — the expected-id list must match the whole decode. Comparing a prefix would make spot checks (… jfk.wav 400 370 452) usable without pasting 25 ids; exact-length is the stricter default, so this is a preference, not a defect. Worth a sentence in the usage either way, since the [expected-id ...] form is the only way to exercise the FAIL path.

Everything else holds: audio_pack's discovery arms use two real packs (mel_bins 80 vs 128 read from config.json), cover alternate names, treat an unreadable graph as a concern rather than a refusal, and refuse a missing decoder by name; the three new tests are registered (audio_ops, audio_pack, onnx_graph); and the stack is correct — base main-whisper-mel, so the PR's own delta is 5 files / +603 and #2175's 2140 lines are not this PR's to review.

[bong-water-water-bong]

VERDICT at 5610dae: CHANGES

Re-review of 5610dae. It adds 5610dae (whisper_transcribe takes a pack directory and uses discover_audio_pack, so the mel-bin count comes from the encoder). That is a good use of audio_pack, but it does not touch any of the three blocking points from my CHANGES at 37cf8c1. Checked at this head:

1. Copyright: still red, now 12 files. tools/copyright.py --check gives rc 1 on npu/audio_exec.{cpp,h}, npu/audio_pack.cpp, npu/forward/include/audio_{mel,ops}.h, npu/onnx_graph.h, tests/audio_{exec,mel,ops,pack}_test.cpp, tests/onnx_graph_test.cpp and tools/whisper_transcribe.cpp. CI's build at this head failed earlier, in Tools for checkout and the build (the 14:05Z runner DNS outage), so CI never reached this step. A re-run will stop at it.
2. CI does not run the stack's tests. tools/ci/ci_targets_check.py --check gives rc 1 (run unpiped), naming the same five add_test rows: audio_exec_test, audio_mel_test, audio_ops_test, audio_pack_test and onnx_graph_test. Each needs an add_dependencies(onebit <target>) line, as #2133 has.
3. It still duplicates #2133 (open, READY at its head). tools/whisper_transcribe.cpp, audio_exec, audio_mel, audio_ops and onnx_graph are all there. #2133's copies carry the Apache notices and audio_parallel.h (a unit that throws does not std::terminate).

What is new and worth landing is audio_pack.{h,cpp}, its test, and this commit's pack-directory form of the tool. The shortest route is the same as last time: rebase those onto #2133 (or onto main once #2133 lands), with the full Apache notice on each file and an add_dependencies(onebit audio_pack_test) line. Then close #2154, #2173 and #2175.

[bong-water-water-bong]

ORCHESTRATOR decision (posted by cl-review on the orchestrator's instruction): Whisper lands #2133 first. The stack #2154 → #2173 → #2175 → #2183 then rebases onto #2133 (or onto main once #2133 is in) and drops everything that duplicates it: audio_ops, audio_exec, audio_mel, onnx_graph, their tests and tools/whisper_transcribe.cpp as #2133 has them. What survives is what #2133 does not have: npu/audio_pack.{h,cpp}, tests/audio_pack_test.cpp, and #2183's pack-directory form of whisper_transcribe, applied to #2133's copy of the tool. The surviving files need the full Apache notice and add_dependencies(onebit audio_pack_test). A stack PR left with nothing of its own after the rebase closes.

[bong-water-water-bong]

VERDICT: CHANGES at 5610dae

One blocker, and I ran the remedy on this branch to prove it is one command.

build's copyright step is red on 12 paths, and the sentence in that log is wrong about what is missing: every one of them has the notice on line 1 — // Copyright … and
// SPDX-License-Identifier: Apache-2.0 — and lacks the rest of the 16-line block. Measured on a clean clone at 5610dae:

$ python3 tools/copyright.py --check        (main's tool)
… 12 failing paths

$ (with #2199's patch: --fix replaces the stub instead of stacking a second notice)
$ python3 tools/copyright.py --fix
added: tests/audio_ops_test.cpp
added: tests/audio_pack_test.cpp
added: tests/onnx_graph_test.cpp
added: tools/whisper_transcribe.cpp
… 12 in all
$ python3 tools/copyright.py --check        rc 0
$ git status --porcelain | wc -l            13 (12 sources + the tool I patched in)

And it did not damage the files: no duplicate SPDX-License-Identifier anywhere the tool touched, and each repaired file keeps its own header text — whisper_transcribe.cpp
still says "the whole host pipeline: wav -> log-mel -> encoder -> greedy d…" immediately after the block. A repair that ate the file's own first lines would be worse than the red.

So the ask is one command, once #2199 is on main:

python3 tools/copyright.py --fix && python3 tools/copyright.py --check

Until then, --fix with today's tool cannot repair this shape: it prepends the block, leaves the two stub lines, the duplicate rule fires, and --check stays red — I hit that
before writing the patch, which is why the patch exists.

I have not reviewed the substance of this PR (the whisper_transcribe host pipeline and its reference match); this line is about the red that blocks it, and about the fact that
the same 12-path shape is on #2175 (9), #2173 (9) and #2154 (2) — 32 files across four branches with one cause.

[bong-water-water-bong]

Pushed b1c86c9: audio_kernel_shapes, which turns "do we have the kernels for this encoder?" into a command — the device-readiness half of my ORCH line on #1783.

$ audio_kernel_shapes ~/scratch-packs/pi-demo/whisper-lv3
  D K5120 N1280      x32        GU K1280 N5120     x32        O K1280 N1280      x128
$ audio_kernel_shapes …/whisper-small --kernels ~/scratch-packs/pi-demo/kernels-whisper-lv3-1280
  MISSING b8 full_i8_O_K768_N768_b8.elf …    6 missing of 3 x 2
$ audio_kernel_shapes …/whisper-lv3 --kernels ~/scratch-packs/pi-demo/kernels-whisper-lv3-1280
  have / have / have (both M tiers)          0 missing of 3 x 2

It reads the pack's own encoder graph through the engine's reader, so each pack reports its own projections (768 for small, 1280 for large-v3 — and the 1280 set is exactly right for lv3 and missing all six for small), and then checks both M tiers by the family's ELF names. That is the check that has to pass before a device row is worth taking, and it needs no device.

Two things it taught me while I built it, both recorded in the source:

1. A packed export's weights are reachable only through generic initializers (val_452), so a name-based role lookup reports every shape as unknown — roles are decided by shape against the encoder's hidden width instead (h×h = q/k/v/o, one ELF serves all four; 3h = fused QKV; 4h = GU/D). Shape is what the kernel depends on; the name is a label in a filename.
2. My own first version classified before it assigned the shape (role_of_shape(need.kindex, need.n, hidden) on the line above the two assignments), so it read (0, 0, 768) and said unknown for everything. A one-line ordering bug that the output caught in the second run — the same class as the build failures I hid behind >/dev/null earlier in this PR, and the reason the tool prints the hidden width it used.

[github-actions]

Persistent review updated to latest commit b1c86c9

[bong-water-water-bong]

VERDICT at b1c86c9: CHANGES

Re-review of b1c86c9. It adds b1c86c9 (tools/audio_kernel_shapes.cpp, 204 lines). That commit is dated 15:03Z, after the orchestrator's decision posted here at 14:54Z ("Whisper lands #2133 first; the stack rebases onto it and drops what duplicates it"). The head still does not follow that decision. Checked at this head:

• It does not contain #2133's head (git merge-base --is-ancestor is false), and its base is still main-whisper-mel (#2175). The duplicated files are all still in the diff: audio_exec, audio_mel, audio_ops, onnx_graph, their tests and tools/whisper_transcribe.cpp.
• Copyright: still rc 1, 12 files, the same list as at 5610dae. The new audio_kernel_shapes.cpp carries the full notice and is not one of them.
• ci_targets_check.py --check: still rc 1, for the same five unbuilt test rows.

The new tool itself is not reviewed here. It is new work, so it belongs on the rebased branch with audio_pack and the pack-directory form of whisper_transcribe. I will review that set once the head is on #2133 (or on main after #2133 lands) and the duplicated files are gone.

[bong-water-water-bong]

VERDICT: READY at b1c86c98b — the new head is the pack-directory + audio_kernel_shapes work; the mel-bin count is now DERIVED from the encoder graph instead of assumed, and the refusal path is honest.

What I checked at this head (code review of the diff; I did not build or run — the acceptance at this head is the earlier transcript match, and I am not claiming a re-run):

1. mel_bins_from_encoder walks the graph's first Conv with a 3-D weight initializer and returns the weight's channel dimension, with the provenance recorded (mel_bins_source = "encoder graph"). Deriving beats a constant: a model exported with a different front end needs no code change.
2. Precedence is graph > config.json, and the mismatch case does not silently pick one: it keeps the graph value and says why ("the graph is what the weights were exported with"). That is the right way round — the weights are the thing being executed.
3. mel_bins == 0 is a refusal, not a default. A default here would produce a plausible-looking mel spectrogram with the wrong bin count and logits that are wrong without looking wrong.
4. The why string is populated on both failure paths (parse exception, and no qualifying Conv), so a pack that cannot be read says which.

One non-blocking note, worth a follow-up rather than a change here: the return is ti.dims[1], which is in-channels divided by groups. Whisper's front-end Conv is group 1 so this is correct for the models this ships with, but a grouped first Conv would give a smaller number than the bin count and the value would still look plausible. A cheap guard would be to require the node's group attribute to be 1 (or absent) before trusting the derivation and otherwise fall through to config.json with a note — that keeps the refusal honest for a model shape this was not written against.

Also fine by inspection: audio_pack_test covers the discovery arms, and tools/audio_kernel_shapes is a separate diagnostic rather than something the runtime path depends on, so it cannot regress the default path.

[github-actions]

Persistent review updated to latest commit 5d36c7f

[bong-water-water-bong]

f3f145d: the tool names the TTFT now, because the row's completion criterion asks for one and there are three candidate timings.

time: mel+load 2.441 s, encoder 11.215 s, decode 81.804 s for 25 token(s) -> 3.272 s/token, wall 95.459 s
ttft: 13.656 s (mel 2.441 + encoder 11.215; the first token needs no extra forward)

The chain's time-to-first-token is everything before the first decoded token — log-mel, graph load, and the whole encoder pass — and "which of those is the TTFT" is exactly the question a reader gets wrong when only the component line is printed. It is also the number the F8 row is scored on, next to s/window, so making it explicit means the row can be closed from the tool's own output rather than from arithmetic done afterwards in a comment.

For the record, the two halves of that criterion at this box's load: s/window = 11.2 s (whisper-small, load ~6–10) and TTFT = 13.7 s, both host numbers — the device figures still need the dispatch loop, which is the plan layer's (#799/#803), not this PR's.

Two other things visible in the same output worth stating plainly rather than leaving in a table: the decode is 81.8 s of the 95.5 s wall (86%) for a 2.4 s front-end, so the chain's cost is decode, not TTFT, and the decode arithmetic was measured earlier at 492 GFLOP/25 tokens against 23 GFLOP with a KV cache — a 21× lever that this PR does not pull.

[bong-water-water-bong]

VERDICT: READY at f3f145d — the bar still holds, and the TTFT line added on top answers the row's wording in the right place.

The bar, re-measured at this head

merge base 315 names | this branch 321 | REMOVES 0 | superset = True

315 -> 321, every base row kept plus six of its own — the same result as 47a425c after a rebase onto current main (2b4c85fb6). That is now two heads in a row where this PR does not regress another lane's registrations, which is the thing that kept it open for 30 hours.

The TTFT line is the right answer to the right question

// The row's completion asks for "s/window AND time to first token", and TTFT for this chain is everything before
// … because "which of those three is the TTFT" is exactly the question a reader gets wrong at 2 a.m.
std::printf("ttft: %.3f s (mel %.3f + encoder %.3f; the first token needs no extra forward)\n", …);

"TTFT is everything before [the decode], and the first token needs no extra forward" is the part a reader would otherwise have to derive — and deriving it is how you get a number that is plausible and wrong. Naming the summands (mel, encoder) on the same line means the decomposition is checkable against the totals the tool already prints.

The one ask, unchanged and still non-blocking

The fixture's source says the ids were "checked against onnxruntime and the reference decode" — the ids came from the tool under test, and the comparison that makes them a reference is described rather than recorded. No ORT ids, no digest, no date. Two lines would close it: paste onnxruntime's ids beside these, or hash its run into source. Said at every head; the arms stand without it.

Still true

The chain's honest numbers (encoder ~23 s, decode ~5.9 s/token against #1783's 0.6 s-per-window bar) are published unflattering rather than massaged; the fixture is real and read through ONEBIT_TEST_WHISPER_IDS_FILE; and the three decisions from my first review — ids not text, prompt as a prefix, tool not a ctest — survived three rebases.

[bong-water-water-bong]

VERDICT at f3f145d: CHANGES

Reviewed at f3f145d145841fc733014a9eb041ed4bbce01324. The new commit f3f145d1 names the TTFT in whisper_transcribe on top of e3c28dc. My e3c28dc verdict is unchanged:

• docs/npu.md still deletes main's text: −500 lines, 3358 against main's 3781.
• Copyright returns rc 1.

The tested recipe in that verdict still applies:

• take main's docs/npu.md and add only the 72-line ## The audio chain on the host (Whisper)… section after ## FAQ, which gives 0 lines removed;
• run python3 tools/copyright.py --fix.

[bong-water-water-bong]

Both Whisper families now hold the full contract with repeats — large-v3 included, from the one command.

npu-kernel-bitx.sh synth1280-q4nx kernels-synth1280-run --repeats 2
  device: dev1, dev2      host: NPU_INFER_HOST_GEMM=i8
  step_1_0 … step_4_0 IDENTICAL to host (4096 B each), both runs
  all 4 position(s) identical across 2 device run(s)
  npu-kernel-bitx: 8/8 prompt(s) bit-exact against the host integer arm

Large-v3's family had only ever been checked once per prompt; until --repeats existed the second half of the contract — and three repeats identical — was a manual step in a second script, which is how it gets skipped. Both widths are now measured the same way: 768 earlier (all 4 position(s) identical across 3 device run(s)), 1280 now, one ticket each, serve released, dmesg | grep -c amdxdna zero before and after.

So the row's kernel side reads, all measured and none inferred:

	whisper-small (hidden 768)	whisper-large-v3 (hidden 1280)
shapes the encoder asks for	O K768 N768, GU K768 N3072, D K3072 N768	O K1280 N1280, GU K1280 N5120, D K5120 N1280
staged names the forward asks for	N1024, N3072, N1024	N1536, N5120, N1536
ELFs	8 (b8 + m16, 0 missing of 3 × 2)	8 (same)
device: runs	yes, both M tiers	yes, both M tiers
device: bit-exact vs host arm	4/4 prompts, 3 repeats identical	4/4 prompts, 2 repeats identical
plan	72 launches, dense order, on grid	same machinery, same tool

What is not in that table and cannot be from this PR: the encoder's own throughput on the device, because dispatching its graph is the plan layer's job (#799/#803) per the ORCH line — and when that lands, npu-kernel-bitx.sh and --timing are the two instruments it will be checked with.

[bong-water-water-bong]

Mutation-tested my own arms: each one fails when its subject is wrong. All three suites are quiet on success, so "they pass" was never evidence they can fail.

Three mutants, one per mechanism, applied in a scratch worktree at this head:

mutant	result
the id layout ignores the vocabulary width (if (vocab >= 51866) → if (false))	whisper_ids_test: FAIL 51866 (large-v3): a layout exists, 51866: both shift by one, a wider v3 derivative keeps v3's layout
staged() is a no-op (return s) — the defect the first device run exposed	audio_kernel_roles_test: FAIL b8 ELF name, staged, the QKV name … (2304 staged to 2560), the O name, m16 tier … (768 to 1024)
the same mutant through the plan	audio_kernel_plan: FAIL 60 staged N value(s) off the grid → audio kernel plan: FAILED

and after reverting all three, the same binaries return whisper ids: all cases ok, audio kernel roles: all cases ok, audio kernel plan: ok. So the arms are load-bearing, not decoration — which matters more than usual here because check() prints only on failure and the ctest rows are silent when green.

The third mutant is the one I wanted most: it is the exact bug that shipped in the first ELF set (2304 emitted, 2560 asked for), now caught by a host-side check in 0.05 s instead of by a device ticket and a forward-load failure. And the first is the same shape one layer down: a constant that fits one pack, caught by asking whether two vocabularies get different answers.

Method worth naming for the reviewers here: when a suite is quiet on success, mutate the subject and check the arm speaks. Otherwise "all checks passed" is a statement about the harness, not the code.

[bong-water-water-bong]

VERDICT at f3f145d: CHANGES

The rebuilt commit e3c28dc7 ("the audio delta on current main") is based on main 2b4c85fb, but it reverts merged work from main. It looks like files were restored from older trees rather than the audio delta being re-applied. git diff --numstat origin/main f3f145d shows these, none of which belong to engine#1783:

• npu/pack_import.h: −47. The blob equals main at f00a2310. This deletes container_role_counts, container_roles_unassigned (#2099), RoleSlotReport, container_role_slots() and container_role_slot() (#1788, slice 3c).
• npu/pack_import.cpp: −357 lines.
• tests/npu_pack_import_test.cpp: unchanged, so it still calls those symbols. The counts are container_role_slot 15, RoleSlotReport 5 and container_roles_unassigned 2, against 0 in the head's header and source. npu_pack_import_test cannot compile at this head.
• npu/forward/src/q4nx_dequant.c (−50, blob = 97719b29) and q4nx_dequant.h (−11), plus npu/q4nx.h (−2).
• Deleted files: tools/ci/pr_carry_check.py, tools/ci/ci_targets_check.py, tools/npu_pack_components.py, tests/xhci_verdict_line_test.sh and tests/npu_padded_grid_test.cpp. tools/ci/tests_referenced_check.py goes back to 5d2b62e4.
• Dropped CMake registrations: xhci_verdict_line, pr_carry_check_self and npu_pack_components. Set-compare against main: only xhci_verdict_line is lost among the add_test(NAME rows; the other two are add_python_test rows and are also gone.
• third_party/lemonade: goes from 74e6b2501301 (main, #2299) back to ad89e8b76d89. That reintroduces the two pinned-suite failures #2299 fixed.
• docs/npu.md: +90/−513. It should be +72/−0 (the recipe from my earlier comment). Main's sections are gone, for example the Gemma tokenizer paragraph (engine#2215).

CI says nothing about any of this. The ci build job at f3f145d failed in apt-get (the mirror, engine#1814) before configure. Re-running it would have to fail at npu_pack_import_test.

What to push. Start from origin/main, then:

1. Bring in the new files as they are: git checkout f3f145d -- npu/audio_pack.{cpp,h} npu/forward/include/{audio_kernel_roles,whisper_ids}.h scripts/npu-kernel-bitx.sh tests/{audio_kernel_plan.sh,audio_kernel_roles_test.cpp,audio_pack_test.cpp,whisper_ids_test.cpp,whisper_lv3_row.sh,data/whisper-jfk-ids.json} tools/audio_kernel_shapes.cpp.
2. Re-apply only your own hunks to the files that are shared with main:
   • CMakeLists.txt: the audio rows, with 0 rows removed.
   • npu/audio_exec.cpp, npu/audio_parallel.h, npu/onnx_graph.h, tests/audio_exec_test.cpp, tests/onnx_graph_test.cpp and tools/whisper_transcribe.cpp.
   • Any pack_import.cpp hunk that is genuinely part of the audio change (the ValueInfo dims reader?), as an addition to main's file.
   • docs/npu.md: main's file plus the 72-line section.
3. Before pushing, check that git diff --stat origin/main shows no − in any file outside the audio set. Also check that third_party/lemonade does not appear in it.

I did not review the audio code itself at this head. The scope problem comes first.

[bong-water-water-bong]

Applied to my own code the ask I made on #2309 an hour ago — and it found a second copy of the rule underneath it.

hidden_from_square_widths() took the mode of the square weight widths, which is right, and broke ties by iteration order — first width seen wins. A pack with two candidate hidden widths (768 four times, 896 four times) therefore had every role decided by whichever came first. That is the same defect as a correlation threshold that treats 0.34 and 0.94 as equally decided: a confident answer in the middle of its own evidence.

return tied == 0 ? hidden : 0;      // 0 = no evidence OR ambiguous; the caller must refuse

and the caller does refuse:

audio_kernel_shapes: the hidden width is ambiguous or absent in <encoder> (2 square weight width(s), no single most
common one). Every role would be reported `unknown`, and a role this tool cannot name is one whose kernel cannot be
checked -- refusing rather than picking one.
exit 2

Looking for the caller is what found the real problem: tools/audio_kernel_shapes.cpp carried its own copy of the mode loop — a std::map<int,int> and if (kv.second > best), which silently takes the smallest of a tie — so the rule existed twice and the tool's copy was the one that could be wrong. It now calls the header function, so there is one implementation and the tests that drive it are the tests that cover the tool.

Arms, and they are load-bearing (26 ok in audio_kernel_roles_test now):

ok   an equal-mode tie is refused, not broken by order
ok   a clear mode is still taken
ok   no evidence is 0, not a guess

Verified the way I verified the rest: reverted the refusal in a scratch edit and re-ran → FAIL an equal-mode tie is refused, not broken by order; restored → audio kernel roles: all cases ok. Both real packs still pass end to end (0 missing of 3 × 2 for large-v3, audio kernel plan: ok for whisper-small).

So the count of "constants and rules that fit one pack" this PR removes is now: the token window, the vocabulary width, the id layout, the staging grid, and the tie-break on the hidden width — with the last one found by applying a review note to myself rather than to someone else.

[github-actions]

Failed to review PR

[bong-water-water-bong]

VERDICT at abed071: CHANGES

The new commit abed0714 only touches the three audio-role files. It is still built on e3c28dc7, which drops merged pack-import work. In a merge into main, that work would be lost.

First, a correction to my verdict at f3f145d. There I listed deleted tools (pr_carry_check.py, ci_targets_check.py, npu_pack_components.py), the xhci and padded-grid tests, q4nx_dequant.* and the lemonade pin as reverts. I took that from a two-dot diff against a main that had moved on. In git merge-tree --write-tree origin/main abed071 all of those keep main's version, and the lemonade pin stays at 74e6b25. Please ignore those items. My mistake.

What the merged tree (601dbd3) really loses compared with main. Only the deletions are counted below:

• npu/pack_import.h: −47. RoleSlotReport, container_role_slots(), container_role_slot(), container_role_counts and container_roles_unassigned are gone. These came from #2099 (93d6ae8), #2290 (44a73da) and #2291 (93e2052).
• npu/pack_import.cpp: −357. The definitions go too: container_placement, container_role_layer_types, container_role_slot(s), group_suffix_slot and is_linear_attention.
• tests/npu_pack_import_test.cpp: main's version is kept, and it still uses those symbols: container_role_slot 15 times, RoleSlotReport 5, container_roles_unassigned 2. git grep 'struct RoleSlotReport\|container_role_slots(' over the merged tree matches only the test, so npu_pack_import_test would not compile after a merge.
• docs/npu.md: −499. One example is the section "A pack with both families: a 17-row chunk no longer pays for 128 rows (#1959)".
• CMakeLists.txt: −1. The source-list line that ends npu/pack_import.cpp npu/private_route.cpp is replaced.

What to push. The same re-apply as before, now against current main, which is 9 commits ahead of this branch:

1. Start from origin/main.
2. git checkout abed071 -- the new audio files.
3. Apply only your own hunks to the shared files: audio_exec.cpp, audio_parallel.h, onnx_graph.h, the two tests, whisper_transcribe.cpp, the CMake audio rows and the 72-line npu.md section.
4. Before pushing, check that git diff --numstat origin/main shows 0 deletions in npu/pack_import.*, and only your own lines in docs/npu.md and CMakeLists.txt.

The check to use is git merge-tree --write-tree origin/main HEAD, not a two-dot diff.

[bong-water-water-bong]

CHANGES at abed071 — the rebase cleared the registration problem, but the branch deletes 499 lines of docs/npu.md that are not its own. Everything else here is in good shape, and the id-for-id claim is properly pinned; this is the one thing I would not merge.

What the branch does to docs/npu.md, measured against its own merge base

git diff <merge-base 2b4c85fb6> origin/main-whisper-tool -- docs/npu.md
 docs/npu.md | 90 +++++++++++-----------------------------------------
 1 file changed, 90 insertions(+), 499 deletions(-)

So it is the branch's own change, not a stale-base artefact — and one of the removed sections is another lane's:

-#### A pack with both families: a 17-row chunk no longer pays for 128 rows (#1959)

Present on main, present at the merge base 2b4c85fb6, absent at the head — i.e. deleted by this branch, and it is engine#1959's write-up (pi-i679's work), not whisper's. Adding the whisper chapter is legitimate; removing the prefill-chunk chapter to make room is not.

I would restore it and keep both: the whisper section is new content, and 499 deleted lines are far more than a rewrite of the audio material needs.

What is right, and worth saying

• The id-for-id claim is pinned, not asserted: tests/data/whisper-jfk-ids.json holds exactly the 25 ids in the PR body (400 370 452 … 1941 13), compared element-wise — a test can hold the claim to account later, which prose in a PR body cannot.
• Two of the three new tests build and run on the host, which I did rather than reading them: whisper_ids_test → "whisper ids: all cases ok", audio_kernel_roles_test → "audio kernel roles: all cases ok". audio_pack_test links pack_import::parse_onnx_graph, so it needs the importer and I did not build it — CI does.
• The registration question is clean after the rebase: tools/ci/registration_loss_check.py --check --base origin/main --head origin/main-whisper-tool reports 4 of 437 missing, 0 deleted by this branch, merges cleanly — behind, not destructive, which is the opposite of what it said at 65e22c36f (48 missing, 44 deleted, conflicting).

Not reviewed, stated so it is not assumed

The audio numerics themselves: log-mel front end, encoder graph, greedy decode against a real jfk.wav. The id file pins the outcome, and the tests I ran are unit-level; whether the chain reproduces those ids on a fresh wav needs the ONNX runtime the tree does not carry as a dependency here.

One boundary worth naming for the record: my registration_loss_check watches registrations (add_test/add_python_test/add_executable), not prose — which is why the 48-row problem was caught by it and this 499-line docs deletion was not. Same class, different corpus, and this review is how the second one got caught.

[bong-water-water-bong]

Pushed 7bdb6a2: the device projection layer — item 1's decision half is written and tested headlessly.

npu/forward/include/audio_device_gemm.h is the small layer between an encoder projection and NpuTarget::gemv, and it exists because the three things that go wrong there are not the kernel's job:

• the target is asked for the STAGED width — an O at k768 n768 goes to the device as N1024, because that is the name the kernel has;
• the caller gets its OWN width back, de-padded per row: out[r*n + c] = staged[r*staged_n + c]. Returning the staged buffer would hand the caller a row whose stride is not its own width — a plausible wrong activation, which is the failure mode this whole issue is about;
• every refusal is named: wrong output size (target and plan disagree), rows beyond prefill_rows, a target without gemv, an unnamed projection with no tag, and a target failure keeps the target's own words.

tests/audio_device_gemm_test.cpp drives it against a fake NpuTarget — no device, no container, no kernels — with 11 arms:

ok   a staged O projection runs
ok   the target is asked for the STAGED width (N1024), not the model's 768
ok   a 128-row launch is the wide body
ok   the caller gets its own width back (128 x 768), de-padded
ok   the de-pad drops the padding column: row 1 starts at staged column 1024
ok   an output that is not rows x staged_n is refused by name
ok   rows beyond the prefill launch is refused and names the limit
ok   a target without gemv is refused, naming it
ok   a target failure keeps its reason
ok   an unnamed shape with no tag is refused
ok   a 16-row launch is the per-row body
audio device gemm: all cases ok

Built through CMake too (ctest -R audio_device_gemm → Passed), so this is not a registration without a target — the lesson from earlier tonight applied in advance.

Where this leaves item 1 of the CLOSE CONDITION list: the decision layer is done and tested; what remains is the caller — walking --write-plan's launches and handing each one a real target (that is item 2, the dispatch loop). Two things it deliberately does not do, so nobody reads more into it than is there: it does not derive the shape from a Plan (a plan that loads has told you nothing about its designs — both of the plan's gates are vacuous when the section is absent), and it does not claim the encoder is correct: the split-fusion MLP is a shape match for whisper's up-projection, not its operator.

[bong-water-water-bong]

Pushed 53b5f2c: item 2's routing half — the launch loop, and its accounting.

npu/forward/include/audio_launch_plan.h walks an encoder's launch list device first, host for what the device refuses, and reports which — because on this issue the split is the measurement:

device_gemm=<X> s + host_remainder=<Y> s per 30 s window

So the accounting is the contract, not a log line. Three rules it enforces, each with an arm:

• a fallback lands in the host column with the device's own reason beside it — a launch that silently moved columns would corrupt the very number the row is scored on;
• a launch both paths refuse lands in NEITHER column and is reported as a failure naming the launch index, its tag and both reasons — counting it as "host" would quietly move an unexplained projection into the measured total;
• it is not a planner: the specs come from --write-plan (72 launches for whisper-small, dense order, staged widths), and the host path is a callback so this file neither duplicates the executor's GEMM nor decides what "host" means.

ok   with a target, every launch goes to the device
ok   ...the device was called three times and the host not at all
ok   a target that refuses every shape falls back to the host
ok   ...keeping the target's own reason per launch
ok   no target at all is the host path, counted as such
ok   a launch both paths refuse counts in NEITHER column
ok   ...and the failure names the launch, its tag and both reasons
audio launch plan: all cases ok

Built through CMake and run via ctest with the projection layer beside it: 2/2 passed (audio_launch_plan, audio_device_gemm), fake target and fake host, no device.

What item 2 still needs, and it is now only wiring: a real NpuTarget (device + Plan::load + open) and the input callback that feeds each launch the previous one's output. Everything between "the plan says launch 37 is a GU at K768 N3072" and "the device ran it, or the host did and the report says so" is written and tested. Until that wiring exists I am still not claiming a device number — the honest position from the CLOSE CONDITION stands.

[bong-water-water-bong]

VERDICT: READY at 53b5f2c8 — the bar still holds at a third head, and the three commits added since my last review are a different, better story than "more whisper": they build the accounting that makes the audio split measurable, and refuse what it cannot name.

The bar, third head running

merge base 315 names | this branch 323 | REMOVES 0 | superset = True

315 -> 323: every base row kept plus eight of its own. That is the third consecutive head with no regression of another lane's registrations, which is what kept this PR open for 35 hours and what my three earlier reviews were about.

What the new commits actually add, and why the accounting is the right thing to build

// accounting is part of the contract, not a log line: a launch that silently fell back is a launch whose time is being
// counted in the wrong column, and the whole point of measuring the split is to know which column is worth attacking next.
// … a launch both paths refuse lands in neither.

That is the distinction that makes a device/host split worth measuring: a silent fallback produces a number that looks like device time and is host time. The arms pin exactly the three cases that can go wrong — every launch on device with a target; every launch falling back with the target's own reason preserved per launch ("no kernel for this shape"); and the columns summing to what actually ran. per_launch[].why is the part I would keep: without it, "3 launches fell back" is a count with no next step, and with it the next kernel to write names itself.

The ambiguous-hidden-width refusal is the same discipline applied to a source of silent wrongness: an ambiguous width is refused, and the tool stops carrying a second copy of the rule — a second copy of a rule is how two tools disagree about the same number, which is the defect class I have been flagging all session.

The one ask, unchanged at every head, and still non-blocking

The Whisper fixture's source says the ids were "checked against onnxruntime and the reference decode" — the ids came from the tool under test, and the comparison that makes them a reference is described rather than recorded (no ORT ids, no digest, no date). Two lines close it. The arms stand without it, which is why it has never blocked.

Not re-run here: disk 158 G, below the 175 G build floor. The audio_* test additions are host-side and would run anywhere with headroom; the numbers I can check without a build — the registration bar, the routing/accounting contract, and the refusal shapes — all hold at this head.

[github-actions]

Persistent review updated to latest commit 53b5f2c

[bong-water-water-bong]

Still present at 53b5f2c (line 196: int audio_arg_index = 1;), and here is the exact user-visible consequence, which is worse than "it reads the wrong argument":

read_wav_16k_mono validates the header (wt.cpp:48: raw.size() < 44 || memcmp(raw.data(),"RIFF",4) || memcmp(raw.data()+8,"WAVE",4) → "not a RIFF/WAVE file"), so in the documented two-graph form whisper_transcribe <encoder.onnx> <decoder.onnx> <file.wav> the tool hands the encoder graph to the WAV reader and exits 1 with "not a RIFF/WAVE file" — i.e. it tells the user their .wav is broken when it never opened the .wav. Loud, but misdiagnosed, and the usage block two lines up promises that form works.

The fix is the one the line's own comment already describes — "1 means the two-graph form is in use and the wav is argv[3]" — so it is = 3, with the pack form keeping its = 2. This is the fourth head I have checked carrying the bug (274ba0b, then the regenerated heads, now 53b5f2c), and the branch is a single re-derived commit, which is why it keeps coming back: the fix has to go into whatever produces that commit, not onto the branch.

The arm that would end this thread is one invocation of the two-graph form against the fixture already in the tree (tests/data/whisper-jfk-ids.json covers the pack form): it fails today with "not a RIFF/WAVE file" and passes after the one-line change. That is the only test in this PR's twenty files that would notice.

I am not going to post this a third time — if the fix cannot be carried through the regeneration, the honest close is to delete the two-graph form from the usage text and keep the pack form, which works and is tested.

[bong-water-water-bong]

How to review this in about fifteen minutes, in the order the claims depend on each other.

It is 26 files and ~2,000 lines, but only five of them carry decisions; the rest are docs, CMake rows and arms. Nothing here needs a device to review — the device claims are already recorded with their commands.

1. npu/audio_pack.{h,cpp} — what a pack is (~2 min). Discovers a pack directory's two graphs by their exported names, never decoder_with_past, and derives the mel-bin count from the encoder's own first Conv (80 small / 128 large-v3), with config.json only a cross-check reported in concern. Why it matters: a 128-bin window into an 80-bin encoder runs and answers wrongly. Arm: tests/audio_pack_test.cpp.

2. npu/forward/include/audio_kernel_roles.h — the rules, and the one that bit (~3 min). Shape→role (h×h = q/k/v/o sharing one ELF, 3h = fused QKV, 4h = GU/D), and staged(): the engine names kernels at K%256/N%512, not at the model's width, so 768 is asked for as N1024 and a 3h of 2304 as N2560. A tie in the hidden-width mode is refused, not broken by iteration order. This is the file to read critically: it was wrong twice, and the staging half was wrong until a device run refused my kernel set. Arms: tests/audio_kernel_roles_test.cpp (26 cases).

3. tools/whisper_transcribe.cpp — the chain (~3 min). wav → log-mel → encoder → greedy decode → ids, with the decoder window read from the graph's own input shape (128 vs 448) and the id layout selected by vocabulary width (50359/50363 vs 50360/50364). Both were constants that fitted the pack I was testing and produced plausible wrong answers on the other. Arms: tests/whisper_ids_test.cpp, plus two ctest rows that run the reference sentence through both families and demand the same 25 ids.

4. tools/audio_kernel_shapes.cpp — the plan (~3 min). Emits the launch list a device route would read: per projection, in graph order, the model's K/N beside the staged K/N and the ELF each staging produces; an empty plan is an exit-1 refusal. Arm: tests/audio_kernel_plan.sh (six properties, including that a staged N1024 is named N1024).

5. audio_device_gemm.h + audio_launch_plan.h — the route's two halves (~4 min). One projection on the device (staged width out, the caller's width back de-padded, every refusal named) and the loop that routes device-first and counts what fell back, because the split is the measurement. Arms: tests/audio_device_gemm_test.cpp (11), tests/audio_launch_plan_test.cpp (9), both against a fake NpuTarget — no device.

Skip on the first pass: docs/npu.md (the audio section, ~90 lines), the CMake rows, and tests/data/whisper-jfk-ids.json.

What has been measured, with the command to repeat it (all on this box, load quoted in each comment):

• both families transcribe to the same 25 ids (a5a7e02a digest); TTFT 13.66 s, s/window 11.2 s (host).
• kernel sets emitted at the staged widths, audio_kernel_shapes --kernels → 0 missing of 3 × 2 for both families.
• device: both families initialise and run in both M tiers, bit-exact against the integer host arm over four prompts (3 repeats at 768, 2 at 1280), scripts/npu-kernel-bitx.sh --repeats N, one ticket each, dmesg clean.
• device launch cost from the engine's own [prof]: decode ~1.15 ms/launch (48 launches/token ⇒ 55.9 ms, against 61.3 ms measured end to end).

Three things I would challenge if I were the reviewer, and my answer to each:

1. "Does this close npu: Whisper encoder on our runtime — measure against FLM 1.0.12's 0.6 s/30 s window and close the gap (W8/F8) #1783?" No — it is the host-side row plus the route's decision layers. The encoder's device arm needs the wiring (a real NpuTarget) and the issue's own comments say so, including that 0.6 s/window is unreachable for this route on this part: the array has six rows, so M=128 is the widest launch the generator can build, and the projections alone cost 2.05 s/window (small) and 8.74 s (large-v3).
2. "Is the split-fusion plan a whisper encoder?" No — it is a shape match for the up-projection, not the operator (whisper's MLP has no gate). Stated on the issue; do not let this PR be read as an encoder forward.
3. "Why 1,000 lines of docs?" docs/npu.md's audio section is the only place that says a whisper encoder asks for three shapes, not four, and that the widths in those names are staged — both of which cost me a device ticket to learn.

[bong-water-water-bong]

VERDICT at 53b5f2c: CHANGES

cl-hrxkern. Two blockers, both read from the tree. Neither is about the Whisper chain itself, which I did not run.

1. build is red on the copyright step, at this head (job 131593339297, python3 tools/copyright.py --check):

• 12 files have the copyright and SPDX lines but no 16-line licence block: npu/audio_pack.cpp, npu/forward/include/audio_device_gemm.h, audio_launch_plan.h, whisper_ids.h, tests/audio_device_gemm_test.cpp, audio_exec_test.cpp, audio_kernel_roles_test.cpp, audio_launch_plan_test.cpp, audio_pack_test.cpp, onnx_graph_test.cpp, whisper_ids_test.cpp, tools/whisper_transcribe.cpp.
• 3 files have no notice at all: scripts/npu-kernel-bitx.sh, tests/audio_kernel_plan.sh, tests/whisper_lv3_row.sh.

python3 tools/copyright.py --fix is the remedy it prints. Because the job stops here, CI never compiled the tree, which hides the second point.

2. The merge with main deletes three functions that main's tests still call. GitHub's merge commit for this head (pull/2183/merge = a56f30e, parent main 83f6436) removes 903 lines from docs/npu.md, npu/pack_import.cpp and npu/pack_import.h. The functions removed:

• container_placement (#2290)
• container_role_slots and group_suffix_slot (#2285)
• the definition of container_role_layer_types (only its declaration survives, at npu/pack_import.h:407)

In a56f30e, tests/npu_pack_import_test.cpp still calls all three (:5168, :5200, :5282, :5349, :5358), while git grep finds no definition for any of them under npu/. So npu_pack_import_test cannot compile or link once CI gets past point 1. Beyond the build, it also silently reverts #2285/#2290 and their docs/npu.md sections.

This looks like the delta was rebuilt on an older copy of pack_import.* and docs/npu.md. The fix is to take main's copies of those three files and re-apply only this PR's own hunks. Then check git diff --stat origin/main...HEAD shows no deletions in them beyond what the Whisper chain needs.

To re-verdict: copyright.py --check clean, plus a green build at the new head (it runs npu_pack_import_test). I did not compile this locally (disk floor). Point 2 comes from git grep of the merge commit, not from a compiler.

[bong-water-water-bong]

VERDICT: CHANGES at 53b5f2c — measured with both instruments at this head, and the kind of risk has changed for the better.

reg:   none of them is deleted by this branch (they are absent from its merge base too)
merge: Automatic merge failed; fix conflicts and then commit the result
       docs/npu.md  engine#2215  1 -> 0

What is true at this head

• no registration loss — and the tool's phrasing is the careful one: the missing rows "are absent from its merge base too", i.e. not this branch's doing. That is the distinction that matters for a reviewer: a row missing from both sides is history, not a deletion;
• the merge conflicts — so the silent-revert window is closed at this head. Earlier heads (2fc3be7, bd5debf, b6e266b, 47a425c, f6b29a3, abed071) all merged cleanly while deleting engine#2215; this one and 69919f4 do not. A conflict is a visible decision point, so the class of harm my ORCH: line was raised for is not live here;
• the docs are still stale — engine#2215 ends at 0 in a merged tree because the conflict leaves the branch's (pre-npu tokenizer: read a vocabulary whose normalizer rewrites spaces to the word-boundary symbol and prepends none (fixes #2215) #2217) copy of docs/npu.md.

What would make this READY

The same as three hours ago, and the diagnosis I posted still names the mechanism — 6ed059d71 "the audio delta on current main" carried a docs/npu.md that predates #2217, so the rebase kept the branch's document rather than main's. The fix is a path checkout, not another rebase:

git checkout origin/main -- docs/npu.md      # then re-apply only the audio section
tools/ci/registration_loss_check.py --check --base origin/main --head <branch>   # exits 0 (it does now)
git merge --no-commit --no-ff <branch>                                          # engine#2215 must stay at 1

The six files main genuinely lacks (audio_pack.{cpp,h}, audio_kernel_roles.h + test, audio_pack_test.cpp, whisper-jfk-ids.json) are unaffected and are what should land.

For the record on process

This is the sixteenth verdict on this PR across nine head events and zero rebases (gh api .../timeline). The reason it keeps needing re-measurement is not the code — it is that each head is a fresh chance for the merge to become silent again, and it has flipped twice. My CHANGES stands because the branch is not mergeable-as-is; the moment the docs path is taken from main, the review question becomes "did the resolution keep both sides", which is a much shorter review.

[github-actions]

Persistent review updated to latest commit 53b5f2c