From c58baedd7562585697c436158ffebcb4d39774f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 21 Sep 2026 12:13:09 -0600 Subject: [PATCH] Vertex decoder: fix the jit-match test on MSVC The test didn't compile: VERTS is captured by reference into testFormat, and MSVC won't use a captured constexpr as an array bound. Make it static. On arm64 it then failed on the UV prescale steps, by one ULP. The arm64 JIT and the NEON handwritten decoders fuse the multiply-add, and the steps only match that when the compiler contracts a * b + c - which clang does and MSVC doesn't, in Debug or Release. Spell out which one happens instead of relying on it. Co-Authored-By: Claude Opus 5 (1M context) --- GPU/Common/VertexDecoderCommon.cpp | 30 ++++++++++++++++++++++-------- docs/building.md | 9 +++++++++ unittest/TestVertexJit.cpp | 5 +++-- 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/GPU/Common/VertexDecoderCommon.cpp b/GPU/Common/VertexDecoderCommon.cpp index bbb9f04647..29f26ee86b 100644 --- a/GPU/Common/VertexDecoderCommon.cpp +++ b/GPU/Common/VertexDecoderCommon.cpp @@ -352,32 +352,46 @@ void VertexDecoder::Step_TcFloatThrough(const VertexDecoder *dec, const u8 *ptr, gstate_c.vertBounds.maxV = std::max(gstate_c.vertBounds.maxV, (u16)uvdata[1]); } +// The arm64 JIT and the NEON handwritten decoders fuse the UV prescale (FMLA), the x86 ones don't +// (MULPS + ADDPS). Spell out which one happens here instead of leaving it to the compiler's +// contraction setting: clang contracts this by default and MSVC doesn't, so relying on it makes the +// steps disagree with the JIT on Windows on ARM only. +static inline float PrescaleUV(float value, float scale, float offset) { +#if PPSSPP_ARCH(ARM64_NEON) + return fmaf(value, scale, offset); +#else + // Safe as long as x86 stays on the SSE2 baseline, which has nothing to contract into. A build + // targeting FMA would need this spelled out too, the other way around from the arm64 one. + return value * scale + offset; +#endif +} + void VertexDecoder::Step_TcU8Prescale(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { float *uv = (float *)(decoded + dec->decFmt.uvoff); const u8 *uvdata = (const u8 *)(ptr + dec->tcoff); - uv[0] = (float)uvdata[0] * (1.f / 128.f) * dec->prescaleUV_->uScale + dec->prescaleUV_->uOff; - uv[1] = (float)uvdata[1] * (1.f / 128.f) * dec->prescaleUV_->vScale + dec->prescaleUV_->vOff; + uv[0] = PrescaleUV((float)uvdata[0] * (1.f / 128.f), dec->prescaleUV_->uScale, dec->prescaleUV_->uOff); + uv[1] = PrescaleUV((float)uvdata[1] * (1.f / 128.f), dec->prescaleUV_->vScale, dec->prescaleUV_->vOff); } void VertexDecoder::Step_TcU16Prescale(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { float *uv = (float *)(decoded + dec->decFmt.uvoff); const u16_le *uvdata = (const u16_le *)(ptr + dec->tcoff); - uv[0] = (float)uvdata[0] * (1.f / 32768.f) * dec->prescaleUV_->uScale + dec->prescaleUV_->uOff; - uv[1] = (float)uvdata[1] * (1.f / 32768.f) * dec->prescaleUV_->vScale + dec->prescaleUV_->vOff; + uv[0] = PrescaleUV((float)uvdata[0] * (1.f / 32768.f), dec->prescaleUV_->uScale, dec->prescaleUV_->uOff); + uv[1] = PrescaleUV((float)uvdata[1] * (1.f / 32768.f), dec->prescaleUV_->vScale, dec->prescaleUV_->vOff); } void VertexDecoder::Step_TcU16DoublePrescale(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { float *uv = (float *)(decoded + dec->decFmt.uvoff); const u16_le *uvdata = (const u16_le *)(ptr + dec->tcoff); - uv[0] = (float)uvdata[0] * (1.f / 16384.f) * dec->prescaleUV_->uScale + dec->prescaleUV_->uOff; - uv[1] = (float)uvdata[1] * (1.f / 16384.f) * dec->prescaleUV_->vScale + dec->prescaleUV_->vOff; + uv[0] = PrescaleUV((float)uvdata[0] * (1.f / 16384.f), dec->prescaleUV_->uScale, dec->prescaleUV_->uOff); + uv[1] = PrescaleUV((float)uvdata[1] * (1.f / 16384.f), dec->prescaleUV_->vScale, dec->prescaleUV_->vOff); } void VertexDecoder::Step_TcFloatPrescale(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { float *uv = (float *)(decoded + dec->decFmt.uvoff); const float_le *uvdata = (const float_le *)(ptr + dec->tcoff); - uv[0] = uvdata[0] * dec->prescaleUV_->uScale + dec->prescaleUV_->uOff; - uv[1] = uvdata[1] * dec->prescaleUV_->vScale + dec->prescaleUV_->vOff; + uv[0] = PrescaleUV(uvdata[0], dec->prescaleUV_->uScale, dec->prescaleUV_->uOff); + uv[1] = PrescaleUV(uvdata[1], dec->prescaleUV_->vScale, dec->prescaleUV_->vOff); } void VertexDecoder::Step_TcU8MorphToFloat(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { diff --git a/docs/building.md b/docs/building.md index f763be1f5a..4e76ba3508 100644 --- a/docs/building.md +++ b/docs/building.md @@ -146,6 +146,15 @@ we have: MSVC links an `inline` function defined in a .cpp anyway, clang correct build and pass on Windows and fail to link only on Android CI, with an undefined symbol pointing at a header line. Fix it by dropping the bogus `inline` from the definition, not by avoiding the call. +The compilers also disagree about floating point contraction, which matters for any test asserting that a JIT +is bit-identical to its C++ reference. Clang folds `a * b + c` into a single fused multiply-add by default; +MSVC never does, under `/fp:precise`, in Debug or Release. So on arm64, where the JITs emit `FMLA`, a +reference written as `a * b + c` matches on Mac, Linux and Android and is off by one ULP on Windows on ARM. +Don't leave it to the compiler: write `fmaf(a, b, c)` when the fused result is wanted (MSVC compiles it to a +single `fmadd`), and `a * b + c` when it isn't. `PrescaleUV` in `GPU/Common/VertexDecoderCommon.cpp` picks per +architecture, matching what each JIT does. x86 doesn't have the problem, since the SSE2 baseline has no FMA +instruction to contract into. + pspautotests are a large set of tests of the PSP OS's API surface, and thus tests our HLE implementation. **To check for regressions, run them exactly the way CI does** (see `.github/workflows/build.yml`): diff --git a/unittest/TestVertexJit.cpp b/unittest/TestVertexJit.cpp index c99c4bc9e9..31b194a8df 100644 --- a/unittest/TestVertexJit.cpp +++ b/unittest/TestVertexJit.cpp @@ -794,8 +794,9 @@ struct JitMismatch { } // namespace [[maybe_unused]] static bool TestVertexJitMatchesSteps() { - constexpr int VERTS = 32; - constexpr int BUF_SIZE = 64 * 1024; + // static, or MSVC treats these as captured references and won't use VERTS as an array bound. + static constexpr int VERTS = 32; + static constexpr int BUF_SIZE = 64 * 1024; // Decode may overrun by a vertex plus 16 bytes, see DecodeVerts. u8 *src = (u8 *)AllocateAlignedMemory(BUF_SIZE, 16); u8 *refOut = (u8 *)AllocateAlignedMemory(BUF_SIZE, 16);