preserve-emulation/ppsspp
1
0
Fork 0
mirror of https://github.com/hrydgard/ppsspp.git synced 2026-09-04 19:55:27 +02:00
Code Issues Packages Projects Releases Wiki Activity

Emitters: fix a batch of encoding bugs

Browse Source 
Found by a review pass over Common/. Three of these affect code the JITs
actually emit today:

* ARM64 TryMOVI(8) returned true unconditionally ("can always do 8"), but MOVI
  with an 8-bit element replicates imm8 into every byte, so it can only encode a
  byte-uniform value. TryAnyMOVI always tries size 8 first, so it succeeded for
  every constant. MOVI2FDUP(FLT_MAX) - VertexDecoderArm64's Jit_PosFloat - came
  out as "movi v0.16b, #0xff", a quiet NaN, and since FMINNM/FMAXNM return the
  other operand for a quiet NaN, the infinity clamp silently did nothing.
  TryAnyMOVI's replication loop was also shifting by every bit position instead
  of by multiples of the element size, and it now only tries an element size the
  value actually repeats at. Regression test added.

* RISC-V SW()'s stack-pointer compression path called C_LWSP instead of C_SWSP,
  turning a store into a load that clobbers rs2 whenever autocompress is on
  (which RiscVJit and VertexDecoderRiscV both enable).

* LoongArch64 EncodeDFj passed the raw register enum instead of DecodeReg(fj),
  so bit 10 was always set and MOVFR2GR_S emitted movfr2gr.d - live in the
  LoongArch JIT's mfc1 and its FPU/vector compilers.

The rest have no callers today, but are wrong as written:

* ARM64: MOVI/MVNI computed the MSL cmode one too high (MSL #8 is 1100, not
  1101); TryMOVI's MVNI-with-MSL branch passed the value instead of its
  complement; TBZ/TBNZ put the register size in bit 31 where b5 belongs and
  didn't mask the bit index to 5 bits; the LDR/LDRSW/PRFM literal form checked
  the wrong mask for imm19 and wrote it unmasked; FCVTZS/FCVTZU's GPR-
  destination branch skipped DecodeReg and derived the type field from the GPR
  rather than from the float source.
* LoongArch64: LDPTR_D/STPTR_W/STPTR_D all passed Opcode32::LDPTR_W;
  AMCAS_DB_D duplicated AMSWAP_DB_D's opcode; EncodeJK shifted rk by 5 instead
  of 10; BYTEPICK_D masked its shift to 2 bits instead of 3.
* x64: VGATHERDPD/VGATHERQPS/VGATHERQPD used the wrong opcode/W combinations
  (only VGATHERDPS was right).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
