Stage 1b: FFT benchmark scenarios and the instruction-count ratchet scaffold - #21
Conversation
|
Status of the red Bench jobs on this draft, after
Nothing else to do here until #17 lands; then this PR rebases, the five jobs run as seeding runs, and Generated by Claude Code |
tap
left a comment
tap
left a comment
There was a problem hiding this comment.
Hostile review B (process / downstream)
Reviewed 847987f against origin/main (5ca3b1c), the plan (Stage 1b, Parts 11/12, D11), the Bench workflow logs for all four runs, the legs branch (1505e41), and MuTap 142361b. Built in a private scratch dir with -DTAP_DSP_BUILD_BENCH=ON -DTAP_DSP_WERROR=ON: warning-free, 160/160 tests pass, each tap_dsp_icount_* prints the same checksum on two runs (0x446b9819, 0xc7aa23fa, 0x408d7334f8ae008e), -DTAP_DSP_BENCH_ENGINE=split_radix stops at the documented static_assert.
Findings
1. should-fix — "plugin API v2" is a false number, in three places. The pinned header is genuinely QEMU v8.2.2 (git ls-remote tag v8.2.2^{} = 11aa0b1ff…, sha256 matches c53a2af1…), but that header defines #define QEMU_PLUGIN_VERSION 1 (line 57) and has no scoreboard API. bench.yml (job comment and the env: comment), bench/README.md ("8.2.2, plugin API v2") and tools/qemu_insn_plugin/insn_count.c ("API v2") all say v2. Inherited verbatim from MuTap's copy, which carries the same error. In a repo whose discipline is "numbers, never remembered", fix it: "plugin API v1 (QEMU_PLUGIN_VERSION 1 in the pinned header)". Harmless at runtime (the plugin declares whatever the header says), wrong as documentation.
2. should-fix — the seeding path is a gate bypass, and nothing detects it. Seeded? decides by truthiness of d.get(key). A PR that changes "m33": {...} to "m33": {} gets a green run: the job switches to --update, prints, uploads, exits 0. bench/README.md forbids it ("A key is never emptied to 'reset' it") but the rule has no teeth. Concrete fix, in the Seeded? step on pull_request events:
base=$(git show "${{ github.event.pull_request.base.sha }}:bench/baselines.json" 2>/dev/null || echo '{}')
python3 - "$base" <<'EOF'
import json,sys; b=json.loads(sys.argv[1]); h=json.load(open('bench/baselines.json')); k='${{ matrix.key }}'
if b.get(k) and not h.get(k): print(f'::error::{k}: baselines emptied in this PR; re-record with --update and write the reason in bench/README.md'); sys.exit(1)
EOF(needs fetch-depth: 0 or an explicit git fetch origin $BASE_SHA). Deleting a single scenario key is already caught (NO BASELINE fails without --update); the whole-dict case is the hole.
3. should-fix — seed-summary is green when the whole matrix is red. Evidence: both runs on 847987f (35262334386, 35262337354) show five failed icount <key> jobs and a successful "Merge seeding artifacts", because if: ${{ !cancelled() }} plus continue-on-error on the download. Either gate it on needs.icount-ratchet.result == 'success' or make the Merge step fail when that result is not success, so a partial seed can never be merged from a half-red run. It must never be a required check; the five icount <key> jobs are the right required checks once seeded.
4. should-fix — where the seeding commit's SHA comes from. README: "fills the table above with the run's commit". The workflow runs on PR heads too, and a PR-head SHA stops resolving after the rebase/squash + branch-delete flow in CLAUDE.md. State that the seed is taken from the push run on main after #17 and this PR merge (record the run URL and the main SHA), never from a PR run.
5. should-fix — duplicate runs, no concurrency group. push: + pull_request: produced two identical six-job runs one second apart on every push (runs 35262334386 push / 35262337354 pull_request; same for the first commit). Five QEMU jobs each. MuTap's ci.yml has concurrency: {group: ci-${{ github.workflow }}-${{ github.ref }}, cancel-in-progress: true}; DspTap's ci.yml on main does not, so this is house drift, but the bench workflow is the expensive one. Add the block; optionally push: branches: [main].
6. should-fix — the GPL header is unrecorded. qemu-plugin.h is SPDX-License-Identifier: GPL-2.0-or-later (lines 1–8 of the pinned file). It is not vendored, fetched at CI time, and the resulting .so is a CI-only tool that is never distributed; insn_count.c is MIT (GPL-compatible). Under this repo's NOTICE.md convention (bundled code under third_party/, with VENDOR.md for provenance) nothing in-tree is GPL, so no NOTICE.md entry is required — the same posture as MuTap's THIRD_PARTY_NOTICES.md ("Nothing here is GPL- or LGPL-encumbered" plus its build-time-tools section). But write the sentence down: one line in bench/README.md "Provenance" and in the insn_count.c header ("compiled against QEMU's GPL-2.0-or-later qemu-plugin.h, fetched at build time, not vendored; the plugin is a CI tool and is not shipped"). Otherwise the next reader sees a GPL header being compiled in CI with no record of why that is fine.
7. should-fix, but a request to the NOTICE owner, not an edit here. Answer to the open question: the MuTap-copied files need no third-party entry (MIT, same copyright family, notice retained: insn_count.c carries "Copyright 2026 MuTap contributors"). What is wrong is NOTICE.md's first sentence, which enumerates DspTap's own MIT code. The docs-provenance branch (f76abce) rewrites it to include/tap/dsp/, tests/, tools/, notebooks/, the build files — bench/ and scripts/ are absent. NOTICE.md is a wave-1 contention file owned by docs-provenance: send them "add bench/ and scripts/"; this PR stays out of NOTICE.md.
8. nit — scripts/icount.py has no SPDX or copyright line. Every other new file here has one; MuTap's original lacks it too. Add # SPDX-License-Identifier: MIT and the copyright lines under the shebang.
9. should-fix (documentation decision) — the size gate's build type is undecided. This job builds Release (-O2); the legs' ci.yml size step and Part 10 item 4's numbers (912868 B etc.) are MinSizeRel. Part 11 puts the .text ceilings "in the same job", which makes them -O2 numbers, not comparable to ci.yml's. Say in bench/README.md which build type the ceilings will be recorded at (recommend: this job, Release, so count and size describe the same object) — the ceilings are a promised item, not shipped, and the README's "once the legs are seeded" is the only schedule; name the wave.
10. nit — Part 11 policy scorecard. Implemented: ±3% two-sided (--tolerance 0.03, both directions fail), runs on push and PR, red = exit 1, --merge, engine hook with static_assert, "what each stage catches" table, sharing note, header digest-verified, host bench labelled informational in the README, the source comment and the PR body. Promised only: .text ceilings (9), the C-vs-port dual build (Stage 2a, correctly deferred), the seed itself (wave 2 per Part 12, consistent). "--update is a written commit" is policy text CI cannot enforce; the "Recorded baselines" table is the only record, so give the seeding/update commit a fixed shape there (key, before, after, delta, reason, run URL).
11. nit — no permissions: block. Artifact upload/download use the runtime token and need no GITHUB_TOKEN scope; add permissions: contents: read at the top of the new file. main's ci.yml lacks it too (house drift), but this file is new.
12. Truthfulness audit (report). PR-body claims checked and reproduced: 160/160; bit-identical checksums; git merge-tree conflict-free against the legs branch — and against all six other wave-1 branches (stage0-counter, stage2a-tests, stage3a-traits, stage5-spectrum, capi-fft, docs-provenance); no other wave-1 branch touches root CMakeLists.txt; toolchain names and variables match legs 1505e41 exactly (cmake/arm-cortex-m4-mps2.cmake + TAP_DSP_M4_FPU, arm-cortex-m33-mps2.cmake, arm-cortex-m55-mps3.cmake, TAP_DSP_FFT_CMSIS OFF set in the M4/M33 toolchain files); the merged README Build section reads correctly (legs' QEMU paragraph, then the bench line). The two PR comments match the logs: the first run's M55 jobs failed at compile on PRIx64 with <cinttypes> included (job 105325847556), consistent with the newlib __STDC_FORMAT_MACROS explanation; after 847987f the M55 keys fail on platform/armv8m_startup.c (cc1 invoked from the old toolchain file's link line — "fail only at link" is accurate operationally) and the M4/M33 keys at configure on missing toolchain files. Host numbers are labelled informational everywhere. The seeding description in the README matches what the workflow does step for step. The only false statement found is item 1.
13. nit — hygiene. PR body carries both the "🤖 Generated with" block and a second _Generated by [Claude Code]_ footer; keep one. Both commits carry the Co-Authored-By and Claude-Session trailers; commit 1's subject is 75 characters (no STYLE.md rule found; squashed on merge anyway). Ownership: every touched path is within the allowed set; the root CMakeLists.txt change is the option plus a guarded add_subdirectory and a comment; the README change is one paragraph in Build.
14. Cross-PR, merge position, un-draft checklist. Hard dependency on #17 only; independent of everything else. Merge immediately after #17 and before the 2a port branch is cut, so the port inherits the engine hook. Un-draft when: (a) rebased on main with #17 in; (b) the five icount <key> jobs are green as seeding runs on this PR — that is the first proof of the cross build, the plugin loading against the v8.2.2 header, semihosting printf, and TAP_DSP_INSN_COUNT being captured; (c) items 1–6 and 8–9 applied; (d) item 7 sent to docs-provenance. After merge: the seeding commit from the main push run (4); only then make the five icount <key> jobs required checks, never "Merge seeding artifacts".
15. Downstream, MuTap (report only, out of scope here). No double counting: MuTap's ratchet counts chain-level binaries (fdkf, suppressor, chain, …) on m55/m33/hexagon against its own baselines.json; this one counts the primitive alone. Two real risks: (i) creep — both gates are ±3% per PR with no cumulative memory, so a primitive change at +2.9% passes here and moves MuTap's chain by less, which also passes; ratchets stop steps, not slopes, and the only mitigation is the written --update table here plus MuTap's pin-bump commit citing the DspTap delta; (ii) MuTap's icount-ratchet runs on ubuntu-latest with the identical header pin (same URL and sha256), so when ubuntu-latest leaves 24.04 MuTap's plugin/QEMU pair breaks first — DspTap's ubuntu-24.04 pin is right and MuTap should copy it. MuTap bench/README.md should gain one line: the FFT's own per-target gate lives in DspTap bench/README.md, and a DspTap pin bump that moves MuTap's counts cites that ratchet's delta in the update commit. MuTap's insn_count.c and bench/README.md carry the same "API v2" error as item 1.
Verdict: not mergeable yet, and correctly a draft — no blocker in the shipped code; fix 1–6 and 8–9, hand 7 to the NOTICE owner, then merge directly after #17 once the five icount jobs seed green.
Generated by Claude Code
tap
left a comment
tap
left a comment
There was a problem hiding this comment.
Hostile review A (correctness)
Reviewed at 847987f against origin/main (5ca3b1c). Built in a private scratch dir with -DTAP_DSP_WERROR=ON -DTAP_DSP_BUILD_BENCH=ON (GCC 13.3, x86-64): clean. All three tap_dsp_icount_* binaries run twice with identical checksums (0x446b9819, 0xc7aa23fa, 0x408d7334f8ae008e), bench_fft runs. Callgrind, objdump, a perturbation experiment, a stubbed-measure harness for icount.py, a CMAKE_SYSTEM_NAME=Generic dry configure, clang-format/clang-tidy with the house configs, and the Bench logs of run 35262337354 were used as evidence below.
Findings
1. should-fix — the "checksum" is a float running sum and absorbs isolated output differences.
Same workload as rfft_f32_512, three variants, one ulp via nextafterf:
| perturbation | checksum |
|---|---|
| none | 0x446b9819 |
| one spectrum bin + one output sample nudged 1 ulp at iteration 1000 | 0x446b9819 (unchanged) |
| 64 spectrum bins nudged 1 ulp at iteration 1000 | 0x446b9819 (unchanged) |
| every output of every iteration nudged 1 ulp | 0x446bb1d8 |
The accumulator ends at ≈942.4, so ulp(acc) ≈ 6e-5 while a spectrum bin's ulp is ≈5e-7: a difference has to be ~100 ulp before it can register at all. So the fold consumes every output (true) but the printed value does not depend on every output bit, which is how the PR body and bench/README.md ("folding every output into a checksum") read. Determinism and DCE defeat are delivered (verified: two runs identical; objdump of main shows both rdft_f calls and both fold loops inside the 2048-iteration loop, cmp $0x800,%rbp), but this value cannot serve as the C-vs-port fingerprint Stage 2a will want on the QEMU legs, where the host parity TU does not run.
Fix: fold bit patterns with integer arithmetic — memcpy each sample to uint32_t/uint64_t and mix (h = (h ^ bits) * 0x9E3779B1u, FNV-1a style). Exact, order-sensitive, 3–4 integer instructions per element on every leg, and it removes 2·2^20 __aeabi_fadd calls from the m4-softfp count. Reword README/comments to match whichever fold you keep.
2. should-fix — nothing on host CI compiles or tidies bench/.
ci.yml line 26 and style.yml line 23 configure without TAP_DSP_BUILD_BENCH, so compile_commands.json has no bench TU and the green clang-tidy check on this PR is vacuous for its C++. (I ran clang-tidy-18 with the house .clang-tidy on both TUs from a BENCH=ON compile database: clean today — but nothing keeps it so.) The README's host smoke test ("two runs of the same binary must print the same checksum") has no CI home either, and bench_common.h never compiles against the vDSP backend on the macOS job.
Fix: add -DTAP_DSP_BUILD_BENCH=ON to the host matrix configure in ci.yml and to style.yml's configure; in bench/icount/CMakeLists.txt, when not cross-compiling, add_test per scenario that runs the binary twice and diffs the TAP_DSP_ICOUNT_DONE lines (or at least PASS_REGULAR_EXPRESSION "ok=1").
3. should-fix — the seeding path rewards emptying a key.
bench.yml "Seeded?" reads the PR's own checkout; a PR that sets "m55": {} (or deletes the key — d.get() → None → 'false' too) switches that key to record-and-upload mode and goes green with only a ::warning. bench/README.md says "A key is never emptied to reset it" and D11 says an accepted regression is a written commit; nothing enforces either.
Fix: allow the Seed step only when github.event_name != 'pull_request' (push to main, or a workflow_dispatch trigger this workflow should gain anyway for re-recording); on a pull_request, an empty key is a failing step whose message carries the seed instructions. Wave-2 seeding still works — it happens on the push after #17 merges.
4. should-fix — scenario-set drift is half-handled (scripts/icount.py + bench.yml).
With measure stubbed:
- (E) a baseline entry with no binary (renamed/removed workload) passes silently in compare mode, rc=0; the dead gate entry lingers until someone runs
--update. - (F) a new binary with no entry fails compare with
NO BASELINE, and the workflow has no path to record it: the Seed step fires only when the whole key is empty. Stage 3b'srfft_q15_512/rfft_q31_*therefore need counts produced outside CI, contradicting "measured on CI, never typed in". - (G)
--updatereturns 0 after recording a +400 % regression — fine as the record mode, but the seed step's job summary prints only the final dict, not the delta lines.
Fix: fail onset(base) - set(measured); always write the measured dict to a--record FILE(or print JSON) and uploadmeasured-<key>on every run, so adding a scenario is "run CI, copy, commit"; include the per-scenario lines in the summary.
5. nit — dilution and the construction claim.
Callgrind, host: main inclusive 97.48 M, rdft_f inclusive 90.11 M → 7.6 % of the count is the out-of-place memcpy ×2, the 2/N scaling loop and the fold; more on scalar Cortex-M. Constant, so the ratchet works, but a 3 % FFT regression reads as ≈2.7 %. Construction is confirmed negligible (the Ooura table build runs on the first rdft_f call; everything in main outside rdft_f is 7.4 M, so <1 % holds with margin; 98 M in the README vs 99.4 M here is toolchain noise). Record the measured non-FFT share next to the 98/114/102 numbers.
6. nit — print_done uses if constexpr in a non-template function. Both branches are compiled (only a template discards the false one), so the float build carries memcpy(&u64, &float, 8) that would be UB if reached. Dead today, GCC/clang clean, but wrong idiom: two overloads print_done(bool, float) / print_done(bool, double).
7. nit — PRIx fix verified. %08lx + unsigned long is 32-bit on newlib ILP32 and 64-bit on LP64, both print ≥8 digits for a value < 2^32; %016llx + unsigned long long is portable; no <cinttypes>/PRI remains under bench/ or scripts/ (only insn_count.c's PRIu64, C on the host). Run 35262337354 confirms: icount m55 and icount m55-ooura compile both TUs and fail only at link (platform/armv8m_startup.c: No such file, which #17 supplies as platform/cortexm_startup.c through its own toolchain file); m4-softfp, m4f, m33 fail only at Could not find toolchain file. The plugin compiled against the pinned header on every leg.
8. nit — icount.py semantics verified. +3.00 % ok, +3.10 % REGRESSION, −3.10 % IMPROVED (both rc=1); delta is relative to the recorded value; --merge skips empty dicts and keeps untouched keys; --help and ap.error behave; baselines.json key order equals sort_keys=True output so --update produces no spurious churn. mps2-an386 is a valid QEMU 8.2 machine (in tree since 6.0; #17's M4 toolchain uses the same string). Plugin identical to MuTap's apart from the marker, QEMU_PLUGIN_VERSION = 2 from the pinned 8.2.2 header. Budget on m4-softfp: ≈25 k float ops per N=512 iteration × 2048 ≈ 5×10^7 soft-float ops ≈ 1.5–2×10^9 guest instructions, i.e. 10–20 s under TCG per binary against the 1200 s timeout; hard-float legs a few ×10^8, in MuTap's range. Not a problem.
9. nit — bench.yml. on: push: unrestricted + pull_request runs all five QEMU jobs twice per PR push (two Bench runs per SHA on this PR); ci.yml has the same shape, but this is the expensive workflow — push: branches: [main]. The seed-summary message "every key is seeded; nothing to merge" is printed when the seeding jobs actually failed (this PR); say "no seeding artifacts found". actions/checkout@v4 unpinned by SHA where MuTap pins — matches ci.yml, a repo-wide question.
10. nit — engine label vs Part 11. Part 11 lists cmsis/accelerate as engine values; the PR folds them into reference_c and lets the key say which backend (documented in bench_common.h). Defensible, but engine=reference_c printed on the m55 key is wrong on its face; add backend=cmsis|ooura|accelerate from the visible macros.
11. nit — bench_common.h / CMake predicates. fill comment says [-0.5, 0.5); static_cast<float>(0xFFFFFFFF) rounds to 2^32, so the top is exactly +0.5 (closed). No runtime double in a float scenario (verified; the constructor's one std::sqrt(double) and fftsg_float.c's table build are one-time). NOT CMAKE_CROSSCOMPILING (bench_fft) vs CMAKE_SYSTEM_NAME STREQUAL Generic (f64) disagree for a Linux-arm64 cross build; pick Generic for both. Generic dry-configure verified: only the two f32 targets are generated.
12. nit — style. Banners, k_/m_ naming, braces per STYLE.md; clang-format --dry-run clean on all four files; STYLE.md does not mandate [[nodiscard]]. k_engine_name.data() to %s relies on the view being built from a literal — true here; a constexpr const char* removes the trap. Root CMakeLists.txt change is the minimal option + guarded add_subdirectory.
Verdict
needs fixes: 1, 2, 3, 4 — then ready to un-draft after #17 merges.
Generated by Claude Code
…size probe Fix pass for the two hostile reviews on #21 (plan Part 13). - Checksum: an integer FNV-1a-64 fold over the memcpy'd bit pattern of every output word replaces the float running sum. Exact and order-sensitive: a 1-ulp nudge of one spectrum bin at one iteration changes rfft_f32_512 from 0x662dd085b5b88325 to 0x259510dc8721cba8 (reviewer A's perturbation, reproduced); a float sum absorbed it. Printed as 16 hex digits (%016llx) with a backend=cmsis|accelerate|ooura field from the visible macros; ok= is a round-trip sanity check. - Seeding is not a gate bypass: the Seed mode runs only on push to main or workflow_dispatch; a pull request with an empty key fails with the seeding instructions, and one that empties a key its base branch had seeded fails against the base ref's baselines.json. Every run records the measured counts (icount.py --record) and uploads measured-<key>; a baseline entry with no binary fails (STALE BASELINE); per-scenario lines go to the step summary. - seed-summary fails when any icount job failed and is documented as never a required check; concurrency group; push: branches [main]; permissions: contents: read. - Plugin API version: the pinned v8.2.2 header defines QEMU_PLUGIN_VERSION 1; bench.yml, README and insn_count.c now say v1. - Sizes: tap_dsp_size_probe_rfft_f32_512 (bench/size_probe.cpp, one profile, no stdio) built MinSizeRel like the legs, measured with arm-none-eabi-size -A (.text row); per-key text_ceiling in bench.yml, 0 = report only; ceilings are set by the wave-2 seeding commit. - Host smoke tests under ctest: each icount binary runs twice, ok=1 and identical DONE lines (bench/icount/smoke.cmake). - CMAKE_SYSTEM_NAME STREQUAL Generic decides host-class everywhere; fill() comment states the closed interval; k_engine_name is a const char*; SPDX/copyright on scripts/icount.py. - Provenance: qemu-plugin.h is GPL-2.0-or-later, fetched at CI time, used only to build a CI tool, never shipped (README and insn_count.c); NOTICE.md untouched (#18 owns it). - README: seeding records the main push run's URL and SHA; fixed table shape for updates; measured dilution (16.1/14.0/16.6 % of the count is copy + scale + fold) next to the 109/125/108 M host figures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
|
Fix pass in The five QEMU jobs stay red until #17 merges (missing toolchain files on M4/M33; the old M55 toolchain file's Review A (correctness)
Review B (process / downstream)
Draft stays a draft until #17 merges and the five 🤖 Generated with Claude Code https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy Generated by Claude Code |
…ffold Stage 1b of docs/audit-fft-and-code-smells.md: the measurement infrastructure lands before the C++20 port so Stage 2b is ratcheted against the vendored C rather than against nothing. - bench/icount: one deterministic binary per scenario (rfft_f32_512, rfft_f32_2048, rfft_f64_512 on host-class targets only), compile-time selected, xorshift input, a checksum over every output printed as its bit pattern; 2^20 samples per direction so construction is <0.2% of the count (measured with callgrind on the host: 98M/114M/102M). TAP_DSP_BENCH_ENGINE selects the engine: reference_c today, with the split_radix hook documented for Stage 2a. - bench/bench_fft.cpp: host wall-clock min-of-N microbenchmark, informational only. - scripts/icount.py and tools/qemu_insn_plugin/insn_count.c adapted from MuTap (MIT): targets m4-softfp, m4f (mps2-an386), m33 (mps2-an505), m55, m55-ooura (mps3-an547); +-3% two-sided gate; --merge folds per-key seeding files. - bench/baselines.json: empty per-key skeleton. Numbers are measured on CI and committed with SHA, GCC and QEMU versions in bench/README.md, never typed in. - .github/workflows/bench.yml: icount-ratchet matrix over the five keys, assuming the Stage 1 legs' toolchain files; an unseeded key runs --update and uploads its baselines as an artifact, a seeded key compares. arm-none-eabi-size reported per binary as the placeholder for the .text ceilings. - Root CMakeLists: option(TAP_DSP_BUILD_BENCH) + add_subdirectory(bench). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
newlib's <inttypes.h> still hides PRIx32/PRIx64 from C++ behind __STDC_FORMAT_MACROS, so the icount binaries failed to compile on the M55 leg while building cleanly on glibc. Print through unsigned long / unsigned long long with explicit casts instead. Host checksums are unchanged (0x446b9819, 0xc7aa23fa, 0x408d7334f8ae008e). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
…size probe Fix pass for the two hostile reviews on #21 (plan Part 13). - Checksum: an integer FNV-1a-64 fold over the memcpy'd bit pattern of every output word replaces the float running sum. Exact and order-sensitive: a 1-ulp nudge of one spectrum bin at one iteration changes rfft_f32_512 from 0x662dd085b5b88325 to 0x259510dc8721cba8 (reviewer A's perturbation, reproduced); a float sum absorbed it. Printed as 16 hex digits (%016llx) with a backend=cmsis|accelerate|ooura field from the visible macros; ok= is a round-trip sanity check. - Seeding is not a gate bypass: the Seed mode runs only on push to main or workflow_dispatch; a pull request with an empty key fails with the seeding instructions, and one that empties a key its base branch had seeded fails against the base ref's baselines.json. Every run records the measured counts (icount.py --record) and uploads measured-<key>; a baseline entry with no binary fails (STALE BASELINE); per-scenario lines go to the step summary. - seed-summary fails when any icount job failed and is documented as never a required check; concurrency group; push: branches [main]; permissions: contents: read. - Plugin API version: the pinned v8.2.2 header defines QEMU_PLUGIN_VERSION 1; bench.yml, README and insn_count.c now say v1. - Sizes: tap_dsp_size_probe_rfft_f32_512 (bench/size_probe.cpp, one profile, no stdio) built MinSizeRel like the legs, measured with arm-none-eabi-size -A (.text row); per-key text_ceiling in bench.yml, 0 = report only; ceilings are set by the wave-2 seeding commit. - Host smoke tests under ctest: each icount binary runs twice, ok=1 and identical DONE lines (bench/icount/smoke.cmake). - CMAKE_SYSTEM_NAME STREQUAL Generic decides host-class everywhere; fill() comment states the closed interval; k_engine_name is a const char*; SPDX/copyright on scripts/icount.py. - Provenance: qemu-plugin.h is GPL-2.0-or-later, fetched at CI time, used only to build a CI tool, never shipped (README and insn_count.c); NOTICE.md untouched (#18 owns it). - README: seeding records the main push run's URL and SHA; fixed table shape for updates; measured dilution (16.1/14.0/16.6 % of the count is copy + scale + fold) next to the 109/125/108 M host figures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
799ab97 to
e77ec32
Compare
) Copies the merged baselines artifact of the first bench.yml push-to-main run after #21 (run 35281280300 at df482d1) into bench/baselines.json: five keys (m4-softfp, m4f, m33, m55, m55-ooura), two scenarios each (rfft_f32_512, rfft_f32_2048), measured on the vendored Ooura C with arm-none-eabi-gcc 13.2.1 (15:13.2.rel1-2) and QEMU 8.2.2 on ubuntu-24.04. bench/README.md records one row per (key, scenario) with the run URL, the main SHA and the toolchain versions, as the seeding rule requires. From this commit every push and pull request compares against these numbers at +/-3%; the bench.yml run on this PR is the first compare-mode run and must report 0% delta (identical tree). Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy Co-authored-by: Claude <noreply@anthropic.com>
1 participant
What this changes
Stage 1b of
docs/audit-fft-and-code-smells.md(Part 3; design in Part 11): the measurement infrastructure for the real FFT, landed before the C++20 port so Stage 2b is ratcheted against the vendored C rather than against nothing.bench/icount/: one deterministic binary per scenario (rfft_f32_512,rfft_f32_2048, andrfft_f64_512on host-class targets only), compile-time selected, xorshift input, a checksum over every output printed as its bit pattern.TAP_DSP_BENCH_ENGINEselects the engine (reference_ctoday;split_radixis a documented hook that fails with a static_assert until Stage 2a).bench/bench_fft.cpp: host wall-clock min-of-N microbenchmark, informational only.scripts/icount.py+tools/qemu_insn_plugin/insn_count.c, adapted from MuTap (MIT, provenance comments kept): targetsm4-softfp,m4f(mps2-an386),m33(mps2-an505),m55,m55-ooura(mps3-an547); ±3 % two-sided gate;--mergefolds per-key seeding files.bench/baselines.json: empty per-key skeleton. No numbers are invented..github/workflows/bench.yml:icount-ratchetmatrix over the five keys; an unseeded key runs--updateand uploads its baselines as an artifact, a seeded key compares.arm-none-eabi-sizeper binary as the placeholder for the.textceilings.bench/README.md: the Part 11 policy; rootCMakeLists.txtgains onlyoption(TAP_DSP_BUILD_BENCH)+ a guardedadd_subdirectory(bench); one line in the README's Build section.Merges after the Stage 1 CI-legs PR (
claude/wave1-stage1-legs):bench.ymluses its toolchain files.Why
Wall clock on shared runners is noise; executed guest instructions under QEMU's TCG plugin are exact, which is why MuTap, SampleRateTap and RatioTap all gate on them. DspTap is the primitive under those ratchets, so a regression here surfaces downstream as a chain-level delta someone "updates" past. The primitive needs its own gate, per profile, per target, seeded from the C before anything changes (D11).
Verification
Hosted, done here:
cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DTAP_DSP_WERROR=ON -DTAP_DSP_BUILD_BENCH=ON && cmake --build build -j4 && ctest: builds warning-free under-Werror, 160/160 tests pass.tap_dsp_icount_*binary run twice: checksums bit-identical across runs (rfft_f32_5120x446b9819,rfft_f32_20480xc7aa23fa,rfft_f64_5120x408d7334f8ae008e).maininclusive 165 k). Well under the 1 % the design asks for.tap_dsp_bench_ffton this container (x86-64, GCC 13.3-O2, load average ~10 from parallel agents, so pessimistic): f32/512 ≈ 1.29 µs fwd / 1.37 µs inv; f32/2048 ≈ 6.1 / 6.4 µs; f64/512 ≈ 1.27 / 1.43 µs. Informational only.-DTAP_DSP_BENCH_ENGINE=split_radixfails with the documented static_assert. A dryCMAKE_SYSTEM_NAME=Genericconfigure excludesrfft_f64_512and the wall-clock bench as intended.bench.ymlYAML parses; the plugin header pin (QEMU v8.2.2 commit) was downloaded and its sha256 verified to match the pinned digest.git merge-treeof this commit withorigin/claude/wave1-stage1-legsis conflict-free, and its toolchain files matchbench.yml's assumptions exactly (cmake/arm-cortex-m4-mps2.cmakewithTAP_DSP_M4_FPU=OFF|ON,cmake/arm-cortex-m33-mps2.cmake,cmake/arm-cortex-m55-mps3.cmake,TAP_DSP_FFT_CMSISpinned OFF on M4/M33 —bench.ymlalso passes it explicitly on every non-Helium key, harmless redundancy; aptgcc-arm-none-eabi+qemu-system-armas in itsci.yml).Not done here, CI is the first real gate: the QEMU legs themselves. This container has neither the Arm toolchain nor QEMU, so nothing in
bench.ymlhas executed. The gap is the whole embedded path: cross-compilingicount_main.cpp(semihostingprintf,std::vectoron newlib), the plugin build against the pinned header, and the per-key counts.Notes for the reviewer
bench/baselines.jsonis empty by design, so on the first green run each job records its counts with--update, prints them in the step summary, and uploads them; theseed-summaryjob merges them into a singlebaselines-mergedartifact. The seeding commit (wave 2) copies that file intobench/baselines.jsonand fills the "Recorded baselines" table inbench/README.mdwith the run's commit, GCC and QEMU versions. Only from that commit on does a red ratchet fail a check. Until the legs PR merges, thebench.ymljobs on this branch will fail at configure (missing toolchain files) — expected; the PR is a draft for that reason.fft.h,tests/, andci.ymlare not modified.README.mdgains one line in the Build section.insn_count.candicount.pycome from MuTap (MIT, same family); provenance is in the file headers andbench/README.md. NoNOTICE.mdentry since it is Tap-family MIT code, not third-party — say if you want one anyway.🤖 Generated with Claude Code
https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
Generated by Claude Code