Skip to content

npu: read an audio pack's graph whole and run its encoder and decoder on the host (refs #1783) - #2173

Closed
bong-water-water-bong wants to merge 1 commit into
main-audio-kernelsfrom
main-whisper-exec
Closed

bong-water-water-bong wants to merge 1 commit into
main-audio-kernelsfrom
main-whisper-exec

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Second slice of #1783 (F8), stacked on #2154 (the four f32 host kernels it calls; that PR's base is main). Refs #1783, which is my row.

What the engine gains: it can read an audio pack's ONNX graph whole and execute it — 447-node encoder and 786-node decoder — and the output matches onnxruntime on both. That is the host reference the NPU arm of F8 will be measured against, in-tree rather than a Python script that can drift (tools/whisper_encoder_ref.py was a second implementation of the same arithmetic).

whisper-small whisper-large-v3
nodes 447 1031
our encoder output vs ORT mean −0.009045236 vs −0.009045244, std 1.678493020 vs 1.678493023 runs (487 of 492 initializers are in the side .onnx.data)
decoder 786 nodes → [1,128,51865], last-row probes within 2e-3 —

Two facts the reader found that changed the executor, and both are asserted:

  1. The family spells GELU two ways — whisper-small decomposes it into Div → Erf → Add → Mul → Mul (0 Gelu nodes); large-v3 carries a real Gelu (34). The invariant that holds either way is Erf + Gelu == layers + 2, and the test pins it as a ratio against the layer count so it holds at both depths.
  2. The weights arrive transposed (61 and 161 Transpose nodes), which is why the graph's MatMuls read (K, N).

One defect in shared code, fixed here: fields() dropped every wire-type-5 (fixed32) payload, so LayerNormalization's epsilon read as absent rather than as a value. Nothing read wire 5 before, so no behaviour changes; the test now asserts epsilon is present and in range.

Deliberately not here: the mel front-end (STFT + filterbank), the transcription tool, and any device path — those are the next slices. The executor refuses an op it does not implement by name and node index rather than skipping it.

Verified: fresh configure rc 0; onnx_graph_test and audio_exec_test both run green against the two packs on this box (both skip with 77 without a path). ctest -R onnx_graph|audio_exec green.

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1783 - Partially compliant

Compliant requirements:

  • Baseline harness: The PR introduces tests that run the encoder and decoder on the host and compare against onnxruntime, measuring the encoder output
  • Encoder route on our runtime: The PR adds the capability to read an audio pack's ONNX graph and execute it on the host
  • Correctness bar: The PR includes tests that verify the output matches onnxruntime, ensuring correctness
  • Measure and close the gap: The PR provides the host reference implementation that can be used to measure performance
  • Row: The PR lays the groundwork for the row by implementing the core functionality needed for audio pack execution

Non-compliant requirements:

  • The PR does not yet implement the full NPU execution path for Whisper (the actual device execution)
  • The PR does not yet include the final performance measurements against the FLM claims
  • The PR does not yet include the full row implementation (the serving and device-specific parts)

Requires further human verification:

  • The actual performance measurements on the NPU hardware against FLM claims
  • The final row implementation and integration with the device execution
  • Verification that the host reference implementation is correctly used for performance comparisons
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fixed32 Attribute Handling

The fix for handling fixed32 attributes (wire type 5) in protobuf parsing is critical for correctly reading LayerNormalization's epsilon attribute. This ensures that the epsilon value is properly read and used, which is essential for the correctness of the layer normalization operation. Without this fix, the epsilon would be read as 0.0, making it indistinguishable from an absent attribute, potentially leading to incorrect layer normalization behavior.

case 5:
    if (size_t(end - p) < 4) throw std::runtime_error("protobuf: truncated fixed32");
    // engine#1783: keep the four bytes in v. A float attribute (LayerNormalization's epsilon) is wire 5, and
    // without this every such attribute reads as 0.0 -- indistinguishable from an absent one.
    f.v = uint32_t(p[0]) | (uint32_t(p[1]) << 8) | (uint32_t(p[2]) << 16) | (uint32_t(p[3]) << 24);
    p += 4;
    break;
Thread Safety in Parallel Execution

The parallel_units function uses std::thread for parallel execution, which can lead to thread safety issues if shared resources are not properly synchronized. While the implementation attempts to ensure bit-identical results by using a deterministic fork-join pattern, it's important to verify that no shared mutable state is accessed concurrently without proper synchronization, especially in a multi-threaded environment.

