Fix DSP 40-bit MAC accumulator (pink-noise / long IIR)#118
Fix DSP 40-bit MAC accumulator (pink-noise / long IIR)#118JoeMatt merged 1 commit intolibretro:masterfrom
Conversation
Track IMACN accumulation in hardware-correct signed 40-bit wrap and keep bits 63..40 clear so RESMAC and control-register reads stay consistent. Adds dsp_acc40.h helpers and test/test_dsp_mac40.c (CI). Mitigates long-IIR DSP paths (e.g. pink-noise generation) diverging vs real HW. Made-with: Cursor
|
verified with updated 240P test rom pink noise test now renders. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Jaguar DSP emulation accuracy by enforcing the correct 40-bit signed semantics for the DSP MAC accumulator, preventing long IMACN chains (e.g., IIR/pink-noise workloads) from diverging due to unbounded 64-bit growth.
Changes:
- Add a small header (
src/dsp_acc40.h) providing 40-bit wrap + sign-extension helpers. - Update DSP IMACN/IMULTN paths to use the helpers so
dsp_accstays in a 40-bit domain. - Add a C regression/unit test and run it in CI alongside the existing blitter SIMD tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/dsp.c |
Uses new helper functions for IMACN/IMULTN accumulator updates. |
src/dsp_acc40.h |
Introduces 40-bit accumulator wrap/sign-extend utilities. |
test/test_dsp_mac40.c |
Adds a unit/regression test for 40-bit accumulator behavior. |
.github/workflows/c-cpp.yml |
Runs the new DSP accumulator test in CI. |
CLAUDE.md |
Documents the new test harness entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| INLINE static void dsp_opcode_imacn(void) | ||
| { | ||
| int32_t res = (int16_t)RM * (int16_t)RN; | ||
| dsp_acc += (int64_t)res; | ||
| //Should we AND the result to fit into 40 bits here??? | ||
|
|
||
| dsp_acc_mac_apply(&dsp_acc, res); |
There was a problem hiding this comment.
dsp_acc is now kept with bits 63..40 cleared, but dsp_opcode_sat32s / DSP_sat32s elsewhere in this file do int32_t temp = dsp_acc >> 32; and rely on the upper bits being sign-extended (so temp becomes -1/0 for in-range values). With the new 40-bit storage, negative accumulator values will produce temp in the 0..255 range and saturate incorrectly. Update those SAT32S implementations to explicitly sign-extend from the 40-bit high byte (e.g., cast via int8_t) or derive temp from dsp_acc_i40_signed().
| #include "dsp.h" | ||
| #include "dsp_acc40.h" |
There was a problem hiding this comment.
Now that the code intends to keep only the low 40 bits in dsp_acc (bits 63..40 must be zero), consider normalizing dsp_acc after savestate loads as well. STATE_LOAD_VAR(buf, dsp_acc) can restore older states where the accumulator was stored sign-extended in the upper bits; masking to 40 bits (or using the new helper) would keep the new invariant consistent immediately after load.
| @@ -0,0 +1,56 @@ | |||
| /* | |||
| * Unit tests for src/dsp_acc40.h (Jaguar DSP 40-bit MAC semantics). | |||
| * Build: cc -O2 -Wall -I../src -o test_dsp_mac40 test/test_dsp_mac40.c | |||
There was a problem hiding this comment.
The build hint in the file header uses -I../src, but this test is invoked from the repo root in CI (and matches other tests) with -I src. Please update the comment to reflect the correct working directory/include path (or mention both) to avoid confusing local builds.
| * Build: cc -O2 -Wall -I../src -o test_dsp_mac40 test/test_dsp_mac40.c | |
| * Build from repo root (CI): cc -O2 -Wall -I src -o test_dsp_mac40 test/test_dsp_mac40.c | |
| * Build from test/: cc -O2 -Wall -I../src -o test_dsp_mac40 test_dsp_mac40.c |
Summary
The Jaguar DSP MAC is a 40-bit signed accumulator. The core kept
dsp_accin a 64-bit variable and only added products withdsp_acc += (int64_t)prodwithout folding to 40 bits. After manyIMACNsteps (typical of IIR / pink-noise style synthesis on the DSP), the emulated value diverged from hardware, with RESMAC and the high-byte read path (dsp_acc >> 32inDSPReadLong) no longer matching a real 40-bit register (and bits 63..40 should stay zero for those reads).This change introduces
src/dsp_acc40.hwith wrap/sign-extend helpers, uses them fromdsp_opcode_imacn/DSP_imacnanddsp_opcode_imultn/DSP_imultn, and addstest/test_dsp_mac40.cplus a CI step (next to the blitter SIMD tests).Context (240p / pink noise)
Long IIR MAC chains are sensitive to accumulator width. A prior hypothesis involved host
__muldi3on 32-bit libretro builds; this fix addresses emulation accuracy of the DSP MAC itself. If any platform still shows issues after this, the next place to check is linkinglibgcc/ compiler-rt for 64-bit soft-mul on that target.Test plan
cc -I./src -o test/test_dsp_mac40 test/test_dsp_mac40.c && ./test/test_dsp_mac40makeon macOSMade with Cursor