Conversation
silk_VAD_GetSA_Q8 had an x86 SSE4.1 implementation but no Arm one, and it runs on every SILK/hybrid frame in the default (float) build. Add a NEON version mirroring the SSE4.1 one: it vectorises the per-subframe energy sum-of-squares ((X[i] >> 3)^2 accumulated in int32), 8 samples per iteration via vshrq_n_s16 + paired vmlal_s16, with a scalar tail. Bit-exact with the C reference (exact integer sum, no overflow), validated by the existing OPUS_CHECK_ASM full-state memcmp. As on x86, silk_VAD_GetNoiseLevels is exported (rather than static inline in VAD.c) when NEON is enabled so the kernel can call it. Dispatched via the existing OVERRIDE_silk_VAD_GetSA_Q8 hook (PRESUME + an RTCD table in arm_silk_map.c); the source goes in the common SILK_SOURCES_ARM_NEON_INTR group, already wired in autotools/CMake/Meson. Microbench on Apple M4 (the real subband lengths, 10-80): ~1.1-1.7x over scalar; E2E within run-to-run noise (VAD is a small per-frame cost). Full meson test suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Tested head: With #483 applied on top of #503, the approximate combined production-c10 encoder improvement versus stock is +4.0159% thread CPU / +4.0195% wall. Positive means faster/lower per-call cost; negative means slower. These are compounded cross-run estimates from the direct #503-versus-stock result and the incremental #483-versus-#503 result, not a direct stock arm. #483 itself measured -0.0578% CPU / -0.0658% wall versus #503, so it did not improve encoding over #503. Route instrumentation confirmed that the modified VAD path executed in encode/re-encode, and the output/state checks were exact. Separately, the unguarded AArch64-only |
vaddvq_s32 is AArch64-only and breaks ARMv7 compilation of the silk_VAD_GetSA_Q8_neon kernel (dispatched for Neon ARMv7, arch index [3] in arm_silk_map.c). Replace with vpadd_s32 + lane extraction, which is portable (compiles on both ARMv7 and AArch64) and produces identical addp codegen on AArch64 (zero performance impact on the horizontal reduction). Fixes lpi review: xiph#483 (comment)
00a7bf1 to
1125bba
Compare
|
Acknowledged the ARMv7 build break you flagged (@lpi): the AArch64-only Re the −0.0578% incremental — that is within run‑to‑run noise here too (the combined #503+#483 result of +4.0% is solid). Will keep an eye on it but no further action needed on the code. Full analysis of both your review points below: 1. The review, summarized
2. Is the review positive? Yes. lpi confirmed the VAD changes work correctly and the combined improvement is solid +4%. The only actionable items were the ARMv7 guard (fixed) and the noise‑level perf note (not a blocker). 3. The fix |
|
@lpi do you think it is fine now ? also the ARM v7 is covered |
Summary
silk_VAD_GetSA_Q8had an x86 SSE4.1 implementation but no Arm one, even though it runs on every SILK/hybrid frame in the default (float) build. This adds a NEON version, mirroring the SSE4.1 one.What's vectorised
The per-subframe energy sum-of-squares —
(X[i] >> 3)^2accumulated in int32 — 8 samples per iteration viavshrq_n_s16+ pairedvmlal_s16(low/high), with a scalar tail and a horizontalvaddvq_s32. Everything else (analysis filterbank, noise estimation, SNR/tilt) is identical to the C reference, exactly as the SSE4.1 version does.silk_VAD_GetSA_Q8_c(exact integer sum of squares, no overflow), validated by the existingOPUS_CHECK_ASMfull-encoder-statememcmp.silk_VAD_GetNoiseLevelsbecomes exported (instead ofstatic inlineinVAD.c) when NEON is enabled, so the kernel can call it.Dispatch / wiring
Uses the existing
OVERRIDE_silk_VAD_GetSA_Q8hook: a newsilk/arm/VAD_arm.hprovides the PRESUME (direct call) and RTCD (SILK_VAD_GETSA_Q8_IMPLtable inarm_silk_map.c) dispatch, mirroringsilk/x86/main_sse.h. The source is added to the commonSILK_SOURCES_ARM_NEON_INTRgroup, which is already wired in autotools / CMake / Meson — so no build-system changes are needed.Numbers (Apple M4)
This is the last of the silk-side x86-has-it/ARM-doesn't parity gaps that runs in the default build.