Skip to content

Stage 1 prerequisites: M55 Ooura-leg flag typo, stale bare-metal filter, fingerprint harness - #50

Merged
tap merged 3 commits into
mainfrom
claude/wave1-stage1-prereqs
Sep 17, 2026
Merged

tap merged 3 commits into
mainfrom
claude/wave1-stage1-prereqs

Conversation

@tap

@tap tap commented Sep 17, 2026

Copy link
Copy Markdown
Owner

What this changes

The three MuTap-side prerequisites of Stage 1 of the DspTap FFT plan (DspTap docs/audit-fft-and-code-smells.md, Part 3 Stage 1; review items P5 and P2): the M55 Ooura-fallback CI leg's stale option name, the bare-metal / Hexagon gtest filters that still named the FFT suites that moved to DspTap, and tests/branchless_parity_check.cpp generalized into tests/fingerprint_harness.cpp, the bit-identity gate every DspTap pin bump runs.

Why

  • CI option name (P5). The "Ooura FFT fallback" M55 leg passed -DMUTAP_FFT_CMSIS=OFF. That was the option's name when the leg was written (1d8bd35); the DspTap extraction (14116f0) renamed it to TAP_DSP_FFT_CMSIS and did not touch ci.yml, so from 14116f0 until this PR CMake ignored the unknown -D, the leg rebuilt CMSIS, and the Ooura-float32-on-M55 profile — the profile the FFT port changes — had no coverage. The leg now passes the real option, asserts the typed cache entry TAP_DSP_FFT_CMSIS:BOOL=OFF (an unknown -D lands as :UNINITIALIZED, so a future rename fails the leg instead of silently rebuilding CMSIS), and its Fingerprints step greps backend=ooura from the harness binary that actually ran on target — a positive proof of the compiled backend. The CMSIS leg asserts backend=cmsis, M33 and Hexagon backend=ooura. The same stale name is fixed in the icount job comment, docs/optimization.md and bench/README.md; optimization.md's pointers to the wrapper, its tests and the vendored CMSIS subset now name DspTap, and its vDSP section states that MuTap turns TAP_DSP_FFT_ACCELERATE back off (macOS float32 flake: Apple vDSP's real FFT is non-deterministic across processes at N=2048 #31).
  • Stale filter. tests/bare_metal_main.cpp and the cross-compile TEST_FILTER in tests/CMakeLists.txt named real_fft_test/*, RealFftCrossPrecision.* and CertifiedGeometries/fft_backend_parity.*, none of which exist in mutap_tests any more (gtest silently selects nothing for them). Pruned; every remaining name was grepped against tests/test_*.cpp (each found in exactly one file), and the host binary selects 52 tests under the pruned filter, so the >= 30 empty-run guard is untouched. Honest consequence, now stated in optimization.md: at the current DspTap pin the CMSIS bin-for-bin parity gate (fft_backend_parity) is executed by no CI anywhere — DspTap's M55 leg is compile-only with tests off — and will be by DspTap's own M55 leg once DspTap Float32 parity: gates for the embedded oracle + narrowband guard #17 lands; until then the M55 float32 battery (52/52) is the only CMSIS gate.
  • Fingerprint harness (the gate P2's rollout rule needs). One binary, mutap_fingerprint, prints FINGERPRINT <component> <profile> <hex> for fdaf, fd_kalman (both with the error block AND the echo-estimate block hashed, via the four-argument process_block, because error = desired - estimate can absorb a one-ULP move of the estimate under a loud near end), pem_afc, postfilter (residual_suppressor alone), nn_suppressor (alone, deterministic live weights at the shipping 48 kHz geometry), aec_chain and aec_chain_nn (both presets), in float and double — 14 lines, FNV-1a over the raw bytes of every output sample. The corpus (level-swept far end, 3-tap sparse echo, colored + white double-talk near end in every third 40-block stretch from block 40 on, a floor) is generated from xorshift state with basic IEEE arithmetic only — no libm, no <random>, no clock, no filesystem — and rounded once to float so both profiles consume identical values and the binary runs unchanged on bare metal. The # header names the geometry, the float32 FFT backend the binary was compiled with (backend=cmsis|vdsp|ooura) and the suppressor form. The file header documents the pin-bump workflow: build, run, git -C submodules/dsptap checkout <pin>, rebuild, run, diff; HANDOFF note 6 and the PR template's "Submodule pin moved" bullet point bumpers at it. The branchless-parity job compiles the same file twice exactly as before (DspTap's fftsg.c/fftsg_float.c by path — Stage 2c/P18 rewrites that job) under set -eo pipefail and diffs every FINGERPRINT line with a 14-line floor on both files; every leg, hosted and emulated, has a Fingerprints step that runs the harness once, verbosely, after a battery that excludes it (-E), so each CI log carries the lines.

Fingerprints at this head (Linux x86-64, GCC 13.3, backend=ooura; identical for MUTAP_SUPPRESSOR_BRANCHLESS=0 and =1):

FINGERPRINT fdaf float b05f67a7426ed1f7
FINGERPRINT fd_kalman float d0dfe8c5f184d382
FINGERPRINT pem_afc float 5183c6aacd998a43
FINGERPRINT postfilter float 2e96c478d53c5eaa
FINGERPRINT nn_suppressor float 60e3762dfcef9fe2
FINGERPRINT aec_chain float 7982243bf2aab6f8
FINGERPRINT aec_chain_nn float 9b0add89588061da
FINGERPRINT fdaf double f9b5694359b78075
FINGERPRINT fd_kalman double 95567d89cbd871c7
FINGERPRINT pem_afc double 19423ce8e48b6b48
FINGERPRINT postfilter double 7143cf725565d4ab
FINGERPRINT nn_suppressor double 9b692f0e7ec4ae7c
FINGERPRINT aec_chain double 8208d54a5b57198a
FINGERPRINT aec_chain_nn double c9616eb564dfaed1

(These differ from the values at 72e7e69 for two expected reasons: the two cancellers now hash their estimate stream as well, and double talk starts at block 40 instead of block 0.) The on-target values are in each emulated leg's Fingerprints step; the main run of this merge is the baseline every later bump diffs against.

Verification

Ran locally (Linux, GCC 13.3, x86-64):

  • cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DMUTAP_WERROR=ON && cmake --build build -j4: clean under -Werror with the house warning set.
  • ctest --test-dir build --output-on-failure -j4: full suite, 198/198 passed in 587 s at 72e7e69 (including mutap_fingerprint); at this head the float suites (the 52-test emulated selection plus every float-typed hosted suite and Float32Parity) were re-run — the harness is the only compiled change since.
  • mutap_fingerprint run twice from the CMake build: byte-identical. The standalone build the parity job uses (g++ -std=c++20 -O2 ... tests/fingerprint_harness.cpp fftsg.o fftsg_float.o, with -Wall -Wextra -Wpedantic -Wconversion -Wshadow -Werror added) for MUTAP_SUPPRESSOR_BRANCHLESS=0 and =1: all 14 FINGERPRINT lines identical to each other and to the CMake build's. ~0.7 s per run.
  • mutap_tests --gtest_list_tests under the pruned filter: 52 tests selected.
  • pre-commit run --all-files (the pinned clang-format v18.1.3 hook): passed; ci.yml parses (11 jobs).

Not run locally — CI is the first real gate for these:

  • The Cortex-M55 Ooura-fallback leg (no Arm toolchain or QEMU in the dev container). This PR is the first time since 14116f0 that the leg builds the Ooura float32 backend on the M55.
  • The Cortex-M55 (both backends), Cortex-M33 and Hexagon legs run mutap_fingerprint under emulation once each, verbosely, and assert backend=; double is soft-float on the Cortex-M legs (M33 worst case ~4 min against the 900 s per-test timeout). At 72e7e69 the M55 legs showed the contract's prediction: the seven double lines identical between CMSIS and Ooura, the seven float lines all different.

Notes for the reviewer

  • No contract change; no submodule pin move (submodules/dsptap stays at 5ca3b1c). Notebooks untouched (no behavior changed).
  • Merge order. This PR must merge before any DspTap pin bump (DspTap TCLwdt over time: three-coupling trace, notebook figure, corrected story #20, Stage 0, is open now): the rollout rule wants every bump PR to say which FINGERPRINT lines are unchanged, and the main CI run of this merge is the baseline for every leg (hosts, M55 CMSIS and Ooura, M33, Hexagon). Stage 2b's gate depends on it.
  • MuTap-Max pin drift. MuTap-Max/submodules/MuTap is at 96b4572, three commits behind main once this merges. Nothing functional is owed (no header or API changed); the re-pin rides the next Max PR per HANDOFF note 6.
  • bench/README.md was touched for one word (the option name) although it was outside this item's file list; it was the last remaining MUTAP_FFT_CMSIS in the tree. .github/pull_request_template.md and HANDOFF.md each gained one sentence pointing pin bumpers at the harness (review B S3).
  • The old single-line MUTAP_SUPPRESSOR_PARITY_FP output is gone; the parity job's comparison moved to a diff of the FINGERPRINT lines.
  • Per the plan's swarm rules this PR owns ci.yml, tests/CMakeLists.txt, tests/bare_metal_main.cpp, tests/fingerprint_harness.cpp and docs/optimization.md in wave 1; nothing under submodules/, include/ or the root CMakeLists.txt changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

tap and others added 2 commits September 17, 2026 18:12
…rint harness

Three MuTap-side prerequisites for the DspTap FFT plan (DspTap
docs/audit-fft-and-code-smells.md, Stage 1; review items P5 and P2).

CI flag typo. The Cortex-M55 "Ooura FFT fallback" leg passed
-DMUTAP_FFT_CMSIS=OFF; the option is TAP_DSP_FFT_CMSIS (DspTap's
CMakeLists.txt, and the name MuTap's own CMakeLists.txt and the M33
toolchain already use). CMake ignored the unknown variable, so that leg
rebuilt CMSIS and the Ooura-float32-on-M55 profile had no coverage. The
leg now passes the real option, tees the configure log and fails if the
"float32 FFT backend = CMSIS" line appears, and asserts the cache value.
The same misspelling is corrected in the icount job's comment,
docs/optimization.md and bench/README.md; optimization.md's pointers to
the FFT wrapper, its tests and the vendored CMSIS subset now name DspTap,
where they live, and its vDSP section records that MuTap turns
TAP_DSP_FFT_ACCELERATE back off (#31).

Stale bare-metal filter. tests/bare_metal_main.cpp and the Hexagon
selection in tests/CMakeLists.txt named real_fft_test/*,
RealFftCrossPrecision.* and CertifiedGeometries/fft_backend_parity.*,
which moved to DspTap with the FFT and no longer exist in mutap_tests.
Pruned; every remaining name was checked against the test sources, and
the host binary selects 52 tests under the pruned filter (the >= 30
empty-run guard is unchanged).

Fingerprint harness. tests/branchless_parity_check.cpp, until now the
only bit-exact gate in MuTap, becomes tests/fingerprint_harness.cpp
(CMake target mutap_fingerprint, a ctest test on every target): a fixed
xorshift-driven synthetic echo corpus, generated from integer state with
basic IEEE arithmetic only (no libm, no <random>, no clock, no files)
and rounded once to float so both profiles see identical values, runs
through fdaf, fd_kalman, pem_afc, the residual suppressor, the learned
suppressor and the two preset chains in float and double, printing one
"FINGERPRINT <component> <profile> <hex>" line per pair (FNV-1a over the
raw output bytes). Two runs on the same host and compiler are identical;
the pin-bump workflow is documented at the top of the file (build, run,
bump the pin, rebuild, run, diff). The branchless-parity job compiles
the same file twice as before and now diffs every FINGERPRINT line
instead of one field, and the hosted matrix legs record the lines in
their logs so two CI runs can be diffed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
ctest hides a passing test's output, so the M55 (CMSIS and Ooura), M33 and
Hexagon jobs re-run mutap_fingerprint verbosely after their battery: each
log now carries the FINGERPRINT lines for that target and backend, so the
two M55 backends and any two pins can be diffed from the logs. The hosted
matrix already had this step.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

@tap tap left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hostile review B (process / downstream)

Scope: ownership, truthfulness of the PR body against CI logs and git history, plan compliance (Stage 1 MuTap, Part 9 rule, Part 12 ownership), downstream rollout, CI hygiene, cross-PR hazards, commit hygiene. Checked against origin/main = 142361b (unchanged since the branch point; mergeable_state: clean), CI runs 35260033992 (pull_request) and 35260029726 (push), and a private clone of the branch with the submodule initialised at 5ca3b1c.

Verified (stated so the findings below are read against it)

  • Files touched: .github/workflows/ci.yml, bench/README.md, docs/optimization.md, tests/CMakeLists.txt, tests/bare_metal_main.cpp, tests/branchless_parity_check.cpp (deleted), tests/fingerprint_harness.cpp (new). Nothing under submodules/; gitlink 5ca3b1c on both HEAD and origin/main.
  • Both CI runs: 14/14 jobs success. The two runs for 7c361c3 alone were cancelled (superseded), so the first commit was never green on its own; irrelevant to merge.
  • M55 job log (105334833454): default leg prints tap::dsp: float32 FFT backend = CMSIS-DSP Helium; the Ooura leg's configure prints no backend line (DspTap only emits one when CMSIS or vDSP is ON), and TAP_DSP_FFT_CMSIS:BOOL=OFF is asserted from the cache. Note the :BOOL in that grep is what makes it robust: an unknown -D lands in the cache as :UNINITIALIZED, so a future rename of the option fails this leg instead of silently rebuilding CMSIS. That is the right check.
  • On-target fingerprints, CMSIS vs Ooura legs: all seven double lines identical (fdaf e447bc281b51e1a8, fd_kalman a80bc7372059460e, pem_afc 77243469100f445d, postfilter 5c64c11643043a90, nn_suppressor b2d4536c85729bb2, aec_chain c1236068de57e291, aec_chain_nn 4de0cc47d274591c); all seven float lines differ. Consistent with "double always keeps Ooura".
  • Local: mutap_fingerprint built Release (GCC 13.3), run twice, cmp identical, 14 FINGERPRINT lines + FINGERPRINT_COMPLETE, 0.71 s. ctest -N = 198 tests. mutap_tests --gtest_list_tests under the pruned bare-metal filter = 52. Full double suite not re-run here.
  • MUTAP_SUPPRESSOR_PARITY_FP: zero references remain.
  • No new uses: added; every uses: in ci.yml stays SHA-pinned. (docs.yml / style.yml use floating tags; pre-existing, outside this PR.)

Findings

S1 (should-fix) — Stale references to the deleted file survive in a file this PR owns and edited. docs/optimization.md:223 still says the branchless-parity CI job "compiles tests/branchless_parity_check.cpp once per macro value", and docs/optimization.md:205 still describes "an output fingerprint over a 600-block double-talk corpus"; the harness is now tests/fingerprint_harness.cpp with k_blocks = 400 and 14 lines. The rename is complete in ci.yml, tests/CMakeLists.txt and the PR body, but a reader of the suppressor section of optimization.md is sent to a file that no longer exists. Fix: rewrite lines 203–206 to name tests/fingerprint_harness.cpp / mutap_fingerprint, the 400-block corpus and "every FINGERPRINT line", and cross-reference the new bullet you added under "What is validated". (HANDOFF.md:531 mentions the old check inside a DONE record; leave it.)

S2 (should-fix) — The history statement is wrong in kind: not a typo, a rename that missed ci.yml. git log -S MUTAP_FFT_CMSIS: the flag was introduced by 1d8bd35 (2026-07-20, "Certify the ITU compliance battery at float32"), when MuTap's own root CMakeLists.txt declared option(MUTAP_FFT_CMSIS ...) — the leg was correct when written. 14116f0 (2026-07-21, "Consume the shared FFT from DspTap") renamed the option to TAP_DSP_FFT_CMSIS and did not touch ci.yml (git show 14116f0 -- .github/workflows/ci.yml | grep CMSIS is empty). So: the leg built Ooura-float32-on-M55 correctly between 1d8bd35 and 14116f0, and lost coverage from the DspTap extraction until this PR. Three places now say otherwise: ci.yml:184-186 ("once passed a misspelled -DMUTAP_FFT_CMSIS=OFF"), docs/optimization.md:60 (same wording), and the PR body ("has had no coverage", "the first time that leg actually builds the Ooura float32 backend on the M55"). The plan's P5 inherited the same framing, but this PR is where the record gets written. Fix: reword both comments to "the option was renamed from MUTAP_FFT_CMSIS to TAP_DSP_FFT_CMSIS when the FFT moved to DspTap (14116f0); this leg kept the old name, which CMake ignored, so from that commit until #50 it rebuilt CMSIS", and correct "first time" in the PR body to "first time since 14116f0". The lesson for future readers is different too: a :BOOL cache assertion catches a rename; a spelling review does not.

S3 (should-fix) — The pin-bump procedure is documented only where a bumper will not look. Part 9's rule makes this harness "the downstream bit-identity gate for every bump", and the eight-line procedure lives at the top of tests/fingerprint_harness.cpp — correct, but a bumper starts from .github/pull_request_template.md's "Submodule pin moved" bullet or HANDOFF.md working note 6 (the submodule dance), and neither mentions the harness; docs/optimization.md names it as "the gate every DspTap pin bump runs" without saying how. grep -rn fingerprint README.md HANDOFF.md .github/ is empty. Fix: one sentence in the PR template's "Submodule pin moved" bullet — "Paste the mutap_fingerprint diff (before/after the pin; procedure at the top of tests/fingerprint_harness.cpp) and say which lines the stage expects to move" — and/or the same in HANDOFF note 6. The template is not a Part 12 contention file; if you would rather not widen this PR's file list, say in the PR body that the template change is owed and by whom.

S4 (should-fix) — Each emulated leg now runs the harness twice; halve it. On bare metal the ctest pass criterion is FINGERPRINT_COMPLETE, which the verbose step checks equally well, so the run inside the battery adds nothing but time. Measured against the last main run (34366405585): Cortex-M33 job 5.4 min → 14.4 min (Test step 4:20 → 8:31, plus Fingerprints 3:59; the second PR run: 6:12 + 2:54); Cortex-M55 5.5 → 9.3 min (harness 49 s + 49 s CMSIS, 35 s + 35 s Ooura); Hexagon 28.4 → 34.0 min (Test 25:42 → 28:32, plus Fingerprints 2:52, against timeout-minutes: 60). All within the job timeouts and the 900 s per-test TIMEOUT (worst 240 s on M33 soft-float), so not a blocker, but Part 12 puts CI runs on the critical path. Fix: ctest --test-dir build --output-on-failure -E '^mutap_fingerprint$' for the battery step on the four emulated legs, then the existing -R '^mutap_fingerprint$' -V step; saves ~4 min M33, ~3 min Hexagon, ~1.5 min M55 per run. On the hosted matrix the double run costs 1 s and can stay. Cross-repo note: DspTap's #24 has the same "ctest hides passing output" problem for its informational parity target and DspTap's ci.yml still runs plain --output-on-failure; the -E / -R -V pair is the pattern both repos should share, rather than MuTap re-running and DspTap printing to nowhere.

S5 (should-fix) — Merge ordering and the baseline are not stated. Nothing in the PR body says this PR must merge before any DspTap pin bump, and DspTap #20 (Stage 0, "Bump MuTap immediately") is open now. The rollout rule wants every bump PR to say which FINGERPRINT lines are unchanged; the first bump can only do that if this PR's merge commit is on main first, because its main CI logs become the baseline for every leg (host x3, M55 CMSIS and Ooura, M33, Hexagon). Fix: add to "Notes for the reviewer": "Merge before the Stage 0 bump; the main run of this merge is the fingerprint baseline every later bump diffs against." Also MuTap-Max: submodules/MuTap is at 96b4572 (two commits behind main today, three after this merges). Nothing functional is owed — no header or API changed — but HANDOFF note 6's re-pin is now three behind; a one-line "MuTap-Max: nothing functional; re-pin rides the next Max PR" in the body keeps the drift visible.

N1 (nit) — set -eo pipefail is redundant. GitHub's default run: shell on Linux is bash --noprofile --norc -eo pipefail {0}. Harmless, and the existing parity job uses set -e the same way; either drop it or keep the repo consistent, but do not rely on it meaning anything.

N2 (nit) — The P18 dependency is described, not flagged. The parity job still compiles submodules/dsptap/third_party/ooura/fftsg.c / fftsg_float.c by path; the harness header and the PR body both describe this, and the plan owns it (P18 → Stage 2c, "CI job rewritten"), so it will not be lost. A one-line comment on the job — # Compiles DspTap's C by path; Stage 2c (P18) rewrites this job when the C goes. — would save the 2c agent a search.

N3 (nit) — Duplicate footer in the PR body. It ends with both the "🤖 Generated with [Claude Code]" block plus session URL and a second --- / _Generated by [Claude Code](...)_ line. One footer.

N4 (nit, ownership) — bench/README.md. One word outside the Part 12 list, disclosed in the body, and no other wave-1 MuTap PR exists to collide with (list_pull_requests: #50 is the only open one). Acceptable; the disclosure is what the rule asks for when no other owner exists.

Ownership / plan compliance summary

  • Part 12 list respected except the disclosed bench/README.md word (N4). Nothing under submodules/, include/ or the root CMakeLists.txt.
  • Stage 1 MuTap items: flag fixed with a rename-proof assertion (verified in the log); filter pruned to 52 tests with the ≥ 30 guard intact (verified locally); harness generalised over fdaf, fd_kalman, pem_afc, postfilter, nn_suppressor plus both preset chains, both profiles, runnable at any pin (verified deterministic locally); diff procedure documented in the file header (S3 for discoverability).
  • Gates table row 1 ("fallback leg builds Ooura float; filter pruned"): met.
  • Rebase: git log 142361b..origin/main is empty; clean.
  • Commits: two, both carrying Co-Authored-By and Claude-Session trailers; template sections filled with what was and was not run.

Verdict: no blocker; mergeable once S1–S3 and S5 (docs and PR-body wording) are addressed; S4 can land here or ride the next ci.yml-owner PR before the M33/Hexagon legs get any longer.


Generated by Claude Code

@tap tap left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hostile review A (correctness)

Reviewed at head 72e7e69 against a private clone (Release, -DMUTAP_WERROR=ON, GCC 13.3, x86-64). Evidence for every point below was produced, not inferred.

Findings

1. should-fix — docs/optimization.md now claims on-target CMSIS coverage that runs nowhere.
The rewritten "What is validated" bullets say DspTap's tests/test_fft_backend.cpp "asserts the CMSIS forward output matches a direct Ooura rdft_f reference bin-for-bin" and that the FFT suites "run in DspTap's CI". At the pinned DspTap (5ca3b1c) the only Arm leg is cortex-m55-cmsis (compile) with -DTAP_DSP_BUILD_TESTS=OFF (submodules/dsptap/.github/workflows/ci.yml:35,48), and the hosted DspTap legs cannot compile CMSIS. So CertifiedGeometries/fft_backend_parity on the CMSIS backend, which this PR (correctly) removes from MuTap's bare-metal filter because the test no longer exists in mutap_tests, is currently executed by no CI anywhere. The M55 CMSIS-vs-Ooura fingerprint diff is not a substitute: by construction every float line differs between the two legs (7/7, see item 8), so it asserts nothing about CMSIS accuracy. The prune is right; the documentation is not. Fix: state that the CMSIS bin-for-bin parity gate is unrun until DspTap's Stage 1 embedded legs land (plan Part 10), and that on the M55 the float32 battery (mutap_tests_emulated, 52 tests) is today the only CMSIS gate.

2. should-fix — stale count in the same paragraph. docs/optimization.md:110 still says "58/58 tests pass" for the M55 float32 battery; the PR itself changes the selection to 52 (tests/bare_metal_main.cpp), and 52 is what the host binary selects under the pruned filter (verified: --gtest_list_tests under the exact filter = 52 of 197). Update to 52/52 or drop the number.

3. should-fix — the harness does not hash "every output sample the component produced". partitioned_fdaf::process_block and partitioned_fdkf::process_block have a fourth output, Sample* estimate = nullptr (fdaf.h:164, fd_kalman.h:272); the harness calls the 3-argument form, so the echo-estimate channel of fdaf and fd_kalman is never mixed. estimate[i] = y and error[i] = desired[i] - y share the same y (fdaf.h:193-195), but a 1-ulp move in y is absorbed by the subtraction whenever |desired| >> |y| (loud near end, quiet echo), so a bump can change the estimate stream while leaving the error stream, and therefore the fingerprint, unchanged. aec_chain does consume that channel (postfilter.h:938) but only through the suppressor, which is not injective either. Fix: give run a second scratch buffer and, for the two cancellers, call the 4-argument form and fp.mix both buffers. Header wording should then match what is hashed.

4. should-fix — the Ooura-leg guard is an absence check plus a cache check; neither proves what was compiled. grep -q 'float32 FFT backend = CMSIS' passes vacuously if DspTap ever rewords its status line (with the option OFF DspTap prints no backend line at all — verified on a host configure, and visible in the M55 log: the Ooura configure step emits nothing about a backend). grep '^TAP_DSP_FFT_CMSIS:BOOL=OFF' build-ooura/CMakeCache.txt does hold (verified: an untyped -D becomes :BOOL once option() runs, and the M55 log prints TAP_DSP_FFT_CMSIS:BOOL=OFF), but it checks the option, not the TAP_DSP_FFT_CMSIS compile definition the header actually keys on. Concrete fix, and it is one line: have the harness print the backend it was compiled with in its # header (backend=cmsis|vdsp|ooura from TAP_DSP_FFT_CMSIS/TAP_DSP_FFT_ACCELERATE), and have the "Fingerprints (Ooura FFT fallback)" step grep -q 'backend=ooura' its own output. That is a positive, on-target assertion about the binary that ran, and it makes every fingerprint log self-describing for cross-pin diffs, which the current header (branchless=1 on both M55 legs) does not.

5. should-fix — branchless-parity pipes without pipefail. The job runs under GitHub's default bash -e {0} (confirmed in the job log: shell: /usr/bin/bash -e {0}; pipefail is only default when shell: bash is set explicitly, which this workflow never does — the explicit set -eo pipefail you added to the Ooura configure step is therefore correct and necessary, not redundant). In the parity job /tmp/parity_$bl | tee /tmp/out_$bl masks a non-zero exit of the harness; the >= 14 floor catches a crash before the 14th line but not one after it (e.g. in teardown), and it is only applied to fp_0. Add pipefail and floor both files.

6. nit — STYLE.md deviations in tests/fingerprint_harness.cpp. (a) std::vector<Sample> x, y, e, yhat; is four declarations on one line; (b) corpus_source orders members public -> private -> public (NL.16 says public data last, after functions, and never a second public block after private; either make them m_-prefixed with accessors or move the block up); (c) profile_name decides by sizeof(Sample) == sizeof(float) — use std::is_same_v<Sample, float>; (d) unsigned long long value() where the member is std::uint64_t (cast at the printf, not in the accessor); (e) std::move without <utility>. clang-format --dry-run --Werror and clang-tidy (repo config) are both clean on the file, so none of these is caught by tooling.

7. nit — corpus starts in double talk. (blk / 40) % 3 == 0 puts the near end on for blocks 0-39, so every canceller's initial convergence is under double talk. It is deterministic and it exercises the freeze paths, so it is not a bug, but it is the opposite of the "converge, then double talk" sequence the header comment describes ("carry every canceller through convergence and into the double-talk / quiet segments"). Either shift the phase ((blk / 40) % 3 == 1) or fix the comment.

What was verified and held

  1. On-target claim (M55, CMSIS vs Ooura). Diffed the two "Fingerprints" steps of job 105334859868 myself: all 7 double lines identical (e447bc28…, a80bc737…, 77243469…, 5c64c116…, b2d4536c…, c1236068…, 4de0cc47…), all 7 float lines differ. Both legs print branchless=1 and 16 hex digits (newlib's %llx is fine under rdimon.specs). The Ooura configure step's cache grep printed TAP_DSP_FFT_CMSIS:BOOL=OFF; the CMSIS configure printed tap::dsp: float32 FFT backend = CMSIS-DSP Helium and the Ooura one printed no backend line.
  2. Discrimination (mutation). Compiled the harness against a copy of fft.h whose float ooura_rdft adds 1 ulp to a[2] (Re bin 1) after the forward: all 7 float lines changed, all 7 double lines unchanged. The mirror mutation on the double overload: 7 double lines changed, 0 float. The hash is FNV-1a over sizeof(Sample) raw bytes per sample (memcpy), so it hashes float bytes in the float profile and double bytes in the double profile; NaN payloads and -0.0 are distinguished.
  3. Determinism claims. No std::sin/cos/exp/pow/sqrt/log on the corpus path (the removed branchless_parity_check.cpp used std::sin/std::fabs; the new corpus is integer xorshift plus + - * in double). xorshift32::next maps (m_s >> 8) - 2^23 times 2^-23 into [-1, 1) with a 24-bit significand, exactly representable in float; the double profile widens static_cast<float>(…) exactly, so both profiles do consume identical samples. Delay-line indices 128/256/384 are inside k_history = 385. Two runs from the CMake build were byte-identical; the standalone g++ -O2 build of the parity job (reproduced locally for MUTAP_SUPPRESSOR_BRANCHLESS=0 and 1 against DspTap's fftsg.o/fftsg_float.o) produced 14 identical lines, identical to the CMake build and identical to the ubuntu-latest CI run (job 105334859825).
  4. Geometry. block 256 / 8 partitions / 48 kHz matches bench/icount/icount_main.cpp:53 (k_geo{48000.0, 256, 8}) and the nn geometry {48000.0, 256, 26, 64, 96} matches icount_main.cpp:105, test_nn_suppressor.cpp:208 and tools/ml/features.py:73.
  5. Filter pruning. real_fft_test, RealFftCrossPrecision, CertifiedGeometries, fft_backend_parity absent from mutap_tests --gtest_list_tests; 52 selected under the bare-metal filter; the >= 30 guard is 22 below that. bare_metal_main.cpp and the cross-compile TEST_FILTER list the same 24 suites. The 52 selected tests pass on the host (4.2 s).
  6. Bare metal. PASS_REGULAR_EXPRESSION "FINGERPRINT_COMPLETE" is printed only after all 14 lines; no allocation inside the 400-block loop (corpus_source and out allocate once per component); M33 soft-float double run is 174 s against the 900 s timeout, M55 38 s (CMSIS) / 27 s (Ooura). -R '^mutap_fingerprint$' cannot collide with a gtest-discovered name. ci.yml parses as YAML; 11 jobs.
  7. -Werror build with the house warning set: 0 warnings in the private build. Local -DTAP_DSP_FFT_CMSIS=OFF host configure: cache entry is TAP_DSP_FFT_CMSIS:BOOL=OFF, so the grep is exact for the -D path as well as the toolchain-file CACHE BOOL path (M33).
  8. Full host suite: 198/198 passed in 434 s (ctest -j4, Release, GCC 13.3), including mutap_fingerprint.

Verdict

No blocker. Merge after 1-4 (docs honesty about CMSIS coverage and the stale 58/58; hash the estimate channel; make the Ooura-leg assertion positive); 5-7 at the author's discretion.


Generated by Claude Code

…rt, docs

Hostile reviews A (correctness) and B (process) on #50.

Harness (A#3, A#6, A#7). fdaf and fd_kalman now run the four-argument
process_block and both output channels are hashed — the error block and
the echo-estimate block — because error = desired - estimate can absorb a
one-ULP move of the estimate under a loud near end. All 14 fingerprints
change as a consequence (expected; the PR body records the new values).
The '#' header prints backend=cmsis|vdsp|ooura from the TAP_DSP_FFT_CMSIS /
TAP_DSP_FFT_ACCELERATE compile definitions the FFT header keys on, so every
log is self-describing. Double talk now starts at block 40 so each
canceller converges in single talk first, as the comment always said.
Style: one declaration per line, corpus buffers private with accessors
(NL.16 order), std::is_same_v for the profile name, value() returns
std::uint64_t (cast at the printf), <utility> included.

CI (A#4, A#5, B#S4). The Ooura-fallback leg keeps the :BOOL=OFF cache
assertion and additionally greps backend=ooura from the harness output of
the binary that ran on target — a positive proof of the compiled backend,
not an absence check; the CMSIS leg asserts backend=cmsis, M33 and Hexagon
backend=ooura. The four emulated legs run the battery with
-E '^mutap_fingerprint$' and the harness once, verbosely, in the
Fingerprints step (saves ~4 min M33, ~3 min Hexagon, ~1.5 min M55 per
run). The parity job runs under set -eo pipefail — GitHub's default run
shell is `bash -e {0}` without pipefail, so the pipes through tee would
otherwise mask a harness crash — and floors both files at 14 lines; a
comment marks it as the Stage 2c (P18) rewrite site.

Docs (A#1, A#2, B#S1, B#S2, B#S3). optimization.md: the CMSIS bin-for-bin
parity gate is executed by no CI at the current DspTap pin (DspTap's M55
leg is compile-only with tests off) and will be by DspTap's own M55 leg
once DspTap #17 lands; the M55 float32 battery is 52/52, not 58/58; the
suppressor section names tests/fingerprint_harness.cpp, 400 blocks and
14 lines instead of the deleted file and a 600-block corpus; the flag
history is a missed rename at the DspTap extraction (14116f0), not a
typo, in ci.yml and the doc alike. HANDOFF note 6 and the PR template's
"Submodule pin moved" bullet each point a pin bumper at the harness
header's diff procedure.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

tap commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Fix pass for both hostile reviews landed in b2ca86c. Every finding, applied or declined:

Review A (correctness)

Review B (process / downstream)

  • S1 stale references — applied. The suppressor section names tests/fingerprint_harness.cpp / mutap_fingerprint, the 400-block corpus and "every FINGERPRINT line, 14 of them", and cross-references the "What is validated" bullet. (HANDOFF's DONE record left alone, as suggested.)
  • S2 history — applied. ci.yml, docs/optimization.md and the PR body now say the option was renamed from MUTAP_FFT_CMSIS to TAP_DSP_FFT_CMSIS at the DspTap extraction (14116f0), the leg kept the old name, and coverage was lost from that commit until Stage 1 prerequisites: M55 Ooura-leg flag typo, stale bare-metal filter, fingerprint harness #50; "first time" became "first time since 14116f0". The :BOOL point (a typed cache assertion catches a rename; a spelling review does not) is in the ci.yml comment.
  • S3 discoverability — applied, both places: one sentence in HANDOFF working note 6 and in the PR template's "Submodule pin moved" bullet, each pointing at the procedure at the top of tests/fingerprint_harness.cpp and asking the bump PR to say which lines the stage expects to move. Both files are outside the wave-1 list and are disclosed in the PR body.
  • S4 harness run twice — applied. The four emulated legs run the battery with -E '^mutap_fingerprint$' and the harness once, verbosely, in the Fingerprints step. The hosted matrix keeps the 1 s double run.
  • S5 merge ordering / baseline — applied in "Notes for the reviewer": merge before the Stage 0 bump (DspTap TCLwdt over time: three-coupling trace, notebook figure, corrected story #20); the main run of this merge is the fingerprint baseline for every leg; MuTap-Max's submodules/MuTap drift (96b4572, three behind after this merges) noted as nothing functional, re-pin rides the next Max PR.
  • N1 set -eo pipefail redundant — declined. Review A ITU compliance for the AEC: the Rev 5 plan and the Stage 0 requirements matrix #5 checked the job log: the workflow never sets shell: bash, so the default is bash -e {0} without pipefail, and the explicit set is load-bearing. Kept, and added to the parity job and every tee'd Fingerprints step.
  • N2 P18 flag — applied. Comment on the parity job: compiles DspTap's C by path; Stage 2c (P18) rewrites this job when the C goes.
  • N3 duplicate footer — applied. One footer.
  • N4 bench/README.md — no action beyond the existing disclosure.

Verification at b2ca86c: -DMUTAP_WERROR=ON build clean; mutap_fingerprint twice, byte-identical; the parity job's standalone build for both macro values with the house warning set plus -Werror, 14 lines identical to each other and to the CMake build; float suites (52-test emulated selection plus every float-typed hosted suite and Float32Parity, 89 tests) 89/89 in 334 s; pinned pre-commit clean; ci.yml parses. The emulated legs (both M55 Fingerprints steps must now show backend= and the new hashes, M33 and Hexagon backend=ooura) are proven by this push's CI run.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy


Generated by Claude Code

@tap
tap merged commit 30bfceb into main Sep 17, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant