From 01b7446a66d5e7a4d3361d70fb00eaa6e6b2c531 Mon Sep 17 00:00:00 2001 From: libretroadmin Date: Sun, 14 Jun 2026 14:36:58 +0000 Subject: [PATCH] =?UTF-8?q?filter:=20revert=20SSE2/NEON=20FIR=20kernel=20?= =?UTF-8?q?=E2=80=94=20non-bit-exact,=20audio=20regressions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SSE2/NEON FIR added in df8a6e8 was not bit-exact with the scalar code it replaced: * It packed int32 samples to int16 with PACKSSDW saturation, on the assumption that NES audio stays inside int16. At full mixer volume with several APU channels active, WaveHi samples can exceed int16 -- the pack then clips, changing the filter response in a way that is not just sub-LSB drift. * It applied the >> 6 once after the horizontal sum rather than per-term as the scalar does. The scalar's per-term shift keeps the int32 accumulator inside ~5e8 worst-case; the delayed shift pushes the intermediate value to ~3e10 and wraps int32 on sustained loud audio, producing garbage samples ("Sound Quality High" sounds broken). * The (int16_t) narrowing cast on the 96 kHz coefficient tables silently wrapped values up to ~51000 to negative numbers, giving nonsense coefficients on that output rate. There is no clean bit-exact int32 x int32 -> int32 SIMD on SSE2 baseline -- pmuludq is 2-wide unsigned 32x32 -> 64 with sign correction needed for the signed case, and the realisable speedup at scalar bit-exactness is marginal vs. the ~1.4% of frame budget the scalar already costs. Restoring the scalar code, which is correct across every rate / quality / volume combination. --- src/filter.c | 143 ++++++--------------------------------------------- 1 file changed, 16 insertions(+), 127 deletions(-) diff --git a/src/filter.c b/src/filter.c index 32d1aba..b2272c5 100644 --- a/src/filter.c +++ b/src/filter.c @@ -7,12 +7,6 @@ #include "fcoeffs.h" -#if defined(__SSE2__) -#include -#elif defined(__ARM_NEON) || defined(__ARM_NEON__) -#include -#endif - static uint32_t mrindex; static uint32_t mrratio; @@ -86,108 +80,6 @@ void SexyFilter(int32_t *in, int32_t *out, int32_t count) { code to be higher, or you *might* overflow the FIR code. */ -/* Int16 mirrors of the symmetric windowed-sinc coefficient tables built - * in MakeFilters. The source tables have max abs value ~21588, which - * comfortably fits in int16, so packing is lossless on the coefficient - * side. This lets the SIMD kernels below use pmaddwd / vmlal_s16, - * 4-8x faster than the scalar 32x32 path while preserving sub-LSB - * audio drift across the 484/1024-tap window. */ -static int16_t coeffs16[NCOEFFS]; -static int16_t sq2coeffs16[SQ2NCOEFFS]; - -/* FIR inner kernel. ncoeffs is NCOEFFS or SQ2NCOEFFS (compile-time - * constants both -- the compiler specialises this for each caller). - * The SSE2 and NEON paths apply the >> 6 once after the sum rather - * than per-term as the scalar does; the difference is per-term - * truncation rounding accumulated across the window, well under one - * LSB of the final 16-bit output sample. Sample magnitudes are - * packed with signed saturation (PACKSSDW / vqmovn_s32) -- typical - * NES audio stays comfortably within int16, and a peak that would - * saturate here would already be clipped at the subsequent SexyFilter - * stage that bounds output to [-32768, 32767]. */ -static INLINE void fir_inner_kernel( - const int32_t *S, const int16_t *D16, uint32_t ncoeffs, - int32_t *out_acc, int32_t *out_acc2) -{ - int32_t acc = 0, acc2 = 0; - uint32_t j; - -#if defined(__SSE2__) - { - __m128i acc_v = _mm_setzero_si128(); - __m128i acc2_v = _mm_setzero_si128(); - for (j = 0; j + 8 <= ncoeffs; j += 8) { - __m128i a_lo = _mm_loadu_si128((const __m128i *)&S[j + 1]); - __m128i a_hi = _mm_loadu_si128((const __m128i *)&S[j + 5]); - __m128i b_lo = _mm_loadu_si128((const __m128i *)&S[j + 2]); - __m128i b_hi = _mm_loadu_si128((const __m128i *)&S[j + 6]); - __m128i s_a = _mm_packs_epi32(a_lo, a_hi); - __m128i s_b = _mm_packs_epi32(b_lo, b_hi); - __m128i co = _mm_loadu_si128((const __m128i *)&D16[j]); - acc_v = _mm_add_epi32(acc_v, _mm_madd_epi16(s_a, co)); - acc2_v = _mm_add_epi32(acc2_v, _mm_madd_epi16(s_b, co)); - } - /* Horizontal sum of the 4 int32 lanes in each accumulator. */ - { - __m128i shuf = _mm_shuffle_epi32(acc_v, _MM_SHUFFLE(2,3,0,1)); - __m128i s1 = _mm_add_epi32(acc_v, shuf); - shuf = _mm_shuffle_epi32(s1, _MM_SHUFFLE(1,0,3,2)); - acc = _mm_cvtsi128_si32(_mm_add_epi32(s1, shuf)); - shuf = _mm_shuffle_epi32(acc2_v, _MM_SHUFFLE(2,3,0,1)); - s1 = _mm_add_epi32(acc2_v, shuf); - shuf = _mm_shuffle_epi32(s1, _MM_SHUFFLE(1,0,3,2)); - acc2 = _mm_cvtsi128_si32(_mm_add_epi32(s1, shuf)); - } - acc >>= 6; - acc2 >>= 6; - } -#elif defined(__ARM_NEON) || defined(__ARM_NEON__) - { - int32x4_t acc_v0 = vdupq_n_s32(0), acc_v1 = vdupq_n_s32(0); - int32x4_t acc2_v0 = vdupq_n_s32(0), acc2_v1 = vdupq_n_s32(0); - for (j = 0; j + 8 <= ncoeffs; j += 8) { - int32x4_t a0 = vld1q_s32(&S[j + 1]); - int32x4_t a1 = vld1q_s32(&S[j + 5]); - int32x4_t b0 = vld1q_s32(&S[j + 2]); - int32x4_t b1 = vld1q_s32(&S[j + 6]); - int16x8_t s_a = vcombine_s16(vqmovn_s32(a0), vqmovn_s32(a1)); - int16x8_t s_b = vcombine_s16(vqmovn_s32(b0), vqmovn_s32(b1)); - int16x8_t co = vld1q_s16(&D16[j]); - acc_v0 = vmlal_s16(acc_v0, vget_low_s16(s_a), vget_low_s16(co)); - acc_v1 = vmlal_s16(acc_v1, vget_high_s16(s_a), vget_high_s16(co)); - acc2_v0 = vmlal_s16(acc2_v0, vget_low_s16(s_b), vget_low_s16(co)); - acc2_v1 = vmlal_s16(acc2_v1, vget_high_s16(s_b), vget_high_s16(co)); - } - { - int32x4_t s_acc = vaddq_s32(acc_v0, acc_v1); - int32x4_t s_acc2 = vaddq_s32(acc2_v0, acc2_v1); -#if defined(__aarch64__) - acc = vaddvq_s32(s_acc); - acc2 = vaddvq_s32(s_acc2); -#else - int32x2_t p = vadd_s32(vget_low_s32(s_acc), vget_high_s32(s_acc)); - acc = vget_lane_s32(vpadd_s32(p, p), 0); - p = vadd_s32(vget_low_s32(s_acc2), vget_high_s32(s_acc2)); - acc2 = vget_lane_s32(vpadd_s32(p, p), 0); -#endif - } - acc >>= 6; - acc2 >>= 6; - } -#else - j = 0; -#endif - - /* Scalar tail handles whatever 0..7 taps the SIMD block didn't, - * and the entire window on builds without SSE2 or NEON. */ - for (; j < ncoeffs; j++) { - acc += (S[j + 1] * (int32_t)D16[j]) >> 6; - acc2 += (S[j + 2] * (int32_t)D16[j]) >> 6; - } - *out_acc = acc; - *out_acc2 = acc2; -} - int32_t NeoFilterSound(int32_t *in, int32_t *out, uint32_t inlen, int32_t *leftover) { uint32_t x; int32_t *outsave = out; @@ -196,9 +88,14 @@ int32_t NeoFilterSound(int32_t *in, int32_t *out, uint32_t inlen, int32_t *lefto if (FSettings.soundq == 2) { for (x = mrindex; x < max; x += mrratio) { - int32_t acc, acc2; - int32_t *S = &in[(x >> 16) - SQ2NCOEFFS]; - fir_inner_kernel(S, sq2coeffs16, SQ2NCOEFFS, &acc, &acc2); + int32_t acc = 0, acc2 = 0; + uint32_t c; + int32_t *S, *D; + + for (c = SQ2NCOEFFS, S = &in[(x >> 16) - SQ2NCOEFFS], D = sq2coeffs; c; c--, D++) { + acc += (S[c] * *D) >> 6; + acc2 += (S[1 + c] * *D) >> 6; + } acc = ((int64_t)acc * (65536 - (x & 65535)) + (int64_t)acc2 * (x & 65535)) >> (16 + 11); *out = acc; @@ -207,9 +104,14 @@ int32_t NeoFilterSound(int32_t *in, int32_t *out, uint32_t inlen, int32_t *lefto } } else { for (x = mrindex; x < max; x += mrratio) { - int32_t acc, acc2; - int32_t *S = &in[(x >> 16) - NCOEFFS]; - fir_inner_kernel(S, coeffs16, NCOEFFS, &acc, &acc2); + int32_t acc = 0, acc2 = 0; + uint32_t c; + int32_t *S, *D; + + for (c = NCOEFFS, S = &in[(x >> 16) - NCOEFFS], D = coeffs; c; c--, D++) { + acc += (S[c] * *D) >> 6; + acc2 += (S[1 + c] * *D) >> 6; + } acc = ((int64_t)acc * (65536 - (x & 65535)) + (int64_t)acc2 * (x & 65535)) >> (16 + 11); *out = acc; @@ -266,17 +168,4 @@ void MakeFilters(int32_t rate) { else for (x = 0; x < (NCOEFFS >> 1); x++) coeffs[x] = coeffs[NCOEFFS - 1 - x] = tmp[x]; - - /* Build the int16 mirror used by the SIMD inner kernel. All - * source FIR tables have max abs value ~21588 (fits in int16 - * losslessly), so a straight narrowing cast preserves every - * coefficient bit. Done once per filter rebuild -- 1024 entries - * is well under 1 us. */ - if (FSettings.soundq == 2) { - for (x = 0; x < SQ2NCOEFFS; x++) - sq2coeffs16[x] = (int16_t)sq2coeffs[x]; - } else { - for (x = 0; x < NCOEFFS; x++) - coeffs16[x] = (int16_t)coeffs[x]; - } }