IR: Fix PurgeTemps miscompiles

- Copy propagation through an FPR temp didn't stop when an instruction
  rewrote the temp in place (it compared an FPR number against the +32
  offset reg), so later reads lost that write.
- A read of the temp in both operands only had src1 replaced, yet the copy
  into the temp was still removed.
- The replacement matched operands by number without checking their type,
  so a StoreFloat whose GPR address had the temp's number got its address
  replaced (IRVTEMP_PFX_S and IRTEMP_0 are both 192).
- A write to lanes 1-3 of a Vec4 temp wasn't noticed.
- IRReadsFromFPRs stopped after the F operands, missing Vec4Scale's vector.
- Exits and barriers didn't count as reading everything, so a write to a
  real reg could be moved above an exit.
- Load32Linked and Store32Conditional were removed when their reg was
  overwritten unread, losing LLBIT and the store.

Also fixes an off-by-one in the vec src3 read check. The unit test now
reports every failing case instead of stopping at the first.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

Core/MIPS/IR/IRAnalysis.cpp

@@ -59,9 +59,9 @@ bool IRReadsFromFPR(const IRInstMeta &inst, int reg, bool *directly) {
 	if (inst.m.types[2] == '2' && reg >= inst.src2 && reg < inst.src2 + 2)
 		return true;
 	if ((inst.m.flags & (IRFLAG_SRC3 | IRFLAG_SRC3DST)) != 0) {
-		if (inst.m.types[0] == 'V' && reg >= inst.src3 && reg <= inst.src3 + 4)
+		if (inst.m.types[0] == 'V' && reg >= inst.src3 && reg < inst.src3 + 4)
 			return true;
-		if (inst.m.types[0] == '2' && reg >= inst.src3 && reg <= inst.src3 + 2)
+		if (inst.m.types[0] == '2' && reg >= inst.src3 && reg < inst.src3 + 2)
 			return true;
 	}
 	return false;
@@ -77,9 +77,8 @@ static int IRReadsFromList(const IRInstMeta &inst, IRReg regs[4], char type) {
 	if ((inst.m.flags & (IRFLAG_SRC3 | IRFLAG_SRC3DST)) != 0 && inst.m.types[0] == type)
 		regs[c++] = inst.src3;
 
-	if (inst.op == IROp::Interpret || inst.op == IROp::CallReplacement || inst.op == IROp::Syscall || inst.op == IROp::SyscallUnresolved ||inst.op == IROp::Break)
-		return -1;
-	if (inst.op == IROp::Breakpoint || inst.op == IROp::MemoryCheck)
+	// Exits read every reg that lives on, and barriers may read anything.
+	if ((inst.m.flags & (IRFLAG_EXIT | IRFLAG_BARRIER)) != 0)
 		return -1;
 
 	return c;
@@ -142,8 +141,8 @@ int IRReadsFromGPRs(const IRInstMeta &inst, IRReg regs[4]) {
 
 int IRReadsFromFPRs(const IRInstMeta &inst, IRReg regs[16]) {
 	int c = IRReadsFromList(inst, regs, 'F');
-	if (c != 0)
-		return c;
+	if (c == -1)
+		return -1;
 
 	// We also need to check V and 2.  Indirect reads already checked, don't check again.
 	if (inst.m.types[1] == 'V' || inst.m.types[1] == '2') {

Core/MIPS/IR/IRPassSimplify.cpp

@@ -1007,6 +1007,21 @@ bool PurgeTemps(const IRWriter &in, IRWriter &out, const IROptions &opts) {
 	memset(lastWrittenTo, -1, sizeof(lastWrittenTo));
 	memset(lastReadFrom, -1, sizeof(lastReadFrom));
 
+	auto writesToFPRCheck = [](const IRInstMeta &inst, const Check &check) {
+		for (int i = 0; i < check.fplen; ++i) {
+			if (IRWritesToFPR(inst, check.reg - 32 + i))
+				return true;
+		}
+		return false;
+	};
+
+	auto nukeCheckedInst = [&](Check &check) {
+		insts[check.index].op = IROp::Mov;
+		insts[check.index].dest = 0;
+		insts[check.index].src1 = 0;
+		check.reg = 0;
+	};
+
 	auto readsFromFPRCheck = [](IRInstMeta &inst, Check &check, bool *directly) {
 		if (check.reg < 32)
 			return false;
@@ -1112,25 +1127,29 @@ bool PurgeTemps(const IRWriter &in, IRWriter &out, const IROptions &opts) {
 				checkMismatch(inst.src1, inst.m.types[1]);
 				checkMismatch(inst.src2, inst.m.types[2]);
 				if ((inst.m.flags & (IRFLAG_SRC3 | IRFLAG_SRC3DST)) != 0)
-					checkMismatch(inst.src3, inst.m.types[3]);
+					checkMismatch(inst.src3, inst.m.types[0]);
 
 				bool cannotReplace = !readsDirectly || lenMismatch;
 				if (!cannotReplace && check.srcReg >= 32 && lastWrittenTo[check.srcReg] < check.index) {
 					// This is probably not worth doing unless we can get rid of a temp.
 					if (!check.readByExit) {
-						if (insts[check.index].dest == inst.src1)
-							inst.src1 = check.srcReg - 32;
-						else if (insts[check.index].dest == inst.src2)
-							inst.src2 = check.srcReg - 32;
-						else
-							_assert_msg_(false, "Unexpected src3 read of FPR");
+						// Replace every F operand that reads it (it might be read twice).
+						const IRReg reg = (IRReg)(check.reg - 32);
+						const IRReg srcReg = (IRReg)(check.srcReg - 32);
+						if (inst.m.types[1] == 'F' && inst.src1 == reg)
+							inst.src1 = srcReg;
+						if (inst.m.types[2] == 'F' && inst.src2 == reg)
+							inst.src2 = srcReg;
+						if ((inst.m.flags & (IRFLAG_SRC3 | IRFLAG_SRC3DST)) != 0 && inst.m.types[0] == 'F' && inst.src3 == reg)
+							inst.src3 = srcReg;
 
-						// Check if we've clobbered it entirely.
-						if (inst.dest == check.reg) {
+						// If this also writes the reg, the check ends here. A full overwrite leaves the
+						// original write dead, since every read in between now uses srcReg.
+						if (writesToFPRCheck(inst, check)) {
+							IRReg destFPRs[4];
+							if (IRDestFPRs(inst, destFPRs) == check.fplen && inst.dest + 32 == check.reg)
+								nukeCheckedInst(check);
 							check.reg = 0;
-							insts[check.index].op = IROp::Mov;
-							insts[check.index].dest = 0;
-							insts[check.index].src1 = 0;
 						}
 					} else {
 						// Let's not bother.
@@ -1176,17 +1195,14 @@ bool PurgeTemps(const IRWriter &in, IRWriter &out, const IROptions &opts) {
 				insts[check.index].dest = 0;
 				insts[check.index].src1 = 0;
 				check.reg = 0;
-			} else if (IRWritesToFPR(inst, check.reg - 32) && check.fplen >= 1) {
+			} else if (check.fplen >= 1 && writesToFPRCheck(inst, check)) {
 				IRReg destFPRs[4];
 				int numFPRs = IRDestFPRs(inst, destFPRs);
 
 				if (numFPRs == check.fplen && inst.dest + 32 == check.reg) {
 					// This means we've clobbered it, and with full overlap.
 					// Sometimes this happens for non-temps, i.e. vmmov + vinit last row.
-					insts[check.index].op = IROp::Mov;
-					insts[check.index].dest = 0;
-					insts[check.index].src1 = 0;
-					check.reg = 0;
+					nukeCheckedInst(check);
 				} else {
 					// Since there's an overlap, we simply cannot optimize.
 					check.reg = 0;
@@ -1230,6 +1246,10 @@ bool PurgeTemps(const IRWriter &in, IRWriter &out, const IROptions &opts) {
 				// These might sometimes be implicitly read/written by other instructions.
 				break;
 			}
+			if (inst.op == IROp::Store32Conditional || inst.op == IROp::Load32Linked) {
+				// These do more than write the reg (the store, and LLBIT), so they must stay.
+				break;
+			}
 			checks.push_back(Check(dest, i, true));
 			break;
 

unittest/TestIRPassSimplify.cpp

@@ -69,7 +69,9 @@ static bool VerifyPass(const IRVerification &v) {
 				continue;
 			}
 
-			printf("%s FAILED: #%d expected '%s' but was '%s'", v.name, (int)i, expectedBuf, actualBuf);
+			printf("%s FAILED: #%d expected '%s' but was '%s'\n", v.name, (int)i, expectedBuf, actualBuf);
+			printf("Actual:\n");
+			LogInstructions(actual);
 			return false;
 		}
 	}
@@ -166,15 +168,115 @@ static const IRVerification tests[] = {
 		},
 		{ &PropagateConstants },
 	},
+	{
+		// The FNeg reads the temp and overwrites it, so the FAdd must see the negation.
+		"PurgeTempsFPRRewrittenInPlace",
+		{
+			{ IROp::FMov, { IRVTEMP_PFX_S }, 5 },
+			{ IROp::FNeg, { IRVTEMP_PFX_S }, IRVTEMP_PFX_S },
+			{ IROp::FAdd, { 0 }, IRVTEMP_PFX_S, 1 },
+		},
+		{
+			{ IROp::FNeg, { IRVTEMP_PFX_S }, 5 },
+			{ IROp::FAdd, { 0 }, IRVTEMP_PFX_S, 1 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		"PurgeTempsFPRReadTwice",
+		{
+			{ IROp::FMov, { IRVTEMP_PFX_S }, 5 },
+			{ IROp::FMul, { 0 }, IRVTEMP_PFX_S, IRVTEMP_PFX_S },
+		},
+		{
+			{ IROp::FMul, { 0 }, 5, 5 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		// The FPR temp has the same number as IRTEMP_0, which is the address here.
+		"PurgeTempsFPRStoreSrc3",
+		{
+			{ IROp::FMov, { IRVTEMP_PFX_S }, 5 },
+			{ IROp::StoreFloat, { IRVTEMP_PFX_S }, IRTEMP_0, 0, 0x10 },
+		},
+		{
+			{ IROp::StoreFloat, { 5 }, IRTEMP_0, 0, 0x10 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		// The FMov writes lane 1 of the temp between its write and the Vec4Mov.
+		"PurgeTempsVec4LaneWrite",
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::FMov, { IRVTEMP_0 + 1 }, 20 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::FMov, { IRVTEMP_0 + 1 }, 20 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		// The Vec4Scale reads 48 before the Vec4Mov writes it.
+		"PurgeTempsVec4ScaleRead",
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::Vec4Scale, { 40 }, 48, 1 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::Vec4Scale, { 40 }, 48, 1 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		// Writing 48 before the exit would change it on the path that exits.
+		"PurgeTempsSwapAcrossExit",
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::ExitToConstIfEq, { 0 }, MIPS_REG_A0, MIPS_REG_A1, 0x08804000 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{
+			{ IROp::Vec4Add, { IRVTEMP_0 }, 32, 36 },
+			{ IROp::ExitToConstIfEq, { 0 }, MIPS_REG_A0, MIPS_REG_A1, 0x08804000 },
+			{ IROp::Vec4Mov, { 48 }, IRVTEMP_0 },
+		},
+		{ &PurgeTemps },
+	},
+	{
+		// sc stores and ll sets LLBIT, so neither goes away when the reg is overwritten.
+		"PurgeTempsKeepsLLSC",
+		{
+			{ IROp::Load32Linked, { MIPS_REG_V1 }, MIPS_REG_A0, 0, 0 },
+			{ IROp::SetConst, { MIPS_REG_V1 }, 0, 0, 1 },
+			{ IROp::Store32Conditional, { MIPS_REG_V0 }, MIPS_REG_A0, 0, 0 },
+			{ IROp::SetConst, { MIPS_REG_V0 }, 0, 0, 1 },
+		},
+		{
+			{ IROp::Load32Linked, { MIPS_REG_V1 }, MIPS_REG_A0, 0, 0 },
+			{ IROp::SetConst, { MIPS_REG_V1 }, 0, 0, 1 },
+			{ IROp::Store32Conditional, { MIPS_REG_V0 }, MIPS_REG_A0, 0, 0 },
+			{ IROp::SetConst, { MIPS_REG_V0 }, 0, 0, 1 },
+		},
+		{ &PurgeTemps },
+	},
 };
 
 bool TestIRPassSimplify() {
 	InitIR();
 
+	bool success = true;
 	for (const auto &test : tests) {
 		if (!VerifyPass(test))
-			return false;
+			success = false;
 	}
 
-	return true;
+	return success;
 }