Stage 1 prerequisites: M55 Ooura-leg flag typo, stale bare-metal filter, fingerprint harness - #50
Conversation
…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
left a comment
There was a problem hiding this comment.
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 undersubmodules/; gitlink5ca3b1con bothHEADandorigin/main. - Both CI runs: 14/14 jobs success. The two runs for
7c361c3alone 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), andTAP_DSP_FFT_CMSIS:BOOL=OFFis asserted from the cache. Note the:BOOLin that grep is what makes it robust: an unknown-Dlands 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
doublelines identical (fdaf e447bc281b51e1a8,fd_kalman a80bc7372059460e,pem_afc 77243469100f445d,postfilter 5c64c11643043a90,nn_suppressor b2d4536c85729bb2,aec_chain c1236068de57e291,aec_chain_nn 4de0cc47d274591c); all sevenfloatlines differ. Consistent with "double always keeps Ooura". - Local:
mutap_fingerprintbuilt Release (GCC 13.3), run twice,cmpidentical, 14 FINGERPRINT lines +FINGERPRINT_COMPLETE, 0.71 s.ctest -N= 198 tests.mutap_tests --gtest_list_testsunder the pruned bare-metal filter = 52. Full double suite not re-run here. MUTAP_SUPPRESSOR_PARITY_FP: zero references remain.- No new
uses:added; everyuses:inci.ymlstays SHA-pinned. (docs.yml/style.ymluse 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.mdword (N4). Nothing undersubmodules/,include/or the rootCMakeLists.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/mainis empty; clean. - Commits: two, both carrying
Co-Authored-ByandClaude-Sessiontrailers; 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
left a comment
There was a problem hiding this comment.
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
- On-target claim (M55, CMSIS vs Ooura). Diffed the two "Fingerprints" steps of job 105334859868 myself: all 7
doublelines identical (e447bc28…,a80bc737…,77243469…,5c64c116…,b2d4536c…,c1236068…,4de0cc47…), all 7floatlines differ. Both legs printbranchless=1and 16 hex digits (newlib's%llxis fine underrdimon.specs). The Ooura configure step's cache grep printedTAP_DSP_FFT_CMSIS:BOOL=OFF; the CMSIS configure printedtap::dsp: float32 FFT backend = CMSIS-DSP Heliumand the Ooura one printed no backend line. - Discrimination (mutation). Compiled the harness against a copy of
fft.hwhose floatooura_rdftadds 1 ulp toa[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 oversizeof(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.0are distinguished. - Determinism claims. No
std::sin/cos/exp/pow/sqrt/logon the corpus path (the removedbranchless_parity_check.cppusedstd::sin/std::fabs; the new corpus is integer xorshift plus+ - *in double).xorshift32::nextmaps(m_s >> 8) - 2^23times2^-23into[-1, 1)with a 24-bit significand, exactly representable in float; the double profile widensstatic_cast<float>(…)exactly, so both profiles do consume identical samples. Delay-line indices 128/256/384 are insidek_history = 385. Two runs from the CMake build were byte-identical; the standaloneg++ -O2build of the parity job (reproduced locally forMUTAP_SUPPRESSOR_BRANCHLESS=0and1against DspTap'sfftsg.o/fftsg_float.o) produced 14 identical lines, identical to the CMake build and identical to the ubuntu-latest CI run (job 105334859825). - 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}matchesicount_main.cpp:105,test_nn_suppressor.cpp:208andtools/ml/features.py:73. - Filter pruning.
real_fft_test,RealFftCrossPrecision,CertifiedGeometries,fft_backend_parityabsent frommutap_tests --gtest_list_tests; 52 selected under the bare-metal filter; the>= 30guard is 22 below that.bare_metal_main.cppand the cross-compileTEST_FILTERlist the same 24 suites. The 52 selected tests pass on the host (4.2 s). - Bare metal.
PASS_REGULAR_EXPRESSION "FINGERPRINT_COMPLETE"is printed only after all 14 lines; no allocation inside the 400-block loop (corpus_sourceandoutallocate 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.ymlparses as YAML; 11 jobs. -Werrorbuild with the house warning set: 0 warnings in the private build. Local-DTAP_DSP_FFT_CMSIS=OFFhost configure: cache entry isTAP_DSP_FFT_CMSIS:BOOL=OFF, so the grep is exact for the-Dpath as well as the toolchain-fileCACHE BOOLpath (M33).- Full host suite: 198/198 passed in 434 s (
ctest -j4, Release, GCC 13.3), includingmutap_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
|
Fix pass for both hostile reviews landed in b2ca86c. Every finding, applied or declined: Review A (correctness)
Review B (process / downstream)
Verification at b2ca86c: 🤖 Generated with Claude Code https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy Generated by Claude Code |
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, andtests/branchless_parity_check.cppgeneralized intotests/fingerprint_harness.cpp, the bit-identity gate every DspTap pin bump runs.Why
-DMUTAP_FFT_CMSIS=OFF. That was the option's name when the leg was written (1d8bd35); the DspTap extraction (14116f0) renamed it toTAP_DSP_FFT_CMSISand did not touchci.yml, so from14116f0until 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 entryTAP_DSP_FFT_CMSIS:BOOL=OFF(an unknown-Dlands as:UNINITIALIZED, so a future rename fails the leg instead of silently rebuilding CMSIS), and its Fingerprints step grepsbackend=oourafrom the harness binary that actually ran on target — a positive proof of the compiled backend. The CMSIS leg assertsbackend=cmsis, M33 and Hexagonbackend=ooura. The same stale name is fixed in the icount job comment,docs/optimization.mdandbench/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 turnsTAP_DSP_FFT_ACCELERATEback off (macOS float32 flake: Apple vDSP's real FFT is non-deterministic across processes at N=2048 #31).tests/bare_metal_main.cppand the cross-compileTEST_FILTERintests/CMakeLists.txtnamedreal_fft_test/*,RealFftCrossPrecision.*andCertifiedGeometries/fft_backend_parity.*, none of which exist inmutap_testsany more (gtest silently selects nothing for them). Pruned; every remaining name was grepped againsttests/test_*.cpp(each found in exactly one file), and the host binary selects 52 tests under the pruned filter, so the>= 30empty-run guard is untouched. Honest consequence, now stated inoptimization.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.mutap_fingerprint, printsFINGERPRINT <component> <profile> <hex>forfdaf,fd_kalman(both with the error block AND the echo-estimate block hashed, via the four-argumentprocess_block, becauseerror = desired - estimatecan 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_chainandaec_chain_nn(both presets), infloatanddouble— 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. Thebranchless-parityjob compiles the same file twice exactly as before (DspTap'sfftsg.c/fftsg_float.cby path — Stage 2c/P18 rewrites that job) underset -eo pipefailand diffs every FINGERPRINT line with a 14-line floor on both files; every leg, hosted and emulated, has aFingerprintsstep 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 forMUTAP_SUPPRESSOR_BRANCHLESS=0and=1):(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
mainrun 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-Werrorwith the house warning set.ctest --test-dir build --output-on-failure -j4: full suite, 198/198 passed in 587 s at 72e7e69 (includingmutap_fingerprint); at this head the float suites (the 52-test emulated selection plus every float-typed hosted suite andFloat32Parity) were re-run — the harness is the only compiled change since.mutap_fingerprintrun 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 -Werroradded) forMUTAP_SUPPRESSOR_BRANCHLESS=0and=1: all 14 FINGERPRINT lines identical to each other and to the CMake build's. ~0.7 s per run.mutap_tests --gtest_list_testsunder the pruned filter: 52 tests selected.pre-commit run --all-files(the pinned clang-format v18.1.3 hook): passed;ci.ymlparses (11 jobs).Not run locally — CI is the first real gate for these:
14116f0that the leg builds the Ooura float32 backend on the M55.mutap_fingerprintunder emulation once each, verbosely, and assertbackend=; 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 sevendoublelines identical between CMSIS and Ooura, the sevenfloatlines all different.Notes for the reviewer
submodules/dsptapstays at5ca3b1c). Notebooks untouched (no behavior changed).mainCI 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/submodules/MuTapis at96b4572, three commits behindmainonce 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.mdwas touched for one word (the option name) although it was outside this item's file list; it was the last remainingMUTAP_FFT_CMSISin the tree..github/pull_request_template.mdandHANDOFF.mdeach gained one sentence pointing pin bumpers at the harness (review B S3).MUTAP_SUPPRESSOR_PARITY_FPoutput is gone; the parity job's comparison moved to adiffof the FINGERPRINT lines.ci.yml,tests/CMakeLists.txt,tests/bare_metal_main.cpp,tests/fingerprint_harness.cppanddocs/optimization.mdin wave 1; nothing undersubmodules/,include/or the rootCMakeLists.txtchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy