perf: use C++-order arithmetic except on AArch64 and x86 with fma - #37
Open
dignifiedquire wants to merge 3 commits into
Open
dignifiedquire wants to merge 3 commits into
dignifiedquire wants to merge 3 commits into
Conversation
ac75be6 rewrote 41 multiply-add expressions as f32::mul_add chains: the butterflies and real-FFT post-processing in fft4g (cft1st, cftmdl, rftfsub, rftbsub), the cascaded biquad filter, and the unrolled taps of the three-band filter bank. mul_add is one instruction only where the target has FMA. On x86_64 without the fma target feature, which is the default for every downstream build, each mul_add becomes an fmaf library call: on Linux an indirect call through the GOT to compiler_builtins' runtime-dispatched fmaf, on Windows MSVC the UCRT import. These calls sit in per-sample loops: the biquad feeds the high-pass filter and the AEC3 decimator, and the filter bank and fft4g run on every frame. With a static CRT on Windows (+crt-static), the result of libucrt's fmaf is not flushed under FTZ/DAZ. On the #34 flush-to-zero branch (fix/issue-34-denormals), its regression test finds 503 subnormal values in the AEC3 decimator output; with this change it passes. Each site now selects its form with a compile-time constant, USE_FMA = cfg!(any(target_arch = "aarch64", target_arch = "arm64ec", target_feature = "fma")). Those targets keep the mul_add chains unchanged; arm64ec runs AArch64 code and compiles mul_add to fmadd, as aarch64 does. Every other target uses the expressions from before ac75be6, which follow the operation order of the C++ reference (M145); GCC and clang do not fuse them on baseline x86_64. Other targets with native FMA (riscv64gc, powerpc64, armv7 with VFPv4, and others) also take the plain path. It follows the C++ source order, as C++ compiled without FP contraction does; GCC and clang would fuse these expressions there. Verification: - aarch64 output is bit-identical to main. Release assembly of the three crates is instruction-identical after normalizing symbol hashes, and a pipeline run (HPF, AEC3, NS and AGC2 at 16, 32 and 48 kHz, mono and stereo, 300 frames each) writes the same 10,368,000 output bytes. - arm64ec release code of the three crates is instruction-identical to main (built with nightly -Zbuild-std; not run). - x86_64 release code for Linux and Windows MSVC references fmaf on main and does not on this branch. With +fma the same sites compile to vfmadd instructions. - On x86_64, fft4g rdft and irdft output for n = 16 to 512 matches the upstream fft4g.cc built with -ffp-contract=off, bit for bit. New tests in each crate pin the policy. Their inputs make the fused and the C++-order results differ, and the implementation must match the fused result on AArch64 and x86 with fma, and the C++-order result on other targets with IEEE single-precision arithmetic. They do not apply to x87-only targets such as i586, where f32 expressions can be evaluated with excess precision. Flipping any single site fails them on aarch64 and on x86_64. The header comment of sonora-bench's cpp_comparison.rs no longer attributes the x86 differences to Rust's mul_add. The C++ reference is built with -march=native, so the C++ compiler may contract it on CPUs with FMA. Refs #34 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 93.64% 93.74% +0.09%
==========================================
Files 146 146
Lines 31130 31627 +497
Branches 31130 31627 +497
==========================================
+ Hits 29153 29650 +497
Misses 1753 1753
Partials 224 224
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Owner
Author
Real x86_64 timings
The temporary branch will be deleted. 🤖 Generated with Claude Code |
This was referenced Sep 29, 2026
…rget Review of 001d67b raised two problems. 1. The predicate that selects the fused or the plain form was written six times: a USE_FMA constant in each of fft4g.rs, cascaded_biquad_filter.rs and three_band_filter_bank.rs, and an EXPECT_FUSED copy in each of their test modules. 2. The tests did not test FMA against non-FMA. USE_FMA was a compile-time constant, so each build compiled and tested one form only, and EXPECT_FUSED restated the same predicate, so the check was circular. On aarch64 the plain C++-order form was never tested; on baseline x86_64 the fused form was never tested. sonora-simd now defines the predicate once, as the documented public constant NATIVE_FMA. The three USE_FMA constants and the three EXPECT_FUSED copies are removed. sonora-fft now depends on sonora-simd for it; sonora-simd depends only on cpufeatures, so this adds no cycle. The lockfiles of the excluded sonora-bench and fuzz workspaces record the new edge. The kernels are generic over `const FMA: bool` and select their form with `if FMA { fused } else { plain }`: cft1st, cftmdl, rftfsub and rftbsub in fft4g, reached through cftfsub, cftbsub and the private rdft_with and irdft_with; the three biquad loops, in process_with, process_in_place_with and apply_biquad; and filter_core, reached through analysis_with and synthesis_with. The public functions keep their signatures and call the NATIVE_FMA instantiation. The tests run both instantiations on every target. Each compares ::<true> with the mul_add chain and ::<false> with the C++-order expression, bit for bit, and asserts that the two references differ for its inputs: - butterflies_match_fused_and_plain_references (cft1st, cftmdl) - rft_sub_matches_fused_and_plain_references (rftfsub, rftbsub) - recursion_matches_fused_and_plain_references (the three biquad loops) - filter_core_matches_fused_and_plain_references (Parts 1 and 3) The butterfly test runs cft1st at n = 4096 and cftmdl at n = 8192 on pseudo-random input. It asserts that every twiddled output (real and imaginary, at j + l, j + 2l and j + 3l, in both groups) differs between the references in at least one butterfly, so each statement is checked on its own. With the golden-ratio input of the other tests, inputs l apart differ by almost the same amount, and some outputs did not differ at all. transforms_pass_the_form_to_every_kernel checks the fft4g call chain: rdft_with::<F> and irdft_with::<F> at n = 512 must equal the kernels composed with the form F written out (with a copy of the last radix-4 pass, which has no mul_add). It also asserts that each set of kernels one ::<FMA> in the chain controls (cft1st; both cftmdl passes; all three; rftfsub or rftbsub) changes both transforms when it alone takes the other form. A second test per file checks that the public functions equal the NATIVE_FMA instantiation, on inputs where the two forms differ: rdft_and_irdft_use_native_fma, public_methods_use_native_fma and analysis_and_synthesis_use_native_fma. Mutation testing on aarch64 and on x86_64 (under Rosetta), with each mutant built and tested on both: - Forcing any one of the 23 if/else blocks to one form fails a test, as does swapping its branches (138 of 138). - In the 18 fft4g blocks, putting one statement or tuple element in the other form fails a test (144 of 144). - Replacing one ::<FMA> in rdft_with, irdft_with, cftfsub or cftbsub with a literal fails a test at the eight sites that can change the output (32 of 32). The two n == 4 calls run no kernel, so their mutants are equivalent. - Wiring a public function to the wrong form fails a test: ::<false> on aarch64, ::<true> on x86_64. As before, the bit-exact tests assume IEEE single-precision evaluation, so x87-only targets such as i586 are not covered. Three attributes keep the production code as it was, or close to it. The generic bodies behind process, analysis, synthesis, rdft and irdft are #[inline(always)]; without it, process, analysis and synthesis compiled with small differences in block layout, scheduling and register allocation. rustc makes small functions that call nothing but intrinsics available for inlining in other crates. process_in_place was one; as a wrapper that calls process_in_place_with, it is not, so it is #[inline]. Its body is not forced inline, because that changed the code of the high-pass filter in sonora. filter_core is #[inline(always)]: its LLVM inline cost sits at the threshold, so whether analysis and synthesis inlined it depended on the codegen-unit partition. With Cargo's default 16 units, 001d67b inlined it on aarch64 (cost 520 against a threshold of 525) and not on x86_64; behind the generic chain it was no longer inlined on aarch64 either, which roughly doubled the code of analysis and synthesis there. Verification against 001d67b: - With the workspace release profile (thin LTO, one codegen unit), release assembly of sonora-fft, sonora-common-audio, sonora-aec3 and sonora-ns is instruction-identical on aarch64-apple-darwin, x86_64-unknown-linux-gnu with and without +fma, and x86_64-pc-windows-msvc. In sonora, every function is identical except ThreeBandFilterBank::analysis and synthesis, which differ in instruction scheduling and register allocation only, with the same floating-point instructions (aarch64-apple-darwin: 219 -> 229 and 222 -> 233 instructions; x86_64-unknown-linux-gnu: the same two functions differ). The comparison normalizes symbol hashes and label numbers, and maps the generic instance names (cft1st::<true> and others) to the old names. - With Cargo's default release profile (16 codegen units, no LTO), sonora-fft, sonora-common-audio, sonora-aec3 and sonora-ns are identical. In sonora, filter_core is now always inlined into analysis and synthesis. Instructions in analysis / synthesis: aarch64-apple-darwin 1240 / 1072 in 001d67b, 1256 / 1090 here (same floating-point instructions); x86_64-unknown-linux-gnu 2537 / 2459 plus a separate 212-instruction filter_core in 001d67b, 930 / 1060 here. - process_stream benchmarks (aarch64, workspace profile) show no change (p > 0.05 for 16k mono, 48k mono and 48k stereo). - A pipeline run (HPF, AEC3, NS and AGC2 at 16, 32 and 48 kHz, mono and stereo, 300 frames each) writes the same 10,368,000 output bytes on aarch64 and on x86_64 under Rosetta. Refs #34 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
The NATIVE_FMA documentation gave one reason for the plain path on every other target: that mul_add is an fmaf library call there. That reason holds only for targets without native FMA. riscv64gc, powerpc64 and armv7 with VFPv4 have native FMA and take the plain path by policy, so that they evaluate the expressions as C++ built without floating-point contraction does. The two cases now have separate sentences. The header of sonora-bench's cpp_comparison.rs said that on ARM (NEON) Rust and C++ produce bit-identical results. They do not. In fft4g.cc, makewt and makect pass a float argument to cos and sin, so C++ calls the float functions too. On aarch64-apple-darwin the -O3 C++ reference computes each cos/sin pair of one argument with one __sincosf_stret call (its fft4g object imports only ___sincosf_stret and _cosf). Rust calls cosf and sinf separately in debug builds, and merges only some pairs in release builds. In a standalone reproduction of the tables for n = 16 to 4096 (4,088 values), Rust differs from C++ -O3 in 92 entries (debug) and 42 (release), and Rust debug differs from C++ -O0, which calls cosf and sinf separately, in none. Each difference is 1 ulp. The header now names AArch64, where NATIVE_FMA selects mul_add, and gives this cause; 32-bit ARM with NEON takes the plain path in Rust. Refs #34 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ac75be6rewrote 41 multiply-add expressions in fft4g, the cascaded biquad filter, and the three-band filter bank asf32::mul_addchains. That gives singlefmaddinstructions on AArch64. On x86_64 without thefmatarget feature, which is the default for every downstream build, eachmul_addinstead becomes anfmaflibrary call:compiler_builtins' runtime-dispatchedfmaf.These calls sit in per-sample loops (the high-pass filter and the AEC3 decimator via the biquad) and in per-frame loops (the filter bank and fft4g).
This PR keeps the
mul_addchains only where the target has native FMA. Everywhere else it restores the expressions from beforeac75be6, which follow the C++ reference's operation order:Effect by target
mainmul_add→fmadd)fma(for example-C target-cpu=native)mul_add→vfmadd)fmafcallsWhy it matters
fmafcall from every multiply-add in these loops.+crt-static), the result of libucrt'sfmafis not flushed under FTZ/DAZ. On the AEC3 slows 10–50× while the render reference is silent (denormals): upstream wraps APM calls in DenormalDisabler, the port does not #34 branch, its regression test found 503 subnormal values in the AEC3 decimator output. With this change merged in, it passes. Refs AEC3 slows 10–50× while the render reference is silent (denormals): upstream wraps APM calls in DenormalDisabler, the port does not #34.Verification
Codegen
fmaffrom all three crates.fmafreferences on either target.+fma: the same sites compile to nativevfmadd.aarch64 bit-identity
mainat001d67b. At the head it still is, exceptThreeBandFilterBank::analysisandsynthesis, which now always inlinefilter_core; their floating-point instructions are unchanged.5de7d2f7….mainat001d67b(compiled only, not run).Tests. The kernels are generic over
const FMA: bool: the biquad loops,filter_core, and fft4g'scft1st,cftmdl,rftfsub,rftbsuband therdft/irdftchain. Production code calls::<NATIVE_FMA>. The tests run both::<true>and::<false>on every target and compare each form bit for bit against an independent reference: themul_addchain, or the C++ expression. Each test also asserts that the two references differ, so it is sensitive. Further tests check that each public function equals its::<NATIVE_FMA>instantiation, and that the fft4g transforms pass the form to every kernel. So macOS CI now tests the C++-order path, and x86 CI tests the fused path. The second commit made this change, after review found the first version's tests circular. Mutation testing, on aarch64 and on x86_64:::<FMA>call site wired to a literal (where it can change output)cargo test --workspace(aarch64)cargo test --workspace(x86_64 Linux, Docker)-D warnings(host, x86_64, x86_64 with+fma), MSRV 1.91.1, docssilent_render_leaves_no_subnormal_statenow passes under+crt-static(previously 503 subnormals)C++ parity on x86_64 (sonora-bench, GCC 13, Docker). The per-test max diffs,
main→ this PR:-march=native-march=native(CI)The C++ reference in CI is built with
-march=native, so GCC fuses the C++ reference itself. Against that build, three maximum diffs increase, all within their tolerances. For hpf_48k, 2.46e-5 is about half ofCOMPONENT_TOL= 5e-5. These increases are the difference between fused and unfused C++, not a Rust regression: against the unfused C++ build, the same tests are bit-exact.Commits
001d67bperf: use C++-order arithmetic except on AArch64 and x86 with fma7a0ff9erefactor: share NATIVE_FMA and test both arithmetic forms on every target. This adds one shared constant (sonora_simd::NATIVE_FMA;sonora-fftgains a dependency onsonora-simd) and the const-generic kernels and tests described above. Pipeline output is byte-identical to001d67bon every target, and tomainon aarch64/arm64ec and x86 withfma. Code generation is identical to001d67bexceptThreeBandFilterBank::analysisandsynthesis, wherefilter_coreis now always inlined.001d67binlined it by a narrow margin on aarch64 and not at all on x86_64 under Cargo's default 16 codegen units. The floating-point instructions are the same, and theprocess_streambenchmarks show no change (measured on aarch64).1ab2c63docs: correct the NATIVE_FMA rationale and the AArch64 parity note. On Apple arm64, Rust's fft4g twiddle tables differ from optimized C++ by 1 ulp in some entries, because Rust and C++ pairsinf/cosfcalls intosincosfdifferently. So aarch64 is close to C++, not bit-identical.Notes and follow-ups
mainhas no "Unreleased" section and AEC3 slows 10–50× while the render reference is silent (denormals): upstream wraps APM calls in DenormalDisabler, the port does not #34's branch adds one. After AEC3 slows 10–50× while the render reference is silent (denormals): upstream wraps APM calls in DenormalDisabler, the port does not #34 lands, add: "Output changes by tiny amounts on every target except AArch64 and x86 withfma. fft4g, the cascaded biquad (HPF, AEC3 decimator), and the three-band filter bank use unfused arithmetic in C++ order, which is closer to C++ built without FP contraction. aarch64 and arm64ec output is unchanged."-ffp-contract=off, or without-march=native, for a stable reference that isn't fused.cpp_comparison.rs: its header comment is updated for the new x86 behavior.001d67bagainstmainon the same runner. The later commits change code generation only in the filter bank's inlining, so the numbers very likely still apply; they were not re-measured.🤖 Generated with Claude Code
https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx