From 029d17edfcfc5673fcf0be1830bc4a094dcf8f9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 19 Sep 2026 10:31:19 -0600 Subject: [PATCH] IR: fix FpCondFromReg operand slot, and some smaller IR bugs FpCondFromReg's meta is "_G", so the register is in src1 - which is where every native backend, PropagateConstants and ReorderLoadStore read it. But IRInterpret read it from dest, and Comp_VecDo3 wrote it to dest to match. The other emitter passes (0, MIPS_REG_ZERO), so both fields are zero there and the disagreement stayed hidden. The result was that vsge/vslt, which save fpcond to IRTEMP_0 and restore it afterwards, restored r0 (always zero) on the native IR JITs instead of the saved value, losing any c.cond.s result live across them. Fixed both the emitter and the interpreter to use src1. Vec4Pack31To8's SSE2 path computed (v >> 24) << 1, which is (v >> 23) with bit 23 forced to zero - the scalar and NEON paths both do (v >> 23) & 0xFF. Shift left first instead. The pspautotests inputs all happen to have bit 23 clear after the clamp, so this wasn't caught. Also: - Evaluate() didn't mask constant-folded shift amounts to 5 bits, though the neighbouring one-immediate path does. - ApplyMemoryValidation only invalidated its address-check cache for ops with a 'G' destination, while Interpret and CallReplacement can write any GPR - both are barriers, so drop the whole cache when we see one. - ReduceVec4Flush indexed isVec4 with (src2 & 3) instead of (src2 & ~3) for Vec4Scale; a missed optimization rather than a miscompile. Co-Authored-By: Claude Opus 5 (1M context) --- Core/MIPS/IR/IRCompVFPU.cpp | 2 +- Core/MIPS/IR/IRInterpreter.cpp | 8 +++++--- Core/MIPS/IR/IRPassSimplify.cpp | 17 +++++++++++------ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/Core/MIPS/IR/IRCompVFPU.cpp b/Core/MIPS/IR/IRCompVFPU.cpp index e5bf32f4e6..8d01ec3dcc 100644 --- a/Core/MIPS/IR/IRCompVFPU.cpp +++ b/Core/MIPS/IR/IRCompVFPU.cpp @@ -971,7 +971,7 @@ namespace MIPSComp { } if (type == VecDo3Op::VSGE || type == VecDo3Op::VSLT) { - ir.Write(IROp::FpCondFromReg, IRTEMP_0); + ir.Write(IROp::FpCondFromReg, 0, IRTEMP_0); } for (int i = 0; i < n; i++) { diff --git a/Core/MIPS/IR/IRInterpreter.cpp b/Core/MIPS/IR/IRInterpreter.cpp index 7a6a764951..a3bb3bcd82 100644 --- a/Core/MIPS/IR/IRInterpreter.cpp +++ b/Core/MIPS/IR/IRInterpreter.cpp @@ -486,8 +486,9 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst) { #if PPSSPP_ARCH(SSE2) __m128i src = _mm_loadu_si128((__m128i *) & mips->fi[inst->src1]); - // Shift each 32-bit lane right by 24 bits. Then left by 1. This matches the rather weird behavior. - src = _mm_slli_epi32(_mm_srli_epi32(src, 24), 1); + // Shift each 32-bit lane left by 1, then take the top byte - that is, (v >> 23) & 0xFF. + // Shifting right by 24 first would drop bit 23. + src = _mm_srli_epi32(_mm_slli_epi32(src, 1), 24); // Pack 32-bit lanes to 16-bit, then 16-bit to 8-bit // This moves our target bytes to the bottom of the XMM register src = _mm_packs_epi32(src, src); @@ -905,7 +906,8 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst) { } case IROp::FpCondFromReg: - mips->fpcond = mips->r[inst->dest]; + // Note: the register is in src1, see the "_G" meta - the native backends read it there. + mips->fpcond = mips->r[inst->src1]; break; case IROp::FpCondToReg: mips->r[inst->dest] = mips->fpcond; diff --git a/Core/MIPS/IR/IRPassSimplify.cpp b/Core/MIPS/IR/IRPassSimplify.cpp index 373093904d..1ea6fef644 100644 --- a/Core/MIPS/IR/IRPassSimplify.cpp +++ b/Core/MIPS/IR/IRPassSimplify.cpp @@ -23,10 +23,11 @@ u32 Evaluate(u32 a, u32 b, IROp op) { case IROp::And: case IROp::AndConst: return a & b; case IROp::Or: case IROp::OrConst: return a | b; case IROp::Xor: case IROp::XorConst: return a ^ b; - case IROp::Shr: case IROp::ShrImm: return a >> b; - case IROp::Sar: case IROp::SarImm: return (s32)a >> b; - case IROp::Ror: case IROp::RorImm: return (a >> b) | (a << (32 - b)); - case IROp::Shl: case IROp::ShlImm: return a << b; + // MIPS (and IRInterpret) only look at the low 5 bits of a variable shift amount. + case IROp::Shr: case IROp::ShrImm: return a >> (b & 31); + case IROp::Sar: case IROp::SarImm: return (s32)a >> (b & 31); + case IROp::Ror: case IROp::RorImm: return __rotr(a, b & 31); + case IROp::Shl: case IROp::ShlImm: return a << (b & 31); case IROp::Slt: case IROp::SltConst: return ((s32)a < (s32)b); case IROp::SltU: case IROp::SltUConst: return (a < b); default: @@ -1886,7 +1887,11 @@ bool ApplyMemoryValidation(const IRWriter &in, IRWriter &out, const IROptions &o } const IRMeta *m = GetIRMeta(inst.op); - if (m->types[0] == 'G' && (m->flags & IRFLAG_SRC3) == 0) { + if ((m->flags & IRFLAG_BARRIER) != 0) { + // Interpret runs an arbitrary instruction and CallReplacement a whole function, so + // either can write any GPR - no address we validated earlier can be trusted after one. + checks.clear(); + } else if (m->types[0] == 'G' && (m->flags & IRFLAG_SRC3) == 0) { uint64_t key = (uint64_t)inst.dest << 32; // Wipe out all the already done checks since this was modified. checks.erase(checks.lower_bound(key), checks.upper_bound(key | 0xFFFFFFFFULL)); @@ -2148,7 +2153,7 @@ bool ReduceVec4Flush(const IRWriter &in, IRWriter &out, const IROptions &opts) { out.Write(inst.op, inst.dest, inst.src1, temp, inst.constant); skip = true; inst.src2 = IRREG_INVALID; - } else if (isVec4[inst.src2 & 3] && usedLaterAsVec4(inst.src2 & ~3) && findAvailTempVec4()) { + } else if (isVec4[inst.src2 & ~3] && usedLaterAsVec4(inst.src2 & ~3) && findAvailTempVec4()) { out.Write(IROp::FMov, temp, inst.src2); out.Write(inst.op, inst.dest, inst.src1, temp, inst.constant); skip = true;