template <typename Fn>
void parallel_units(size_t units, Fn&& fn) {
    size_t want = 1;
    if (const char* env = std::getenv("ONEBIT_AUDIO_THREADS")) {
        const long n = std::strtol(env, nullptr, 10);
        want = n > 0 ? size_t(n) : 1;
    } else {
        const size_t hw = std::thread::hardware_concurrency();
        want = hw == 0 ? 1 : (hw > 8 ? 8 : hw);
    }
    if (want > units) want = units;
    if (want <= 1) {
        for (size_t u = 0; u < units; ++u) fn(u);
        return;
    }
    std::vector<std::thread> pool;
    pool.reserve(want);
    for (size_t t = 0; t < want; ++t)
        pool.emplace_back([&, t]() {
            for (size_t u = t; u < units; u += want) fn(u);
        });
    for (auto& th : pool) th.join();
}
Memory Management in Tensor Operations

The code performs memory allocations for tensors and their data, particularly in operations like MatMul and LayerNormalization. It's important to ensure that these allocations are efficient and do not cause memory fragmentation or excessive memory usage, especially in a constrained environment like an NPU runtime.

if (n.op == "MatMul") {
    const Tensor& a = need(n.ins[0]);
    const Tensor& b = t.count(n.ins[1]) != 0 ? need(n.ins[1]) : init_f32(n.ins[1]);
    if (a.dims.size() < 2 || b.dims.size() < 2) fail(where() + ": needs rank >= 2 on both sides");
    const size_t ra = a.dims.size(), rb = b.dims.size();
    const int64_t M = a.dims[ra - 2], K = a.dims.back(), K2 = b.dims[rb - 2], N = b.dims.back();
    if (K != K2) fail(where() + ": " + std::to_string(K) + " vs " + std::to_string(K2));
    // Attention's q.k^T and scores.v are 4-D: (batch, heads, M|K, K|N). The batch dims broadcast, the last two
    // are a plain GEMM, so one projection covers both the layer weights and the attention products.
    const std::vector<int64_t> ab(a.dims.begin(), a.dims.end() - 2), bb(b.dims.begin(), b.dims.end() - 2);
    const Broadcast bp = plan_broadcast(ab, bb);
    r.dims = bp.dims;
    r.dims.push_back(M);
    r.dims.push_back(N);
    r.data.assign(nelem(r.dims), 0.0f);
    // Rows are independent, so the work splits over (batch, row) without touching any element's arithmetic.
    const size_t batches = nelem(bp.dims);
    const size_t rows = size_t(std::max<int64_t>(M, 1));
    parallel_units(batches * rows, [&](size_t u) {
        const size_t bi = u / rows;
        const int64_t i = int64_t(u % rows);
        std::vector<size_t> idx(bp.dims.size(), 0);
        size_t rem = bi;
        for (size_t d = bp.dims.size(); d-- > 0;) {   // row-major unflatten of the batch index
            const size_t dim = size_t(bp.dims[d] ? bp.dims[d] : 1);
            idx[d] = rem % dim;
            rem /= dim;
        }
        size_t oa = 0, ob = 0;
        for (size_t d = 0; d < idx.size(); ++d) {
            oa += idx[d] * bp.stride_big[d];
            ob += idx[d] * bp.stride_small[d];
        }
        const float* ap = a.data.data() + oa * size_t(M * K);
        const float* bpp = b.data.data() + ob * size_t(K * N);
        float* rr = r.data.data() + bi * size_t(M * N) + size_t(i) * size_t(N);
        for (int64_t k = 0; k < K; ++k) {
            const float av = ap[size_t(i) * size_t(K) + size_t(k)];
            if (av == 0.0f) continue;
            const float* br = bpp + size_t(k) * size_t(N);
            for (int64_t j = 0; j < N; ++j) rr[j] += av * br[j];
        }
    });

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 57b8747: CHANGES

Reviewed head 57b8747 (base main-audio-kernels, i.e. stacked on #2154). This is the first verdict.

This is #1931's pre-review code, and it undoes a fix #2133 carries. #2133 (open, with 3 VERDICT reviews including my READY at 7e10fcc) already lands all of these files on main. Diffing this head against #2133's 7e10fcc, file by file:

  • audio_exec.h, audio_mel.h, onnx_graph.h, audio_exec_test.cpp, audio_mel_test.cpp, onnx_graph_test.cpp: identical except the 12-line Apache notice, which is missing here.
  • npu/audio_exec.cpp (+28/−13): it drops #include "audio_parallel.h" and inlines the old fork-join, in which a unit that throws escapes its std::thread and calls std::terminate. #2133's npu/audio_parallel.h catches per worker, joins all and rethrows on the caller, and audio_parallel_test pins it. My mutants at 7e10fcc showed that test fails with the exception swallowed. Here neither the header nor the test exists, so a throwing MatMul unit takes the process down.

CI is red on the missing notices. build's copyright step, reproduced locally:

missing copyright notice: tests/audio_exec_test.cpp
missing copyright notice: tests/audio_mel_test.cpp
missing copyright notice: tests/audio_ops_test.cpp      (from #2154, which this stacks on)
missing copyright notice: tests/onnx_graph_test.cpp

#2133 and this cannot both land: each adds the same files with different content.

Suggested: close this and #2154 in favour of #2133. If the reviewers want #2133 split, split it from #2133's files as they are, which already have the notices and audio_parallel.h, rather than from #1931's earlier commits.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 57b8747

Canonical line for a CHANGES that already exists on this head, and I reproduced both of its findings before restating them — a block the gate cannot see is not a block.

1. The copyright step fails, reproduced here on this head:

$ python3 tools/copyright.py --check
missing copyright notice: tests/audio_exec_test.cpp
missing copyright notice: tests/audio_mel_test.cpp
missing copyright notice: tests/audio_ops_test.cpp      <- from #2154, which this stacks on
missing copyright notice: tests/onnx_graph_test.cpp
$ rc = 1                                                     (9 notices missing in total)

So the build job's red is not a flake and not the parent PR's alone: four of the missing notices are the test files this branch brings, one of them inherited from #2154. That
means anchoring the stack on #2154 does not clear it either.

2. The regression claim, verified in one grep:

$ grep -c audio_parallel.h npu/audio_exec.cpp
0

The prior review says this branch "drops #include "audio_parallel.h" and inlines the old fork-join, in which a unit that throws terminates the process". The include is indeed gone,
and #2133 — which carries that fix and both mutant-proved arms (mutant A worker-stride → bit-identity FAIL; mutant B no try/catch → terminate called) — adds the same files. Two
PRs adding the same files with different content cannot both land
, and the one with the exception path is the one to keep.

Where I land, same as the prior reviewer: close this and #2154 in favour of #2133. If the audio work has to be split, split it from #2133's files, so the split inherits the
audio_parallel.h seam rather than re-deriving a version without it.

Why I am posting this at all rather than leaving the earlier CHANGES: it is written VERDICT at 57b8747: CHANGES — state after the word at — which the gate's form does not match,
so this PR currently looks unverdicted to any sweep using that rule (the same sweep is how I got here). Two PRs tonight (#2123 above, and #1960 per another lane's brief) had a verdict
only in that form. A verdict the gate cannot parse is a verdict that does not exist, and the durable fix is one line in the reader; this line is the workaround.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Measured before/after for the threading that is in this PR's audio_exec.cpp but had no number attached (interleaved on one binary, same input, ONEBIT_AUDIO_THREADS as the control):

threads=1 (serial):  mel+load 7.220 s, encoder 30.792 s, decode 206.080 s -> 8.243 s/token, wall 244.093 s
threads=8 (default): mel+load 3.717 s, encoder 13.186 s, decode 136.222 s -> 5.449 s/token, wall 153.125 s

Encoder 2.33× (30.8 → 13.2 s) and decode 1.51× (8.24 → 5.45 s/token), end to end 1.59× — with the transcript identical both times (same 25 ids, checked against the reference decode). The arithmetic is untouched by construction: the split is over (batch, row), each thread writes a disjoint slice, and the per-element summation order does not change — which is why the ids match rather than merely "match closely", and why the serial path is available as ONEBIT_AUDIO_THREADS=1 for anyone who wants the old behaviour or an A/B.

Two things this number is good for, both about #1783 rather than about the code:

  1. It is the honest size of the gap the row has to close. Our host encoder is now 13.2 s per 30 s window against FLM's 0.6 s on the NPU — a factor of 22 on the host, which says the comparison the issue wants is not "tune the host loop", it is "get the encoder onto the NPU". The 30.8 → 13.2 s step makes that clearer rather than blurring it, because it removes the naive-loop excuse from the host side.
  2. It gives the device arm a fair baseline to report against: if the NPU route lands at, say, 1.5 s, the honest statement is "11× faster than our own host reference at 8 threads", not "2.5× better than the number we had before".

Not changed by this measurement: the serial path's output and the threaded path's output are the same 25 ids, so this is a throughput change only. No optimisation beyond threading is attempted here — a blocked MatMul is not in this PR.

bong-water-water-bong pushed a commit that referenced this pull request Nov 11, 2026
… one

Measured on the shape #2199 names: a header keeping only `// Copyright ...` and
`// SPDX-License-Identifier: Apache-2.0`.

  $ copyright.py --check     copyright + SPDX present but the licence block is not: short.h
  $ copyright.py --fix       added: short.h
  $ copyright.py --check     copyright notice duplicated inside the header window: short.h   rc 1

--fix prepended the canonical block and LEFT the two stub lines, so the file came out
with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed
red. The tool's own suggested remedy could not repair the shape it had just named --
which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have
not self-healed with one command.

`strip_short_header()` removes exactly the two lines `short_notice()` looks for, and
only when they are the head of the file: anything else is returned unchanged, so
`--fix` behaves as before for every other shape. The file's own header text and a
leading `#!` are kept.

After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the
description line intact.

Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short
header, exactly one SPDX line survives, the full block is there, the file's own header
text is kept, and --check is green after the repair. That file: 23 arms, 0 failures.
`copyright.py --check` on this tree rc 0 (nothing that was green became red).

Refs #2199, #2175, #2183, #2154.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator Author

Absorbed into #2183, which is now based on main — the lane is at the PR cap (16:45Z: at most two open PRs, and no new ones while main is red), and a four-deep stack of stacked PRs is exactly the fragmentation the cap exists to remove.

This PR's commit and everything it depended on are in #2183 (7 commits, 2,347 lines, all 16 files): kernels → graph reader + executor → log-mel front-end → the wav→ids tool and the pack container. Nothing is withdrawn; the reviewable surface is one PR with the whole host chain in it, and the device arm's preconditions are checkable from it (audio_kernel_shapes --kernels).

bong-water-water-bong added a commit that referenced this pull request Nov 12, 2026
…2199)

* copyright: a short header is named as one, not as a missing notice

engine#2175 / #2173 have been red on "Copyright notice on every file" for a day, and
the message sends the reader the wrong way: the notice is on line 1. What those files
keep is `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`, and then they
jump to the file's own description -- the 16-line licence block `notice_block()`
compares against is not there, so `has_notice()` is false and `missing[]` prints
"missing copyright notice".

Measured on the two heads, same work, four lines apart:

  #2175 audio_mel.h line 4:  // audio_mel.h -- the log-mel front-end: hann-windowed STFT ...
  #2133 audio_mel.h line 4:  // Licensed under the Apache License, Version 2.0 (the "License");

#2133 is rc 0 and #2175 is rc 1 on that difference alone, so the fix is a 14-line paste
and not a hunt. `short_notice()` names the shape:

  copyright + SPDX present but the licence block is not (paste the 16-line form): <path>

The exit code is unchanged -- a short header is still a failure, because the licence
block is what the gate exists to require. Only the sentence changes, so nothing that
was green becomes red and nothing that was red becomes green.

Test: three arms in tests/copyright_check_untracked_test.py on a staged short header --
it fails, the failure names the shape, and it does NOT claim the notice is missing.
Verified: that test 18 arms / 0 failures; `copyright.py --check` on this tree rc 0;
and the predicate on the real files (short.h -> has_notice False / short_notice True,
full.h -> has_notice True / short_notice False).

Refs #2175, #2173, #2133.

* copyright: --fix replaces a short header instead of stacking a second one

Measured on the shape #2199 names: a header keeping only `// Copyright ...` and
`// SPDX-License-Identifier: Apache-2.0`.

  $ copyright.py --check     copyright + SPDX present but the licence block is not: short.h
  $ copyright.py --fix       added: short.h
  $ copyright.py --check     copyright notice duplicated inside the header window: short.h   rc 1

--fix prepended the canonical block and LEFT the two stub lines, so the file came out
with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed
red. The tool's own suggested remedy could not repair the shape it had just named --
which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have
not self-healed with one command.

`strip_short_header()` removes exactly the two lines `short_notice()` looks for, and
only when they are the head of the file: anything else is returned unchanged, so
`--fix` behaves as before for every other shape. The file's own header text and a
leading `#!` are kept.

After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the
description line intact.

Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short
header, exactly one SPDX line survives, the full block is there, the file's own header
text is kept, and --check is green after the repair. That file: 23 arms, 0 failures.
`copyright.py --check` on this tree rc 0 (nothing that was green became red).

Refs #2199, #2175, #2183, #2154.

* copyright: --fix finds a short header behind a shebang

The first version of strip_short_header() read lines 0-1 only, so a script's short
header -- which sits on lines 1-2, behind `#!` -- was not found, add_notice() put the
block in after the shebang and left the stub, and the duplicate rule fired:

  $ copyright.py --check   copyright + SPDX present but the licence block is not: short.sh
  $ copyright.py --fix     added: short.sh
  $ copyright.py --check   copyright notice duplicated inside the header window: short.sh   rc 1

That is the same defect one file type over, and it is the file type most likely to be
written quickly: a new script.

Now the two lines are located after a leading `#!` and the shebang is carried through.
After: --fix -> added, --check rc 0, one SPDX line, the shebang still on line 1, the
script's own comment intact.

Five arms in tests/copyright_check_untracked_test.py: --fix repairs a short header
behind a shebang; the shebang stays on line 1; exactly one SPDX line; the script's own
comment survives; --check green afterwards. That file: 28 arms, 0 failures.
`copyright.py --check` on this tree rc 0, and re-validated on engine#2183's real branch
(12 short headers -> --fix -> rc 0, unchanged by this commit).

Refs #2199, #2183.

* copyright: drop strip_short_header, now that main's --fix does that job

Answers the CHANGES at 751aa35. Main has taken two --fix changes since this branch
was cut:

- #2192 (83012c8) repairs a short header, including one behind a `#!` -- `add_notice`
  filters `canonical_notice_line()` out of the head before prepending the block;
- #2228 (ada88bb) makes --fix keep blank lines.

Merged on top of those, this branch's `strip_short_header()` was redundant and
actively wrong: its `if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:]`
dropped a blank line after the stub, undoing #2228. The reviewer measured both:

| file | main | this head (before) |
|---|---|---|
| blank.sh (`#!` + stub + blank + code) | blank kept | blank dropped |
| blank.h (stub + blank + `#pragma once`) | blank kept | blank dropped |

So the function and its call are gone. What this PR keeps is the half main does not
have: `short_notice()` and the sentence it prints --

    copyright + SPDX present but the licence block is not (paste the 16-line form)

-- which is why the PR exists at all: main's `copyright.py` still says "missing
copyright notice" for a file whose notice is on line 1.

Re-ran the reviewer's four-case table at this head: `short.h`, `tool.py` (#! + # stub),
`blank.sh` (#! + stub + blank), `blank.h` (stub + blank + pragma) all repair to one
SPDX line, `--check` rc 0, **blanks kept where the fixture had one**, shebangs on line 1,
and each file's own comment preserved.

tests/copyright_check_untracked_test.py: 32 arms, 0 failures (main's blank-line arms and
this branch's short-header arms both present). copyright.py --check on this tree: rc 0.

---------

Co-authored-by: bong-water-water-bong <bong@water>
Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 57b8747

#2199 merged, so this is now one command rather than a paste -- and I ran the merged tool on this branch's head to confirm.

Measured at this head with origin/main:tools/copyright.py (the merged one): 9 short-header path(s) — files that carry // Copyright and // SPDX and not the 16-line licence block. The
sweep after #2199 landed, across the four branches that had this shape:

PR short-header paths
#2183 6
#2175 9
#2173 9
#2154 2

The command, verified on #2154 (the smallest one) rather than assumed:

$ python3 tools/copyright.py --fix
added: npu/forward/include/audio_ops.h
added: tests/audio_ops_test.cpp
$ python3 tools/copyright.py --check     rc 0

Nothing else changed, no description lines lost. The same run on #2183 cleared all six.

So: python3 tools/copyright.py --fix && python3 tools/copyright.py --check on this branch, and the copyright step goes green. That is the whole ask -- the tool now names the shape
("copyright + SPDX present but the licence block is not (paste the 16-line form)") and repairs it, which is what four branches spent a day waiting for.

Why still CHANGES: the 9 files are unmodified at this head, so build's copyright step fails on it. Nothing else in the tree is red as far as this lane can see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant