From a1ed24ae2d78be7c67d14dcc7e931c493773869e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 21 Sep 2026 10:12:19 -0600 Subject: [PATCH] Vertex decoder: fix the handwritten decoders, and test them too The two handwritten SIMD decoders are used with or without the JIT, and had drifted from the step functions: - The God of War one ignored g_DoubleTextureCoordinates, giving HD Remaster games the wrong UVs, and passed NaN and infinite positions through. Those now come out finite like everywhere else. - The GTA one expanded 5551 colors wrongly on NEON (a left shift where the SSE version shifts right). - On ARM64, both now fuse the UV scale and offset, like the JIT and the steps as the compiler builds them. Both are back in the unit test, which now also feeds NaN and infinity to plain float positions and checks they come out finite, and runs only on x86-64 and arm64 for now. Co-Authored-By: Claude Opus 5 (1M context) --- GPU/Common/VertexDecoderCommon.cpp | 3 ++ GPU/Common/VertexDecoderHandwritten.cpp | 35 ++++++++++++-- unittest/TestVertexJit.cpp | 64 ++++++++++++++++++------- 3 files changed, 80 insertions(+), 22 deletions(-) diff --git a/GPU/Common/VertexDecoderCommon.cpp b/GPU/Common/VertexDecoderCommon.cpp index bbbb9d4c76..bbb9f04647 100644 --- a/GPU/Common/VertexDecoderCommon.cpp +++ b/GPU/Common/VertexDecoderCommon.cpp @@ -833,6 +833,9 @@ void VertexDecoder::Step_PosS16(const VertexDecoder *dec, const u8 *ptr, u8 *dec void VertexDecoder::Step_PosFloat(const VertexDecoder *dec, const u8 *ptr, u8 *decoded) { Vec4F32 v = Vec4F32::Load((const float *)(ptr + dec->posoff)); + // NaN and infinity only have to come out finite, so that the viewport scale can zero them later + // like the PSP does (0 * NaN == 0 there). The platforms differ in how (SSE clamps, NEON zeroes), + // and so do the JITs, which is fine. v.CleanNaNInfs().Store((float *)(decoded + dec->decFmt.posoff)); } diff --git a/GPU/Common/VertexDecoderHandwritten.cpp b/GPU/Common/VertexDecoderHandwritten.cpp index 576aeb6a67..896423b286 100644 --- a/GPU/Common/VertexDecoderHandwritten.cpp +++ b/GPU/Common/VertexDecoderHandwritten.cpp @@ -1,5 +1,6 @@ #include "Common/CommonTypes.h" #include "Common/Data/Convert/ColorConv.h" +#include "Core/HDRemaster.h" #include "GPU/Common/VertexDecoderCommon.h" #include "GPU/GPUState.h" @@ -61,41 +62,61 @@ void VtxDec_Tu16_C8888_Pfloat(const u8 *srcp, u8 *dstp, int count, const UVScale }; const GOWVTX *src = (const GOWVTX *)srcp; OutVTX *dst = (OutVTX *)dstp; - float uscale = uvScaleOffset->uScale * (1.0f / 32768.0f); - float vscale = uvScaleOffset->vScale * (1.0f / 32768); + // The HD Remasters double the texture coordinates, see Step_TcU16DoublePrescale. + const float uvDiv = g_DoubleTextureCoordinates ? (1.0f / 16384.0f) : (1.0f / 32768.0f); + float uscale = uvScaleOffset->uScale * uvDiv; + float vscale = uvScaleOffset->vScale * uvDiv; float uoff = uvScaleOffset->uOff; float voff = uvScaleOffset->vOff; u32 alpha = 0xFFFFFFFF; + // The position is loaded together with the color, as lanes 1-3. A NaN or infinite coordinate has + // to come out finite, so that the viewport scale can zero it later like the PSP does. Which finite + // value doesn't matter, so any lane with an all-ones exponent is zeroed. Lane 0 (the color) is + // left alone: its mask is 0, which never equals the exponent. + alignas(16) static const u32 expMask[4] = { 0, 0x7F800000, 0x7F800000, 0x7F800000 }; + #if PPSSPP_ARCH(SSE2) __m128 uvOff = _mm_setr_ps(uoff, voff, uoff, voff); __m128 uvScale = _mm_setr_ps(uscale, vscale, uscale, vscale); __m128i alphaMask = _mm_set1_epi32(0xFFFFFFFF); + const __m128i posExpMask = _mm_load_si128((const __m128i *)expMask); + const __m128i expAllOnes = _mm_set1_epi32(0x7F800000); for (int i = 0; i < count; i++) { __m128i uv = _mm_set1_epi32(src[i].packed_uv); __m128 fuv = _mm_cvtepi32_ps(_mm_unpacklo_epi16(uv, _mm_setzero_si128())); __m128 finalUV = _mm_add_ps(_mm_mul_ps(fuv, uvScale), uvOff); u32 normal = src[i].packed_normal; __m128i colpos = _mm_loadu_si128((const __m128i *)&src[i].col); + alphaMask = _mm_and_si128(alphaMask, colpos); + colpos = _mm_andnot_si128(_mm_cmpeq_epi32(_mm_and_si128(colpos, posExpMask), expAllOnes), colpos); _mm_store_sd((double *)&dst[i].u, _mm_castps_pd(finalUV)); dst[i].packed_normal = normal; _mm_storeu_si128((__m128i *)&dst[i].col, colpos); - alphaMask = _mm_and_si128(alphaMask, colpos); } alpha = _mm_cvtsi128_si32(alphaMask); #elif PPSSPP_ARCH(ARM_NEON) - float32x2_t uvScale = vmul_f32(vld1_f32(&uvScaleOffset->uScale), vdup_n_f32(1.0f / 32768.0f)); + const float scaleArr[2] = { uscale, vscale }; + float32x2_t uvScale = vld1_f32(scaleArr); float32x2_t uvOff = vld1_f32(&uvScaleOffset->uOff); uint32x4_t alphaMask = vdupq_n_u32(0xFFFFFFFF); + const uint32x4_t posExpMask = vld1q_u32(expMask); + const uint32x4_t expAllOnes = vdupq_n_u32(0x7F800000); for (int i = 0; i < count; i++) { uint16x4_t uv = vld1_u16(&src[i].u); // TODO: We only need the first two lanes, maybe there's a better way? uint32x2_t fuv = vget_low_u32(vmovl_u16(uv)); // Only using the first two lanes +#if PPSSPP_ARCH(ARM64_NEON) + // Fused, like the arm64 JIT and the steps as the compiler builds them. + float32x2_t finalUV = vfma_f32(uvOff, vcvt_f32_u32(fuv), uvScale); +#else float32x2_t finalUV = vadd_f32(vmul_f32(vcvt_f32_u32(fuv), uvScale), uvOff); +#endif u32 normal = src[i].packed_normal; uint32x4_t colpos = vld1q_u32((const u32 *)&src[i].col); alphaMask = vandq_u32(alphaMask, colpos); + colpos = vbicq_u32(colpos, vceqq_u32(vandq_u32(colpos, posExpMask), expAllOnes)); vst1_f32(&dst[i].u, finalUV); dst[i].packed_normal = normal; vst1q_u32(&dst[i].col, colpos); @@ -246,7 +267,11 @@ void VtxDec_Tu8_C5551_Ps16(const u8 *srcp, u8 *dstp, int numVerts, const UVScale uint8x8_t uv8 = vreinterpret_u8_u64(uv8_one); uint16x4_t uv16 = vget_low_u16(vmovl_u8(uv8)); uint32x4_t uv32 = vmovl_u16(uv16); +#if PPSSPP_ARCH(ARM64_NEON) + float32x4_t uvf = vfmaq_f32(uvOffset, vcvtq_f32_u32(uv32), uvScale); +#else float32x4_t uvf = vaddq_f32(vmulq_f32(vcvtq_f32_u32(uv32), uvScale), uvOffset); +#endif alpha &= col0; @@ -258,7 +283,7 @@ void VtxDec_Tu8_C5551_Ps16(const u8 *srcp, u8 *dstp, int numVerts, const UVScale int32x2_t a_shifted = vshr_n_s32(vreinterpret_s32_u32(vshl_n_u32(vand_u32(col, amask), 16)), 7); uint32x2_t a = vreinterpret_u32_s32(a_shifted); col = vorr_u32(vorr_u32(r, g), b); - col = vorr_u32(col, vand_u32(vshl_n_u32(col, 5), lowbits)); + col = vorr_u32(col, vand_u32(vshr_n_u32(col, 5), lowbits)); col = vorr_u32(col, a); // TODO: Mix into fewer stores. diff --git a/unittest/TestVertexJit.cpp b/unittest/TestVertexJit.cpp index a7170fc280..c99c4bc9e9 100644 --- a/unittest/TestVertexJit.cpp +++ b/unittest/TestVertexJit.cpp @@ -16,6 +16,7 @@ // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. #include +#include #include #include #include @@ -682,7 +683,8 @@ int DecodedComponentSize(u8 fmt) { } } -void FillVertexData(JitMatchRng &rng, const VertexDecoder &dec, u8 *src, int count) { +// With specialPos, some plain float positions get a NaN or infinity, and specialPos[v] says which. +void FillVertexData(JitMatchRng &rng, const VertexDecoder &dec, u8 *src, int count, bool *specialPos) { static const int wtSize[] = { 0, 1, 2, 4 }; static const int tcSize[] = { 0, 1, 2, 4 }; static const int nrmPosSize[] = { 0, 1, 2, 4 }; @@ -728,6 +730,14 @@ void FillVertexData(JitMatchRng &rng, const VertexDecoder &dec, u8 *src, int cou fillScalars(p + dec.posoff, 3, nrmPosSize[dec.pos]); } } + if (specialPos) { + specialPos[v] = (rng.Next() & 7) == 0; + if (specialPos[v]) { + static const float specials[] = { NAN, INFINITY, -INFINITY }; + float_le f = specials[rng.Next() % 3]; + memcpy(src + v * dec.size + dec.posoff + (rng.Next() % 3) * 4, &f, 4); + } + } } } @@ -783,7 +793,7 @@ struct JitMismatch { } // namespace -static bool TestVertexJitMatchesSteps() { +[[maybe_unused]] static bool TestVertexJitMatchesSteps() { constexpr int VERTS = 32; constexpr int BUF_SIZE = 64 * 1024; // Decode may overrun by a vertex plus 16 bytes, see DecodeVerts. @@ -819,12 +829,15 @@ static bool TestVertexJitMatchesSteps() { if (!jit.jitted_) { return; } - // TODO: The handwritten decoders don't match the steps yet. - if (!cache->IsInSpace((const u8 *)jit.jitted_)) { - return; - } formatsJitted++; + // Which step writes each component: weights (if any) first, then tc, col, nrm, pos. + int stepIndex = ref.weighttype ? 1 : 0; + StepFunction tcStep = ref.tc ? ref.steps_[stepIndex++] : nullptr; + StepFunction colStep = ref.col ? ref.steps_[stepIndex++] : nullptr; + StepFunction nrmStep = ref.nrm ? ref.steps_[stepIndex++] : nullptr; + StepFunction posStep = ref.steps_[stepIndex]; + for (int i = 0; i < 8; i++) { gstate_c.morphWeights[i] = rng.Float24(-0.5f, 1.5f); } @@ -835,30 +848,27 @@ static bool TestVertexJitMatchesSteps() { const int srcBytes = ref.VertexSize() * VERTS; _assert_(srcBytes + 256 <= BUF_SIZE && ref.decFmt.stride * (VERTS + 1) + 16 <= BUF_SIZE); - FillVertexData(rng, ref, src, VERTS); + // Only plain float positions are cleaned of NaN and infinity (see Step_PosFloat). + bool specialPos[VERTS]{}; + FillVertexData(rng, ref, src, VERTS, posStep == &VertexDecoder::Step_PosFloat ? specialPos : nullptr); const KnownVertexBounds initialBounds{ 0xFFFF, 0xFFFF, 0, 0 }; memset(refOut, 0, BUF_SIZE); gstate_c.vertexFullAlpha = true; gstate_c.vertBounds = initialBounds; - ref.DecodeVerts(refOut, src, &uvScale, VERTS); + // Vary the count a little, so the decoders' leftover-vertex paths get used too. + const int numVerts = VERTS - (formatsTested & 3); + ref.DecodeVerts(refOut, src, &uvScale, numVerts); const bool refFullAlpha = gstate_c.vertexFullAlpha; const KnownVertexBounds refBounds = gstate_c.vertBounds; memset(jitOut, 0, BUF_SIZE); gstate_c.vertexFullAlpha = true; gstate_c.vertBounds = initialBounds; - jit.DecodeVerts(jitOut, src, &uvScale, VERTS); + jit.DecodeVerts(jitOut, src, &uvScale, numVerts); const bool jitFullAlpha = gstate_c.vertexFullAlpha; const KnownVertexBounds jitBounds = gstate_c.vertBounds; - // Which step writes each component: weights (if any) first, then tc, col, nrm, pos. - int stepIndex = ref.weighttype ? 1 : 0; - StepFunction tcStep = ref.tc ? ref.steps_[stepIndex++] : nullptr; - StepFunction colStep = ref.col ? ref.steps_[stepIndex++] : nullptr; - StepFunction nrmStep = ref.nrm ? ref.steps_[stepIndex++] : nullptr; - StepFunction posStep = ref.steps_[stepIndex]; - char fmtDesc[256]{}; ref.ToString(fmtDesc, sizeof(fmtDesc), true); const char *kind = cache->IsInSpace((const u8 *)jit.jitted_) ? "jit" : "handwritten"; @@ -885,9 +895,25 @@ static bool TestVertexJitMatchesSteps() { } int badVerts = 0; std::string detail; - for (int v = 0; v < VERTS; v++) { + for (int v = 0; v < numVerts; v++) { const u8 *r = refOut + v * ref.decFmt.stride + off; const u8 *j = jitOut + v * ref.decFmt.stride + off; + if (specialPos[v] && step == posStep) { + // NaN and infinity only have to come out finite. How is up to the platform. + bool finite = true; + for (int c = 0; c < 3; c++) { + float fr, fj; + memcpy(&fr, r + c * 4, 4); + memcpy(&fj, j + c * 4, 4); + finite = finite && std::isfinite(fr) && std::isfinite(fj); + } + if (!finite && badVerts++ == 0) { + float fj[3]; + memcpy(fj, j, 12); + detail = StringFromFormat("vert %d: NaN/inf input gave %g %g %g", v, fj[0], fj[1], fj[2]); + } + continue; + } if (memcmp(r, j, sz) == 0) { continue; } @@ -1001,6 +1027,7 @@ static bool TestVertexJitMatchesSteps() { return mismatches.empty(); } + typedef bool (*VertexTestFunc)(); static VertexTestFunc vertdecTestFuncs[] = { @@ -1021,7 +1048,10 @@ static VertexTestFunc vertdecTestFuncs[] = { &TestVertex16Skin, &TestVertexFloatSkin, + // The other architectures' JITs haven't been brought in line yet. +#if PPSSPP_ARCH(AMD64) || PPSSPP_ARCH(ARM64) &TestVertexJitMatchesSteps, +#endif }; bool TestVertexJit() {