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);