This commit is contained in:
Henrik Rydgård
2026-08-30 13:44:34 +02:00
co-authored by Claude Opus 5
parent e4a0f649fa
commit 5f9131ed2b
5 changed files with 86 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Common/Arm64Emitter.cpp
+54 -24
View File
@@ -516,9 +516,13 @@ void ARM64XEmitter::EncodeTestBranchInst(u32 op, ARM64Reg Rt, u8 bits, const voi
_assert_msg_(distance >= -0x2000 && distance <= 0x1FFF, "%s: Received too large distance: %llx", __FUNCTION__, distance);
_assert_msg_(bits < (b64Bit ? 64 : 32), "%s: bit %d out of range for the register", __FUNCTION__, bits);
Rt = DecodeReg(Rt);
Write32((b64Bit << 31) | (0x36 << 24) | (op << 24) | \
(bits << 19) | (((u32)distance << 5) & 0x7FFE0) | Rt);
// Bit 31 is b5 (the high bit of the bit index), NOT the register size - and b40 is only 5 bits wide.
// SetJumpTarget's re-encoding of this same instruction gets this right.
Write32((((u32)bits & 0x20) << 26) | (0x36 << 24) | (op << 24) | \
(((u32)bits & 0x1F) << 19) | (((u32)distance << 5) & 0x7FFE0) | Rt);
}
void ARM64XEmitter::EncodeUnconditionalBranchInst(u32 op, const void* ptr)
@@ -662,12 +666,13 @@ void ARM64XEmitter::EncodeLoadRegisterInst(u32 bitop, ARM64Reg Rt, u32 imm)
bool b64Bit = Is64Bit(Rt);
bool bVec = IsVector(Rt);
_assert_msg_(!(imm & 0xFFFFF), "%s: offset too large %d", __FUNCTION__, imm);
// The literal offset field is imm19, at bits 23:5.
_assert_msg_(!(imm & ~0x7FFFFu), "%s: offset too large %d", __FUNCTION__, imm);
Rt = DecodeReg(Rt);
if (b64Bit && bitop != 0x2) // LDRSW(0x2) uses 64bit reg, doesn't have 64bit bit set
bitop |= 0x1;
Write32((bitop << 30) | (bVec << 26) | (0x18 << 24) | (imm << 5) | Rt);
Write32((bitop << 30) | (bVec << 26) | (0x18 << 24) | ((imm & 0x7FFFF) << 5) | Rt);
}
void ARM64XEmitter::EncodeLoadStoreExcInst(u32 instenc,
@@ -2261,12 +2266,13 @@ void ARM64FloatEmitter::FCVTZS(ARM64Reg Rd, ARM64Reg Rn, int scale) {
Write32((1 << 30) | (0 << 29) | (0x1F << 24) | (imm << 16) | (0x1F << 11) | (1 << 10) | (Rn << 5) | Rd);
} else {
// Rd is a GPR here and Rn is the float source, so the type comes from Rn - and both still need decoding.
bool sf = Is64Bit(Rd);
u32 type = 0;
if (IsDouble(Rd))
type = 1;
u32 type = IsDouble(Rn) ? 1 : 0;
int rmode = 3;
int opcode = 0;
Rd = DecodeReg(Rd);
Rn = DecodeReg(Rn);
Write32((sf << 31) | (0 << 29) | (0x1E << 24) | (type << 22) | (rmode << 19) | (opcode << 16) | (scale << 10) | (Rn << 5) | Rd);
@@ -2281,12 +2287,13 @@ void ARM64FloatEmitter::FCVTZU(ARM64Reg Rd, ARM64Reg Rn, int scale) {
Write32((1 << 30) | (1 << 29) | (0x1F << 24) | (imm << 16) | (0x1F << 11) | (1 << 10) | (Rn << 5) | Rd);
} else {
// Rd is a GPR here and Rn is the float source, so the type comes from Rn - and both still need decoding.
bool sf = Is64Bit(Rd);
u32 type = 0;
if (IsDouble(Rd))
type = 1;
u32 type = IsDouble(Rn) ? 1 : 0;
int rmode = 3;
int opcode = 1;
Rd = DecodeReg(Rd);
Rn = DecodeReg(Rn);
Write32((sf << 31) | (0 << 29) | (0x1E << 24) | (type << 22) | (rmode << 19) | (opcode << 16) | (scale << 10) | (Rn << 5) | Rd);
}
@@ -3489,7 +3496,7 @@ void ARM64FloatEmitter::MOVI(u8 size, ARM64Reg Rd, u8 imm8, u8 shift, bool MSL)
else if (size == 16)
cmode = 0b1000 | (shift >> 2);
else if (MSL)
cmode = 0b1100 | (shift >> 3);
cmode = 0b1100 | ((shift >> 3) - 1); // MSL #8 is cmode 1100, MSL #16 is cmode 1101.
else if (size == 32)
cmode = (shift >> 2);
else if (size == 64)
@@ -3510,7 +3517,7 @@ void ARM64FloatEmitter::MVNI(u8 size, ARM64Reg Rd, u8 imm8, u8 shift, bool MSL)
if (size == 16)
cmode = 0b1000 | (shift >> 2);
else if (MSL)
cmode = 0b1100 | (shift >> 3);
cmode = 0b1100 | ((shift >> 3) - 1); // MSL #8 is cmode 1100, MSL #16 is cmode 1101.
else if (size == 32)
cmode = (shift >> 2);
else
@@ -4231,11 +4238,12 @@ void ARM64FloatEmitter::MOVI2FDUP(ARM64Reg Rd, float value, ARM64Reg scratch, bo
if (negate) {
FNEG(32, Rd, Rd);
}
} else if (TryAnyMOVI(32, Rd, ival)) {
} else if (TryAnyMOVI(32, Rd, (uint32_t)ival)) {
// NOTE: The casts matter - a negative int would sign-extend into the upper 32 bits.
if (negate) {
FNEG(32, Rd, Rd);
}
} else if (TryAnyMOVI(32, Rd, ival ^ 0x80000000)) {
} else if (TryAnyMOVI(32, Rd, (uint32_t)(ival ^ 0x80000000))) {
if (!negate) {
FNEG(32, Rd, Rd);
}
@@ -4251,8 +4259,13 @@ void ARM64FloatEmitter::MOVI2FDUP(ARM64Reg Rd, float value, ARM64Reg scratch, bo
bool ARM64FloatEmitter::TryMOVI(u8 size, ARM64Reg Rd, uint64_t elementValue) {
if (size == 8) {
// Can always do 8.
MOVI(size, Rd, elementValue & 0xFF);
// MOVI with an 8-bit element replicates imm8 into every byte, so this only works if the
// value is byte-uniform - it is NOT "always possible", which is what this used to claim.
const uint8_t byte = (uint8_t)elementValue;
if (elementValue != byte * 0x0101010101010101ULL) {
return false;
}
MOVI(size, Rd, byte);
return true;
} else if (size == 16) {
if ((elementValue & 0xFF00) == 0) {
@@ -4291,7 +4304,8 @@ bool ARM64FloatEmitter::TryMOVI(u8 size, ARM64Reg Rd, uint64_t elementValue) {
MOVI(size, Rd, (elementValue >> shift) & 0xFF, shift, true);
return true;
} else if ((elementValue & mask) == notOnes) {
MVNI(size, Rd, (elementValue >> shift) & 0xFF, shift, true);
// MVNI inverts, so the imm8 to encode comes from the complement (same as the non-MSL case above).
MVNI(size, Rd, (~elementValue >> shift) & 0xFF, shift, true);
return true;
}
}
@@ -4315,25 +4329,41 @@ bool ARM64FloatEmitter::TryMOVI(u8 size, ARM64Reg Rd, uint64_t elementValue) {
return false;
}
// True if the 64-bit pattern is nothing but an elementSize-bit value repeated.
static bool IsRepeatedElement(uint64_t value, int elementSize) {
if (elementSize >= 64)
return true;
const uint64_t mask = (1ULL << elementSize) - 1ULL;
const uint64_t element = value & mask;
for (int i = elementSize; i < 64; i += elementSize) {
if (((value >> i) & mask) != element)
return false;
}
return true;
}
bool ARM64FloatEmitter::TryAnyMOVI(u8 size, ARM64Reg Rd, uint64_t elementValue) {
// Try the original size first in case that's more optimal.
if (TryMOVI(size, Rd, elementValue))
return true;
// Spread the element across the full 64 bits, so we can try the other element sizes against it.
uint64_t value = elementValue;
if (size != 64) {
uint64_t masked = elementValue & ((1ULL << size) - 1ULL);
for (int i = size; i < 64; ++i) {
const uint64_t masked = elementValue & ((1ULL << size) - 1ULL);
value = 0;
for (int i = 0; i < 64; i += size) {
value |= masked << i;
}
}
for (int attempt = 8; attempt <= 64; attempt += attempt) {
// Original size was already attempted above.
if (attempt != size) {
if (TryMOVI(attempt, Rd, value))
return true;
}
// A different element size can only express this value if the value actually repeats at
// that width - MOVI/MVNI replicate the element across the whole register.
if (!IsRepeatedElement(value, attempt))
continue;
if (TryMOVI(attempt, Rd, value))
return true;
}
return false;
Common/LoongArch64Emitter.cpp
+7 -9
View File
@@ -267,7 +267,7 @@ enum class Opcode32 {
AMCAS_W = 0x38590000,
AMCAS_DB_W = 0x385b0000,
AMCAS_D = 0x38598000,
AMCAS_DB_D = 0x38698000,
AMCAS_DB_D = 0x385b8000,
CRC_W_B_W = 0x00240000,
CRC_W_H_W = 0x00248000,
@@ -1292,7 +1292,7 @@ static inline u32 EncodeDJ(Opcode32 opcode, LoongArch64Reg rd, LoongArch64Reg rj
static inline u32 EncodeJK(Opcode32 opcode, LoongArch64Reg rj, LoongArch64Reg rk) {
_assert_msg_(IsGPR(rj), "JK instruction rj must be GPR");
_assert_msg_(IsGPR(rk), "JK instruction rk must be GPR");
return (u32)opcode | ((u32)rk << 5) | ((u32)rj << 5);
return (u32)opcode | ((u32)rk << 10) | ((u32)rj << 5);
}
static inline u32 EncodeDJKUa2(Opcode32 opcode, LoongArch64Reg rd, LoongArch64Reg rj, LoongArch64Reg rk, u8 sa2) {
@@ -1426,7 +1426,7 @@ static inline u32 EncodeFdJ(Opcode32 opcode, LoongArch64Reg fd, LoongArch64Reg r
static inline u32 EncodeDFj(Opcode32 opcode, LoongArch64Reg rd, LoongArch64Reg fj) {
_assert_msg_(IsGPR(rd), "DFj instruction rd must be GPR");
_assert_msg_(IsFPR(fj), "DFj instruction fj must be FPR");
return (u32)opcode | ((u32)fj << 5) | (u32)DecodeReg(rd);
return (u32)opcode | ((u32)DecodeReg(fj) << 5) | (u32)DecodeReg(rd);
}
static inline u32 EncodeJUd5(Opcode32 opcode, LoongArch64FCSR fcsr, LoongArch64Reg rj) {
@@ -2223,7 +2223,7 @@ void LoongArch64Emitter::BYTEPICK_W(LoongArch64Reg rd, LoongArch64Reg rj, LoongA
void LoongArch64Emitter::BYTEPICK_D(LoongArch64Reg rd, LoongArch64Reg rj, LoongArch64Reg rk, u8 sa3) {
_assert_msg_(rd != R_ZERO, "%s write to zero is a HINT", __func__);
Write32(EncodeDJKUa2(Opcode32::BYTEPICK_D, rd, rj, rk, sa3));
Write32(EncodeDJKUa3(Opcode32::BYTEPICK_D, rd, rj, rk, sa3));
}
void LoongArch64Emitter::REVB_2H(LoongArch64Reg rd, LoongArch64Reg rj) {
@@ -2543,17 +2543,15 @@ void LoongArch64Emitter::LDPTR_W(LoongArch64Reg rd, LoongArch64Reg rj, s16 si14)
void LoongArch64Emitter::LDPTR_D(LoongArch64Reg rd, LoongArch64Reg rj, s16 si14) {
_assert_msg_(rd != R_ZERO, "%s write to zero is a HINT", __func__);
Write32(EncodeDJSk14ps2(Opcode32::LDPTR_W, rd, rj, si14));
Write32(EncodeDJSk14ps2(Opcode32::LDPTR_D, rd, rj, si14));
}
void LoongArch64Emitter::STPTR_W(LoongArch64Reg rd, LoongArch64Reg rj, s16 si14) {
_assert_msg_(rd != R_ZERO, "%s write to zero is a HINT", __func__);
Write32(EncodeDJSk14ps2(Opcode32::LDPTR_W, rd, rj, si14));
Write32(EncodeDJSk14ps2(Opcode32::STPTR_W, rd, rj, si14));
}
void LoongArch64Emitter::STPTR_D(LoongArch64Reg rd, LoongArch64Reg rj, s16 si14) {
_assert_msg_(rd != R_ZERO, "%s write to zero is a HINT", __func__);
Write32(EncodeDJSk14ps2(Opcode32::LDPTR_W, rd, rj, si14));
Write32(EncodeDJSk14ps2(Opcode32::STPTR_D, rd, rj, si14));
}
void LoongArch64Emitter::PRELD(u32 hint, LoongArch64Reg rj, s16 si12) {
Common/RiscVEmitter.cpp
+7 -6
View File
@@ -1619,7 +1619,7 @@ void RiscVEmitter::SW(RiscVReg rs2, RiscVReg rs1, s32 simm12) {
C_SW(rs2, rs1, (u8)simm12);
return;
} else if (rs1 == R_SP && (simm12 & 0xFC) == simm12) {
C_LWSP(rs2, (u8)simm12);
C_SWSP(rs2, (u8)simm12);
return;
}
}
@@ -2555,8 +2555,9 @@ void RiscVEmitter::VSETIVLI(RiscVReg rd, u8 uimm5, VType vtype) {
_assert_msg_((vtype.value & ~0xFF) == 0, "%s with invalid vtype", __func__);
_assert_msg_(IsGPR(rd), "%s rd (VL) must be GPR", __func__);
_assert_msg_((u32)uimm5 <= 0x1F, "%s (AVL) can only set up to 31", __func__);
// vsetivli is distinguished from vsetvli by imm[11:10] == 0b11, so the vtype alone isn't enough.
s32 simm12 = 0xFFFFFC00 | vtype.value;
Write32(EncodeI(Opcode32::OP_V, rd, Funct3::OPCFG, (RiscVReg)uimm5, (s32)vtype.value));
Write32(EncodeI(Opcode32::OP_V, rd, Funct3::OPCFG, (RiscVReg)uimm5, simm12));
}
void RiscVEmitter::VSETVL(RiscVReg rd, RiscVReg rs1, RiscVReg rs2) {
@@ -3056,7 +3057,7 @@ void RiscVEmitter::VMINU_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm
}
void RiscVEmitter::VMINU_VX(RiscVReg vd, RiscVReg vs2, RiscVReg rs1, VUseMask vm) {
Write32(EncodeIVV(vd, rs1, vs2, vm, Funct6::VMINU));
Write32(EncodeIVX(vd, rs1, vs2, vm, Funct6::VMINU));
}
void RiscVEmitter::VMIN_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm) {
@@ -3064,7 +3065,7 @@ void RiscVEmitter::VMIN_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm)
}
void RiscVEmitter::VMIN_VX(RiscVReg vd, RiscVReg vs2, RiscVReg rs1, VUseMask vm) {
Write32(EncodeIVV(vd, rs1, vs2, vm, Funct6::VMIN));
Write32(EncodeIVX(vd, rs1, vs2, vm, Funct6::VMIN));
}
void RiscVEmitter::VMAXU_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm) {
@@ -3072,7 +3073,7 @@ void RiscVEmitter::VMAXU_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm
}
void RiscVEmitter::VMAXU_VX(RiscVReg vd, RiscVReg vs2, RiscVReg rs1, VUseMask vm) {
Write32(EncodeIVV(vd, rs1, vs2, vm, Funct6::VMAXU));
Write32(EncodeIVX(vd, rs1, vs2, vm, Funct6::VMAXU));
}
void RiscVEmitter::VMAX_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm) {
@@ -3080,7 +3081,7 @@ void RiscVEmitter::VMAX_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm)
}
void RiscVEmitter::VMAX_VX(RiscVReg vd, RiscVReg vs2, RiscVReg rs1, VUseMask vm) {
Write32(EncodeIVV(vd, rs1, vs2, vm, Funct6::VMAX));
Write32(EncodeIVX(vd, rs1, vs2, vm, Funct6::VMAX));
}
void RiscVEmitter::VMUL_VV(RiscVReg vd, RiscVReg vs2, RiscVReg vs1, VUseMask vm) {
Common/x64Emitter.cpp
+3 -3
View File
@@ -2375,15 +2375,15 @@ void XEmitter::VGATHERDPS(int bits, X64Reg regOp1, OpArg arg, X64Reg regOp2) {
}
void XEmitter::VGATHERDPD(int bits, X64Reg regOp1, OpArg arg, X64Reg regOp2) {
_assert_msg_(regOp1 != regOp2 && !arg.IsIndexedReg(regOp1) && !arg.IsIndexedReg(regOp2), "VGATHER cannot have overlapped registers");
WriteAVX2Op(bits, 0x66, 0x3893, regOp1, regOp2, arg);
WriteAVX2Op(bits, 0x66, 0x3892, regOp1, regOp2, arg, 0, 1);
}
void XEmitter::VGATHERQPS(int bits, X64Reg regOp1, OpArg arg, X64Reg regOp2) {
_assert_msg_(regOp1 != regOp2 && !arg.IsIndexedReg(regOp1) && !arg.IsIndexedReg(regOp2), "VGATHER cannot have overlapped registers");
WriteAVX2Op(bits, 0x66, 0x3892, regOp1, regOp2, arg);
WriteAVX2Op(bits, 0x66, 0x3893, regOp1, regOp2, arg);
}
void XEmitter::VGATHERQPD(int bits, X64Reg regOp1, OpArg arg, X64Reg regOp2) {
_assert_msg_(regOp1 != regOp2 && !arg.IsIndexedReg(regOp1) && !arg.IsIndexedReg(regOp2), "VGATHER cannot have overlapped registers");
WriteAVX2Op(bits, 0x66, 0x3893, regOp1, regOp2, arg);
WriteAVX2Op(bits, 0x66, 0x3893, regOp1, regOp2, arg, 0, 1);
}
void XEmitter::VPGATHERDD(int bits, X64Reg regOp1, OpArg arg, X64Reg regOp2) {
_assert_msg_(regOp1 != regOp2 && !arg.IsIndexedReg(regOp1) && !arg.IsIndexedReg(regOp2), "VPGATHER cannot have overlapped registers");
unittest/TestArm64Emitter.cpp
+15
View File
@@ -282,6 +282,21 @@ bool TestArm64Emitter() {
emitter.EORI2R(X1, X3, 0x3F0000003F0, INVALID_REG);
RET(CheckLast(emitter, "d21c1461 eor x1, x3, #0x3f0000003f0"));
// Regression test: TryMOVI(8) used to claim it could encode any value ("can always do 8"),
// and TryAnyMOVI always tries element size 8 first - so this emitted "movi v0.16b, #0xff",
// a quiet NaN, instead of FLT_MAX. That silently disabled the vertex decoder's infinity clamp,
// since FMINNM/FMAXNM just return the other operand when one side is a quiet NaN.
{
const u8 *start = emitter.GetCodePointer();
fp.MOVI2FDUP(Q0, FLT_MAX, X1);
EXPECT_EQ_INT((int)(emitter.GetCodePointer() - start), 8);
u32 instrs[2];
memcpy(instrs, start, sizeof(instrs));
// mvni v0.4s, #0x80, lsl #16 (that's -FLT_MAX), then negate it.
EXPECT_EQ_HEX(instrs[0], 0x6F044400);
EXPECT_EQ_HEX(instrs[1], 0x6EA0F800);
}
printf("ARM64 emitter test completed!\n");
//emitter.ANDI2R(W1, W3, 0xFF00FF00FF00FF00ULL, INVALID_REG);
//RET(CheckLast(emitter, "00000000 and x1, x3, 0xFF00FF00FF00FF00"));