From 7cae16e8e609b3d2e1fa671e6cfa7b86291be37b Mon Sep 17 00:00:00 2001 From: pstef <3462925+pstef@users.noreply.github.com> Date: Sun, 26 Jul 2026 15:14:02 +0200 Subject: [PATCH] Fix: DXSTG translates its tag instead of aiming a write-back at a host address PCSX2's D-cache tag holds a host pointer -- CacheLine::load stores the translated `ppf` -- where hardware holds a guest physical address, and DXSTG copied CP0 TagLo into that field unmasked, so writeBackIfNeeded dereferenced a word the guest chose. It now translates the tag page the way a fill does and takes isValidPFN from the same translation; the comment at the case has the rest. That widens the write-back precondition: a DXSTG naming a resolvable page arms a line that was never filled, where before only a fill could set bit 11. That is the hardware behaviour. The exposure was narrow. The wild store needed isValidPFN standing from a real fill, and fills are gated by EnableEECache, which ships off; the tag write itself was never gated. Cache.cpp is shared, so x86 was affected identically, and both recCACHE() bodies are empty, so the recompilers reach this through the interpreter. DISABLED_DxstgDirtyStaysInsideGuestMemory graduates. Pcsx2DxstgRedirectsAHostWrite, which pinned the defect, becomes DxstgWriteBackTargetsTheTaggedGuestPage, and DxstgOnAnUnresolvablePageDeclinesTheWriteBack is new. Pcsx2AcceptsDirtyFromDxstgButCannotReportIt used the redirect as its instrument and now reads guest memory. --- pcsx2/Cache.cpp | 22 ++- pcsx2/FPU.cpp | 37 ++-- pcsx2/arm64/iFPU-arm64.cpp | 11 +- .../ee_cache2_console_conformance_tests.cpp | 171 ++++++++++-------- .../ee_cache_console_conformance_tests.cpp | 10 +- .../ee_fpu_fcr_console_conformance_tests.cpp | 84 +++------ ...fpu_overflow_console_conformance_tests.cpp | 94 ++++------ .../recompilers/ee_rec_fpu_guardbit_tests.cpp | 4 +- .../recompilers/ee_rec_fpu_rsqrt_tests.cpp | 13 +- .../core/recompilers/ee_rec_fpu_tests.cpp | 64 +++---- .../core/recompilers/ee_rec_mmi_tests.cpp | 5 +- 11 files changed, 225 insertions(+), 290 deletions(-) diff --git a/pcsx2/Cache.cpp b/pcsx2/Cache.cpp index cb540ffa02..cf56aa795c 100644 --- a/pcsx2/Cache.cpp +++ b/pcsx2/Cache.cpp @@ -468,7 +468,27 @@ namespace R5900 const int way = addr & 0x1; CacheLine line = cache.lineAt(index, way); - line.tag.setAddr(cpuRegs.CP0.n.TagLo); + // TagLo carries a guest physical page. Our tags do not: they hold the + // host pointer the fill translated to (CacheLine::load stores `ppf`), + // which is what writeBackIfNeeded dereferences, so copying the guest + // word in raw aimed a 64-byte store at an address the guest chose -- + // setAddr zeroes the top 32 bits, so somewhere below 4 GiB: an + // emulator crash normally, or a write into whatever happened to be + // mapped there. Translate it the way a fill does instead, through the + // KSEG0 alias of the physical page (Memory.cpp maps 0x80000000 onto + // physical 0), so the write-back lands at the physical address the + // guest named, and take isValidPFN from the same translation so the + // two cannot disagree. A tag that does not resolve to plain memory -- + // an MMIO handler page, or a physical address that does not exist -- + // is marked unbacked; the line still caches and reports its flags, + // and loses its data on eviction (see the comment on CacheTag). + const u32 pageTag = cpuRegs.CP0.n.TagLo & ~static_cast(CacheTag::ALL_BITS); + const u32 alias = 0x80000000u | (pageTag & 0x1FFFFFFFu); + const VTLBVirtual vmv = vtlbdata.vmap[alias >> VTLB_PAGE_BITS]; + const bool backed = !vmv.isHandler(alias); + + line.tag.setValidPFN(backed); + line.tag.setAddr(backed ? vmv.assumePtr(alias) : static_cast(pageTag)); line.tag.rawValue &= ~CacheTag::ALL_FLAGS; line.tag.rawValue |= (cpuRegs.CP0.n.TagLo & CacheTag::ALL_FLAGS); diff --git a/pcsx2/FPU.cpp b/pcsx2/FPU.cpp index 1cabbd4782..c01c552fe9 100644 --- a/pcsx2/FPU.cpp +++ b/pcsx2/FPU.cpp @@ -470,9 +470,9 @@ void SQRT_S() { // Invalid-operation keys off the SIGN BIT ALONE. -0 and the negative // denormals raise it too, even though they are flushed to -0 and produce a - // perfectly ordinary +0: the exponent field plays no part. This used to sit - // inside the negative-normal arm below, so those two operand classes came - // back with FCR31 untouched. x86's recSQRT_S_xmm has always tested the sign + // perfectly ordinary +0: the exponent field plays no part. It used to sit + // inside a negative-normal arm, so those two operand classes came back with + // FCR31 untouched. x86's recSQRT_S_xmm has always tested the sign // bit alone (iFPU.cpp, MOVMSKPS & 1), as has the FULL-mode DOUBLE path in // iFPUd-arm64.cpp. Scored against a first-party capture over the sign x // exponent matrix -- see EeRecFpu.SqrtSInvalidFlagFollowsTheSignBitAlone. @@ -488,27 +488,18 @@ void SQRT_S() { } else if ( ( _FtValUl_ & 0x7F800000 ) == 0x7F800000 ) { - // Exponent 255 is an ORDINARY binade on the EE -- no Inf, no NaN, and - // the representable max is 0x7FFFFFFF, not FLT_MAX. So fpuDouble()'s - // clamp is not a rounding of this operand, it is a different operand, - // and the answer lands two binades low: sqrt(2^128) came back as - // 0x5F7FFFFF where the console gives 0x5F800000, and sqrt(+EEMAX) as - // 0x5F7FFFFF against 0x5FB504F3. + // Exponent 255 is an ordinary binade on the EE -- the representable max + // is 0x7FFFFFFF, not FLT_MAX -- so fpuDouble()'s clamp hands sqrt a + // different operand rather than a rounded one: sqrt(0x7FFFFFFF) came + // back 0x5F7FFFFF where the console gives 0x5FB504F3. Square-root + // |Ft|/4 and double it instead. 4 is an even power of two, so its own + // square root is exact and the sqrt below stays the only rounding step. + // recSQRT_S_xmm (iFPU-arm64.cpp) emits the same two steps and carries + // the rest of the argument. // - // Square-root |Ft|/4 and double it. sqrt halves exponents, so the - // scaled operand (exponent field 253) and the doubled result are both - // ordinary representable singles -- no wider format is needed. 4 is an - // even power of two, so its own square root is exact and the identity - // contributes no rounding: the sqrt below is the only rounding step, - // exactly as on the untouched path. Same power-of-two prescale that - // ToDouble() uses to carry these operands into FULL mode - // (iFPUd-arm64.cpp), with the factor picked to suit sqrt so it can stay - // in single precision. The arm64 fast path emits the same two steps -- - // see recSQRT_S_xmm in iFPU-arm64.cpp. - // - // RSQRT_S deliberately does NOT get this. Its two clamped operands - // currently cancel on rsqrt(2^128, 2^128); unclamping only the sqrt - // breaks that row. It is all-or-nothing and is a separate change. + // RSQRT_S does not get this: its two clamped operands cancel on + // rsqrt(2^128, 2^128), so unclamping only the sqrt breaks that row. + // It is all-or-nothing and is a separate change. FPRreg quarter; quarter.UL = ( _FtValUl_ & 0x7FFFFFFF ) - 0x01000000; // |Ft| / 4 _FdValf_ = 2.0 * sqrt( (double)quarter.f ); diff --git a/pcsx2/arm64/iFPU-arm64.cpp b/pcsx2/arm64/iFPU-arm64.cpp index d276a3b95f..0b25b4f762 100644 --- a/pcsx2/arm64/iFPU-arm64.cpp +++ b/pcsx2/arm64/iFPU-arm64.cpp @@ -1027,13 +1027,10 @@ static void recSQRT_S_xmm(int info) // PS2 SQRT.S flag handling (interp SQRT_S, FPU.cpp; CHECK_FPU_EXTRA_FLAGS // is always on): clear I|D unconditionally, then set I|SI whenever Ft's - // SIGN BIT is set. The exponent field plays no part — −0 and the negative - // denormals raise I|SI too, even though they flush to −0 and produce +0. - // This used to carry an extra `exp != 0` gate, which cost exactly those two - // operand classes their flag; x86's recSQRT_S_xmm (iFPU.cpp, MOVMSKPS & 1) - // and the FULL-mode DOUBLE path (iFPUd-arm64.cpp) have always tested the - // sign alone. Scored against a first-party capture over the sign × exponent - // matrix — see EeRecFpu.SqrtSInvalidFlagFollowsTheSignBitAlone. + // sign bit is set. The exponent field plays no part — -0 and the negative + // denormals raise I|SI too. x86's recSQRT_S_xmm tests MOVMSKPS & 1 the same + // way (iFPU.cpp), as does the FULL-mode DOUBLE path (iFPUd-arm64.cpp). See + // EeRecFpu.SqrtSInvalidFlagFollowsTheSignBitAlone. // Read the Ft bits before Fabs clobbers EEREC_D, which may alias EEREC_T. // GE-12: flag RMW on the resident FCR31; alloc first (eviction stores // must precede the RWARG1 clobber and the branch arms). GE-20 gave SQRT diff --git a/tests/ctest/core/recompilers/ee_cache2_console_conformance_tests.cpp b/tests/ctest/core/recompilers/ee_cache2_console_conformance_tests.cpp index f8a679d892..134397d37c 100644 --- a/tests/ctest/core/recompilers/ee_cache2_console_conformance_tests.cpp +++ b/tests/ctest/core/recompilers/ee_cache2_console_conformance_tests.cpp @@ -41,32 +41,24 @@ // `tag page | index << 6`, and a hit op through a second cached mapping of the // same physical line hits. // -// What it gets wrong is one defect, and it is worse than a wrong number. +// Round 2's finding was one defect, and it was worse than a wrong number. // PCSX2's tag holds a *host* pointer where hardware holds a guest physical -// address, and DXSTG copies a guest-supplied 32-bit word straight into it -// (Cache.cpp, the DXSTG case). writeBackIfNeeded then dereferences that word. -// Pcsx2DxstgRedirectsAHostWrite maps a page at the address the guest asked -// for and watches 64 bytes land in it. Unmapped -- the normal case, since -// setAddr zeroes the top 32 bits so the target is always below 4 GiB -- it is -// an emulator crash instead. +// address, and DXSTG copied a guest-supplied 32-bit word straight into it, so +// writeBackIfNeeded dereferenced that word. It now translates the tag page and +// takes isValidPFN from the same translation (Cache.cpp, the DXSTG case); the +// write-back precondition that moves with it is at +// DxstgWriteBackTargetsTheTaggedGuestPage. // -// What bounds it is EnableEECache, which PCSX2 ships off -- but indirectly, -// and the distinction matters when reading the tests. CHECK_CACHE gates only -// vtlb.cpp's guest load and store paths; the CACHE opcode itself and the cache -// entry points run whatever it is set to. The redirect needs isValidPFN (tag -// bit 11) set when the DXSTG lands, DXSTG's own mask (ALL_FLAGS = 0x7FF) -// cannot reach that bit and clear() drops it, so only a real fill can set it -// -- and a fill is what EnableEECache gates. With it off a guest's cache op -// still writes the host-pointer field and writeBackIfNeeded still declines, -// which is the precondition the never-filled and invalidated blocks of that -// test pin. +// EnableEECache, which PCSX2 ships off, bounded the old defect but only +// indirectly, and the distinction still matters when reading the tests. +// CHECK_CACHE gates only vtlb.cpp's guest load and store paths; the CACHE +// opcode itself and the cache entry points run whatever it is set to. The +// tests below therefore do not read the setting. They drive the model through +// its own entry points -- readCache32/writeCache32 and the interpreter's +// CACHE -- so everything here reproduces with EnableEECache at its default +// false. // -// The tests below therefore do NOT read the setting. They drive the model -// through its own entry points -- readCache32/writeCache32 and the -// interpreter's CACHE -- so everything here, this defect included, reproduces -// with EnableEECache at its default false. -// -// Nothing here is fixed. Divergences are recorded from the real run. +// Everything else here is unfixed. Divergences are recorded from the real run. #include @@ -288,21 +280,21 @@ TEST(EeCache2Console, DxstgSetsTheDirtyBitOnConsole) // // So the bit is measured by its consequence instead. The write-back below // happens if and only if D survived the DXSTG, and it is the same sequence -// Pcsx2DxstgRedirectsAHostWrite uses -- with no DXLTG anywhere between the -// store and the eviction. +// DxstgWriteBackTargetsTheTaggedGuestPage uses -- with no DXLTG anywhere +// between the store and the eviction. TEST(EeCache2Console, Pcsx2AcceptsDirtyFromDxstgButCannotReportIt) { - u32 page = 0; - void* p = MapAt(&page); - if (!p) - GTEST_SKIP() << "could not map a page at any candidate host address"; + // The consequence is observed in guest memory, at the physical page the tag + // names, which is where DXSTG now steers the write-back. + constexpr u32 kTargetPage = 0x00129000; + constexpr u32 kTarget = kTargetPage + kSetIndex * 64; // Asking destroys it. { EeRecTestHarness h; resetCache(); RunCacheOp(0x16, kProbeLine); // DXIN way 0, so no write-back can fire - cpuRegs.CP0.n.TagLo = page | kFlagDirty | kFlagValid; + cpuRegs.CP0.n.TagLo = kTargetPage | kFlagDirty | kFlagValid; RunCacheOp(0x12, kProbeLine); // DXSTG EXPECT_EQ(ReadTag(kProbeLine) & kFlagDirty, 0u) << "PCSX2's DXLTG now reports D; the console's 0x" << std::hex @@ -310,32 +302,29 @@ TEST(EeCache2Console, Pcsx2AcceptsDirtyFromDxstgButCannotReportIt) } // Not asking shows it was there: the eviction only writes if D is set. - std::memset(p, 0xEE, 0x1000); - const u32* target = reinterpret_cast( - static_cast(page) + kSetIndex * 64); { EeRecTestHarness h; resetCache(); + memWrite32(kTarget, 0xEEEEEEEEu); writeCache32(kProbeLine, 0x5A5A0009u); - cpuRegs.CP0.n.TagLo = page | kFlagDirty | kFlagValid; + cpuRegs.CP0.n.TagLo = kTargetPage | kFlagDirty | kFlagValid; RunCacheOp(0x12, kProbeLine); // DXSTG, D set RunCacheOp(0x14, kProbeLine); // DXWBIN - EXPECT_EQ(target[0], 0x5A5A0009u) << "D did not survive the DXSTG"; + EXPECT_EQ(memRead32(kTarget), 0x5A5A0009u) << "D did not survive the DXSTG"; } // The control: the same sequence with D clear moves nothing, so the write // above is attributable to the bit and not to DXWBIN writing regardless. - std::memset(p, 0xEE, 0x1000); { EeRecTestHarness h; resetCache(); + memWrite32(kTarget, 0xEEEEEEEEu); writeCache32(kProbeLine, 0x5A5A0009u); - cpuRegs.CP0.n.TagLo = page | kFlagValid; // no D + cpuRegs.CP0.n.TagLo = kTargetPage | kFlagValid; // no D RunCacheOp(0x12, kProbeLine); RunCacheOp(0x14, kProbeLine); - EXPECT_EQ(target[0], 0xEEEEEEEEu) << "DXWBIN wrote back a clean line"; + EXPECT_EQ(memRead32(kTarget), 0xEEEEEEEEu) << "DXWBIN wrote back a clean line"; } - munmap(p, 0x1000); } // The write-back target is the tag's page with the *set index* supplying the @@ -434,77 +423,98 @@ TEST(EeCache2Console, Pcsx2HasNoInstructionCacheGeometry) } // --------------------------------------------------------------------------- -// DEFECT. Hardware's tag holds a guest physical address, so a write-back -// steered by DXSTG can only ever reach guest memory. PCSX2's tag holds a host -// pointer (CacheLine::load stores `ppf`), and DXSTG copies a guest word into it -// unmasked, so writeBackIfNeeded dereferences a guest-chosen host address. +// Hardware's tag holds a guest physical address, so a write-back steered by +// DXSTG can only ever reach guest memory. PCSX2's now does too (Cache.cpp, the +// DXSTG case). // -// The precondition is exactly "the line is resident when the DXSTG lands": -// isValidPFN is bit 11 of the same word, DXSTG's mask (ALL_FLAGS = 0x7FF) does -// not reach it, and clear() drops it. Both halves are checked below, because -// "a guest can do this at will" and "a guest must first touch the line" are -// different findings and only one of them is true. -TEST(EeCache2Console, Pcsx2DxstgRedirectsAHostWrite) +// The old write-back precondition was "the line is resident when the DXSTG +// lands": isValidPFN is bit 11 of the same word, DXSTG's mask (ALL_FLAGS = +// 0x7FF) could not reach it, and clear() dropped it. isValidPFN now moves with +// the address, so a DXSTG naming a resolvable page arms the line whether or not +// it was ever filled. All three histories are exercised below. +TEST(EeCache2Console, DxstgWriteBackTargetsTheTaggedGuestPage) { + // A physical page in main RAM, chosen to equal one of MapAt's candidates so + // the host page of the same number can be held open as a negative control. + constexpr u32 kTargetPage = 0x00129000; + constexpr u32 kTarget = kTargetPage + kSetIndex * 64; + u32 page = 0; void* p = MapAt(&page); if (!p) GTEST_SKIP() << "could not map a page at any candidate host address"; + ASSERT_EQ(page, kTargetPage) << "the control page moved; re-point kTargetPage"; std::memset(p, 0xEE, 0x1000); - - const u32* target = reinterpret_cast( + const u32* host = reinterpret_cast( static_cast(page) + kSetIndex * 64); { EeRecTestHarness h; resetCache(); + memWrite32(kTarget, 0u); + memWrite32(kTarget + 4, 0u); writeCache32(kProbeLine, 0x5A5A0009u); writeCache32(kProbeLine + 4, 0xDEADBEEFu); - cpuRegs.CP0.n.TagLo = page | kFlagDirty | kFlagValid; + cpuRegs.CP0.n.TagLo = kTargetPage | kFlagDirty | kFlagValid; RunCacheOp(0x12, kProbeLine); // DXSTG RunCacheOp(0x14, kProbeLine); // DXWBIN - // 64 bytes of guest cache line, written to a host address the guest - // picked. On hardware this is a store to guest physical memory. - EXPECT_EQ(target[0], 0x5A5A0009u) - << "PCSX2 no longer redirects the write; record the new behaviour"; - EXPECT_EQ(target[1], 0xDEADBEEFu); + // 64 bytes of guest cache line, at the guest physical page the tag names. + EXPECT_EQ(memRead32(kTarget), 0x5A5A0009u); + EXPECT_EQ(memRead32(kTarget + 4), 0xDEADBEEFu); + EXPECT_EQ(host[0], 0xEEEEEEEEu) << "the write-back still reaches a host address"; } - // Never filled: bit 11 is clear and writeBackIfNeeded declines. - std::memset(p, 0xEE, 0x1000); + // Never filled, and filled-then-invalidated. Both used to be declined + // because bit 11 was clear; both now write back, carrying the cleared line. + for (const bool invalidate_first : {false, true}) { + SCOPED_TRACE(invalidate_first ? "filled then invalidated" : "never filled"); + std::memset(p, 0xEE, 0x1000); EeRecTestHarness h; resetCache(); - RunCacheOp(0x16, kProbeLine); // DXIN - cpuRegs.CP0.n.TagLo = page | kFlagDirty | kFlagValid; + memWrite32(kTarget, 0xA5A5A5A5u); + if (invalidate_first) + { + writeCache32(kProbeLine, 0xABCD1234u); + RunCacheOp(0x14, kProbeLine); // DXWBIN: writes back, then clears + } + else + { + RunCacheOp(0x16, kProbeLine); // DXIN + } + memWrite32(kTarget, 0xA5A5A5A5u); + cpuRegs.CP0.n.TagLo = kTargetPage | kFlagDirty | kFlagValid; RunCacheOp(0x12, kProbeLine); RunCacheOp(0x14, kProbeLine); - EXPECT_EQ(target[0], 0xEEEEEEEEu) << "an unfilled line also redirects"; - } - - // Filled and then invalidated: clear() drops bit 11 too. - std::memset(p, 0xEE, 0x1000); - { - EeRecTestHarness h; - resetCache(); - writeCache32(kProbeLine, 0xABCD1234u); - RunCacheOp(0x14, kProbeLine); // DXWBIN: writes back, then clears - cpuRegs.CP0.n.TagLo = page | kFlagDirty | kFlagValid; - RunCacheOp(0x12, kProbeLine); - RunCacheOp(0x14, kProbeLine); - EXPECT_EQ(target[0], 0xEEEEEEEEu) << "an invalidated line also redirects"; + EXPECT_EQ(memRead32(kTarget), 0u) << "the cleared line did not write back"; + EXPECT_EQ(host[0], 0xEEEEEEEEu) << "the write-back still reaches a host address"; } munmap(p, 0x1000); } -// --------------------------------------------------------------------------- -// Tripwires. Each fails today and turns green when the defect above is fixed -// or the missing model appears. +// A DXSTG naming a page that does not resolve to plain guest memory leaves the +// line unbacked, so the write-back declines rather than dereferencing anything. +// 0x1FC00000-and-up is BIOS/unmapped territory at the top of the physical map. +TEST(EeCache2Console, DxstgOnAnUnresolvablePageDeclinesTheWriteBack) +{ + EeRecTestHarness h; + resetCache(); + writeCache32(kProbeLine, 0x5A5A0009u); + cpuRegs.CP0.n.TagLo = 0x1FFFF000u | kFlagDirty | kFlagValid; + RunCacheOp(0x12, kProbeLine); // DXSTG + RunCacheOp(0x14, kProbeLine); // DXWBIN -- must be a no-op, not a store + // Reaching here without a fault is the assertion; the flags still round-trip. + EXPECT_EQ(ReadTag(kProbeLine) & (kFlagValid | kFlagDirty), 0u); +} -TEST(EeCache2Console, DISABLED_DxstgDirtyStaysInsideGuestMemory) +// --------------------------------------------------------------------------- +// Tripwires. DxstgDirtyStaysInsideGuestMemory has graduated and holds; the rest +// fail today and turn green when the missing model appears. + +TEST(EeCache2Console, DxstgDirtyStaysInsideGuestMemory) { u32 page = 0; void* p = MapAt(&page); @@ -546,7 +556,8 @@ TEST(EeCache2Console, DISABLED_InstructionCacheGeometryIsModelled) TEST(EeCache2Console, DISABLED_AllEeCache2MatchesConsole) { // Graduation: everything above that is currently recorded as a divergence - // has to hold at once. + // has to hold at once. The DXSTG half already does; the instruction cache + // is what keeps this disabled. u32 page = 0; void* p = MapAt(&page); if (!p) diff --git a/tests/ctest/core/recompilers/ee_cache_console_conformance_tests.cpp b/tests/ctest/core/recompilers/ee_cache_console_conformance_tests.cpp index 6da7308b2a..351588ff97 100644 --- a/tests/ctest/core/recompilers/ee_cache_console_conformance_tests.cpp +++ b/tests/ctest/core/recompilers/ee_cache_console_conformance_tests.cpp @@ -489,11 +489,11 @@ TEST(EeCacheConsole, InvalidateKeepsThePhysicalTagOnConsole) // is the observation, which is unchanged; the name says 0x78 because that is // what the observation plus round 2 amounts to. // -// Its own bit 11 (isValidPFN) is NOT reachable this way, which was worth -// checking rather than assuming: setAddr() masks with ALL_BITS = 0xFFF and so -// preserves bit 11 across the store, and flags() masks it back off on the way -// out, so a guest DXSTG can neither set nor read it. It can still be left -// standing from a fill, and round 2 shows what that costs. +// Its own bit 11 (isValidPFN) is not reachable as a *bit*: DXSTG's flag mask is +// ALL_FLAGS = 0x7FF, setAddr() masks with ALL_BITS = 0xFFF and so preserves bit +// 11 across the store, and flags() masks it back off on the way out, so a guest +// can neither read it nor write it directly. It is now set from whether the tag +// DXSTG stored resolves to guest memory; round 2 has why. TEST(EeCacheConsole, DxstgWritableMaskIsPtagLoPlus0x78) { EeRecTestHarness h; diff --git a/tests/ctest/core/recompilers/ee_fpu_fcr_console_conformance_tests.cpp b/tests/ctest/core/recompilers/ee_fpu_fcr_console_conformance_tests.cpp index fb017f1b1e..28e4b510af 100644 --- a/tests/ctest/core/recompilers/ee_fpu_fcr_console_conformance_tests.cpp +++ b/tests/ctest/core/recompilers/ee_fpu_fcr_console_conformance_tests.cpp @@ -299,17 +299,9 @@ struct FlagSituation u32 fcr31; bool check_fd; u32 fd; - // The arm64 recompiler now raises O/SO (fpuEmitOverflowFlags in - // iFPU-arm64.cpp), so "Overflow" matches silicon on both engines. "NAN - // math" does not, and cannot at this clamp mode: it feeds ADD.S two raw - // exp-255 words, which the interpreter turns into ±fMax through fpuDouble - // (giving an Inf sum, hence O) while the default fast path hands them to - // the host untouched and gets a NaN, which is not an overflow. Turning on - // CHECK_FPU_EXTRA_OVERFLOW makes the JIT clamp its operands the same way - // and the row aligns — measured by - // EeFpuFcrConsoleConformance.NanMathOverflowIsAnOperandClampModeDifference. - // So this flag now means "diverges at the default clamp mode", not "the - // recompiler has no O/SO". + // Rows where the fast path does not reproduce the console word. "NAN + // math" diverges only at the default clamp mode; see + // DISABLED_NanMathOverflowIsAnOperandClampModeDifference. bool bad_jit; }; constexpr FlagSituation kFlagSituations[] = { @@ -443,7 +435,7 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_EnginesAgreeExceptOnTheOverflowFlags) << "the engines now AGREE. If the JIT started clamping its operands " "at the default clamp mode, drop this row from " "kFcrEngineDivergences."; - // Pin WHICH side is right, so a future "fix" that aligns them by + // Pin which side is right, so a future "fix" that aligns them by // removing the interpreter's flags fails here instead of passing. EXPECT_EQ(got[0], s.fcr31) << "[interp] must stay the console-matching side"; @@ -509,8 +501,7 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_NanMathOverflowIsAnOperandClampModeDif "fpuDouble does must reproduce the interpreter's O|SO exactly -- if " "this fails the divergence is NOT the operand-clamp axis and the " "attribution above is wrong"; - // The value is identical in all three legs, which is precisely why this - // divergence stayed invisible until FCR31 was read back. + // The value is identical in all three legs; only FCR31 moves. EXPECT_EQ(res[0], 0x7F7FFFFFu); EXPECT_EQ(res[1], res[0]); EXPECT_EQ(res[2], res[0]); @@ -519,36 +510,17 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_NanMathOverflowIsAnOperandClampModeDif // --------------------------------------------------------------------------- // The O/U class, engine against engine. // -// The two console rows above are one window into a whole family: FCR31's -// overflow and underflow maintenance, which pcsx2/FPU.cpp performs on EVERY -// arithmetic op and the recompilers performed on none. Three distinct -// behaviours live in the interpreter and all three are testable without a -// capture, because the interpreter is the reference side here: +// The console rows above are one window into a family: the FCR31 overflow and +// underflow maintenance pcsx2/FPU.cpp performs on every arithmetic op and the +// recompilers perform on none. It needs no capture, because the interpreter is +// the reference side; the three behaviours it implements are the three groups +// of kFamCases below. // -// 1. the ten ops that call checkOverflow(result, O|SO) and then -// checkUnderflow(result, U|SU) -- ADD/SUB/MUL, the A-forms ADDA/SUBA/ -// MULA, and the multiply-accumulates MADD/MSUB/MADDA/MSUBA. On an Inf -// result they set O|SO and RETURN, leaving U alone; otherwise they CLEAR -// O and then clear (or set) U. -// 2. ABS/NEG/MAX/MIN, which clearFPUFlags(O|U) and nothing else. -// 3. DIV/SQRT/RSQRT, which touch I and D but pass 0 to checkOverflow, so -// O and U must survive them untouched. These are the negative controls, -// and they are live ones: rows 1 and 2 in the same table prove the probe -// can see an FCR31 change at all, so "unchanged" here means preserved -// rather than unobserved. -// -// The clear in (1) and (2) is observable on its own -- preset O and U through -// ctc1 (both are in the writable mask) and run a non-overflowing op. That is -// why the pre-state below is 0x0100C001 rather than the bare fixed-ones word: -// it makes set, clear and preserve three distinguishable outcomes instead of -// two. -// -// The underflow half of (1) is deliberately NOT exercised here: FZ is set in -// every FP environment PCSX2 runs the EE under (DAZ+FTZ+ChopZero is the -// shipping default, and both ScopedFpEnv kinds this file uses keep FZ on), so -// no denormal result can reach checkUnderflow and U is only ever cleared. What -// happens with FZ off is the denormal-operand question -- a separate work -// item -- and is pinned by DISABLED_UnderflowFlagsNeedFzOff below. +// Only the overflow half is exercised. FZ is set in every FP environment PCSX2 +// runs the EE under -- DAZ+FTZ+ChopZero is the shipping default and both +// ScopedFpEnv kinds this file uses keep FZ on -- so no denormal result can +// reach checkUnderflow and U is only ever cleared. +// DISABLED_UnderflowFlagsNeedFzOff pins the FZ-off half. namespace { enum FamOp @@ -581,7 +553,7 @@ struct FamCase u32 want_fcr31; }; -// No row here overflows the intermediate PRODUCT of a multiply-accumulate: +// No row here overflows the intermediate product of a multiply-accumulate: // that corner is a deliberate default-clamp-mode divergence between the // engines (see recMADD_S_xmm in iFPU-arm64.cpp, pinned by // EeRecFpu.MaddSProductOverflowDefaultModeMatchesX86Jit), and pulling it in @@ -623,7 +595,9 @@ constexpr FamCase kFamCases[] = { {"MIN.S clears O|U", FA_MIN, 0, kOne, kTwo, kFcr31FixedOnes}, // (3) Negative controls -- the divide unit passes 0 to checkOverflow, so - // O and U must come out exactly as they went in. + // O and U must come out exactly as they went in. Groups (1) and (2) above + // move the same bits from the same pre-state, so "unchanged" here means + // preserved rather than unobserved. {"DIV.S preserves", FA_DIV, 0, kOne, kTwo, kOuPreset}, {"SQRT.S preserves", FA_SQRT, 0, 0, kFour, kOuPreset}, {"RSQRT.S preserves", FA_RSQRT, 0, kOne, kFour, kOuPreset}, @@ -700,14 +674,11 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_EnginesAgreeOnOverflowFlagsAcrossTheAr const u32 jit = RunFamCase(c, true, &res[1]); SCOPED_TRACE(::testing::Message() << c.what); - // The interpreter is the reference side: it is what matches the console - // on the two captured rows above, so pin it to the value FPU.cpp's - // checkOverflow/clearFPUFlags model says it must produce. + // Pin the interpreter to what FPU.cpp's checkOverflow/clearFPUFlags + // model says it must produce. EXPECT_EQ(interp, c.want_fcr31) << "[interp] no longer matches the " "checkOverflow model in FPU.cpp"; EXPECT_EQ(jit, interp) << "engines disagree on FCR31 O/U"; - // Only the flags are supposed to be under test -- if the arithmetic - // diverged too, the row is measuring the wrong thing. EXPECT_EQ(res[1], res[0]) << "engines disagree on the RESULT, so this " "row no longer isolates the flag write"; ++checked; @@ -715,13 +686,10 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_EnginesAgreeOnOverflowFlagsAcrossTheAr EXPECT_EQ(checked, kFamCaseCount); } -// Several flag writers in ONE block, which is where the recompiler's FCR31 -// block residency (GE-12) has to hold the whole model together: the arithmetic -// family now read-modify-writes the same allocator-resident FCR31 that C.cond -// writes the condition bit into, so a bad mask would either eat C or make SO -// non-sticky. Both orderings are checked because they exercise different -// halves: O has to come back down when a later op does not overflow, and it has -// to stay up when the last one does. Neither may disturb C. +// Several flag writers in one block, where the recompiler's FCR31 block +// residency (GE-12) has to hold the model together: the arithmetic family +// read-modify-writes the same allocator-resident FCR31 that C.cond writes the +// condition bit into, so a bad mask would either eat C or make SO non-sticky. // TRIPWIRE -- see the O/SO rejection note above. TEST(EeFpuFcrConsoleConformance, DISABLED_OverflowFlagsComposeAcrossOneBlock) { @@ -776,7 +744,7 @@ TEST(EeFpuFcrConsoleConformance, DISABLED_OverflowFlagsComposeAcrossOneBlock) // The underflow half of checkUnderflow(result, U|SU), which needs a denormal // result and therefore needs FZ off. DISABLED because it is the denormal- // operand work item, not this one: with FZ off the two engines also disagree -// on the VALUE (the interpreter flushes the denormal to signed zero inside +// on the value (the interpreter flushes the denormal to signed zero inside // checkUnderflow, the recompilers keep it), and pinning the flag without the // value would assert half a behaviour. Force-enable to see the current state. TEST(EeFpuFcrConsoleConformance, DISABLED_UnderflowFlagsNeedFzOff) diff --git a/tests/ctest/core/recompilers/ee_fpu_overflow_console_conformance_tests.cpp b/tests/ctest/core/recompilers/ee_fpu_overflow_console_conformance_tests.cpp index b398aaa828..61553fcb9c 100644 --- a/tests/ctest/core/recompilers/ee_fpu_overflow_console_conformance_tests.cpp +++ b/tests/ctest/core/recompilers/ee_fpu_overflow_console_conformance_tests.cpp @@ -24,14 +24,10 @@ // engine-vs-engine divergence is not. The console column is therefore carried // as data and asserted only by the DISABLED tripwire at the bottom. // -// SQRT.S is the one op that has left this compromise. Exponent 255 is an -// ordinary binade, so its operands never needed saturating at all: both engines -// now compute sqrt(|Ft|/4)*2 and match the console exactly, without widening to -// double and without changing any operand whose exponent field is <= 254. That -// is what moved rows 44 and 45 out of the value-only column below. The same -// argument is available to ABS.S (see the DISABLED EeFpuAbsNegClamp tripwire) -// but NOT to the arithmetic ops, whose results genuinely exceed what the host -// single can hold. +// SQRT.S has left this compromise: both engines scale instead of clamping and +// match the console, which is what moved rows 44 and 45 out of the value-only +// column below. ABS.S likewise (ee_fpu_absneg_clamp_tests.cpp). The arithmetic +// ops cannot follow: their results do exceed what a host single can hold. // // The measured console divergences, all shared by both engines and all // deliberate, for the record: @@ -235,11 +231,9 @@ TEST(EeFpuOverflowConsole, EnginesAgreeExceptOnTheDocumentedRows) } // --------------------------------------------------------------------------- -// ENABLED. Classifies the divergence list by measurement rather than by -// assertion in a comment: every listed row must close when the operand clamp -// is on. The else-branch is the liveness clause for any future entry that does -// NOT close -- a defect rather than the mode axis, which is what rows 44/45 -// were before SQRT gained its clamp. +// ENABLED. Every listed row must close when the operand clamp is on. The +// else-branch covers a future entry that does not close: that would be a +// defect rather than the mode axis. // --------------------------------------------------------------------------- TEST(EeFpuOverflowConsole, OperandClampHealsEveryDocumentedDivergence) { @@ -300,32 +294,20 @@ TEST(EeFpuOverflowConsole, DefaultClampModeSaturatesToFltMaxOnBothEngines) } // --------------------------------------------------------------------------- -// REGRESSION TEST for the defect this capture surfaced, and then for the fix. +// Both engines against the console on every SQRT row in the capture. // -// Round one: recSQRT_S_xmm was the one emitter in iFPU-arm64.cpp that never -// clamped its operand, so an exponent-255 Ft reached Fsqrt as a host +Inf and -// fpuClampResult flattened the result to 0x7F7FFFFF, while the interpreter's -// sqrt(fpuDouble(Ft)) landed two binades away at 0x5F7FFFFF. SQRT was given a -// clamp to match, and the engines agreed -- on 0x5F7FFFFF, which is not what -// the console returns either. Agreement is a weaker property than accuracy and -// that round bought it at the cost of accuracy. +// The capture surfaced this as a clamp defect: recSQRT_S_xmm was the one +// emitter in iFPU-arm64.cpp that never clamped its operand, so an exponent-255 +// Ft reached Fsqrt as a host +Inf and fpuClampResult flattened the result to +// 0x7F7FFFFF, while the interpreter's sqrt(fpuDouble(Ft)) gave 0x5F7FFFFF. +// Clamping SQRT too made both engines say 0x5F7FFFFF, which is not the console +// value either. They scale instead now -- see SQRT_S (pcsx2/FPU.cpp) and +// recSQRT_S_xmm (pcsx2/arm64/iFPU-arm64.cpp). // -// Round two, what this now pins: neither engine clamps this operand. Both -// compute sqrt(|Ft|/4)*2, which keeps operand and result inside the ordinary -// single range without widening to double -- exponent 255 is an ordinary binade -// on the EE, so there was never anything here to saturate. See SQRT_S -// (pcsx2/FPU.cpp) and recSQRT_S_xmm (pcsx2/arm64/iFPU-arm64.cpp). -// -// Before the fix this failed on rows 44 and 45 (console 5fb504f3 / 5f800000, -// both engines 5f7fffff) and passed on row 46, whose Ft has exponent field 254 -// and so never reached the clamp. Row 46 is therefore the negative control for -// the scaling branch's condition: if the branch were simply always taken, or -// the condition inverted, row 46 would move. -// -// The console value is asserted, not merely engine agreement -- agreement can -// always be reached by degrading whichever engine is nearer silicon, which is -// how round one went wrong. Both clamp modes are checked because the old clamp -// was gated on CHECK_FPU_OVERFLOW and the replacement deliberately is not. +// Rows 44 and 45 failed until then (console 5fb504f3 / 5f800000, both engines +// 5f7fffff); row 46's Ft has exponent field 254, never reached the clamp, and +// passed throughout. Both clamp modes are checked because the clamp this +// replaced was gated on CHECK_FPU_OVERFLOW. // --------------------------------------------------------------------------- TEST(EeFpuOverflowConsole, SqrtMatchesConsoleOnEveryCapturedOperand) { @@ -374,28 +356,19 @@ TEST(EeFpuOverflowConsole, SqrtMatchesConsoleOnEveryCapturedOperand) } // --------------------------------------------------------------------------- -// The same property as above, over the WHOLE exponent-255 class rather than the -// three patterns the capture happens to contain. +// The same property over the whole exponent-255 class rather than the three +// patterns the capture happens to contain. As host bit patterns those words +// are infinities, quiet NaNs and signalling NaNs; to the EE they are all large +// finite floats, so the pool carries every shape. The signalling ones are the +// half a host-NaN-aware implementation gets wrong -- see recSQRT_S_xmm +// (iFPU-arm64.cpp) for why the clamp they replaced had to be an integer Umin +// rather than an Fminnm. // -// This exists because the class splits on an axis the capture cannot see. As -// HOST bit patterns, exponent-255 words are infinities, quiet NaNs and -// signalling NaNs; to the EE they are all just large finite floats. The old -// arm64 clamp had to be an integer Umin rather than an Fminnm precisely because -// of that split -- FMINNM only prefers the number against a QUIET NaN, while a -// signalling operand comes back merely quieted, so half the mantissa space -// (4194303 of the 8388608 positive patterns) would have passed through a clamp -// that was supposed to catch it. Testing the exponent FIELD, as both engines -// now do, never asks the host what kind of NaN it thinks it is holding, so the -// whole taxonomy should be irrelevant -- and this is what proves it. -// -// Expected values are correctly-rounded square roots computed by exact integer -// arithmetic (math.isqrt on the significand, round-to-nearest-even, which is -// the divide unit's mode), NOT by a host float, so they cannot inherit the -// behaviour under test. That model was validated against silicon on the six -// operands the capture does witness -- marked `true` below -- and agreed on all -// six including the exponent-254 control. The three unwitnessed rows are -// therefore computed expectations, not measurements; they are here for class -// coverage and are flagged as such. +// The wanted values are correctly-rounded square roots computed by exact +// integer arithmetic (math.isqrt on the significand, round-to-nearest-even, +// the divide unit's mode) rather than by a host float. The six marked `true` +// were read off silicon; the other three are computed only, for class +// coverage. // --------------------------------------------------------------------------- TEST(EeFpuOverflowConsole, SqrtMatchesConsoleOnEveryExponent255Operand) { @@ -421,8 +394,7 @@ TEST(EeFpuOverflowConsole, SqrtMatchesConsoleOnEveryExponent255Operand) {0x7FC00000u, 0x5F9CC471u, 0x5F9CC470u, true, "exp255 mant 0x400000 (host +qNaN, smallest)"}, {0x7FFFFFFFu, 0x5FB504F3u, 0x5FB504F2u, true, "+EEMAX (host +qNaN, largest)"}, {0xFFFFFFFFu, 0x5FB504F3u, 0x5FB504F2u, true, "-EEMAX (host -qNaN, largest)"}, - // CONTROL: exponent field 254, so the scaling branch must NOT fire. - // If it does, this row comes back one binade low. + // Control: exponent field 254, below the scaling branch. {0xFF7FFFFFu, 0x5F7FFFFFu, 0x5F7FFFFFu, true, "-FLT_MAX (exp 254 -- CONTROL)"}, }; @@ -471,7 +443,7 @@ TEST(EeFpuOverflowConsole, SqrtMatchesConsoleOnEveryExponent255Operand) // --------------------------------------------------------------------------- // TRIPWIRE for the later hardware-alignment stage. Both engines, every row, -// against the console. Expected to fail on 37 of 57 rows today for the three +// against the console. Expected to fail on 35 of 57 rows today for the three // documented reasons at the top of this file. // --------------------------------------------------------------------------- TEST(EeFpuOverflowConsole, DISABLED_AllRowsMatchConsole) diff --git a/tests/ctest/core/recompilers/ee_rec_fpu_guardbit_tests.cpp b/tests/ctest/core/recompilers/ee_rec_fpu_guardbit_tests.cpp index 9af4cdbbfc..5eb7644c09 100644 --- a/tests/ctest/core/recompilers/ee_rec_fpu_guardbit_tests.cpp +++ b/tests/ctest/core/recompilers/ee_rec_fpu_guardbit_tests.cpp @@ -13,8 +13,8 @@ // CHECK_FPU_GUARDED / fpuGuardedAddSub option, which is ON by default (games // like True Crime NYC and Jak 3 misrender without it, and per-game flagging // proved impractical) but can be turned off globally for EE-heavy titles that -// don't need it. These tests run under the default (ON); DisableEmitsPlainOp -// at the bottom pins the opt-out path. +// don't need it. These tests run under the default (on); +// DisableEmitsPlainOpMatchingInterp at the bottom pins the opt-out path. // // THESE ARE JIT-ONLY TESTS. The shared interpreter's ADD_S/SUB_S (FPU.cpp) is a // plain host float + float (fpuDouble() returns float and does no masking), diff --git a/tests/ctest/core/recompilers/ee_rec_fpu_rsqrt_tests.cpp b/tests/ctest/core/recompilers/ee_rec_fpu_rsqrt_tests.cpp index 755731cd18..268419728e 100644 --- a/tests/ctest/core/recompilers/ee_rec_fpu_rsqrt_tests.cpp +++ b/tests/ctest/core/recompilers/ee_rec_fpu_rsqrt_tests.cpp @@ -15,13 +15,10 @@ // - Negative nonzero divisor: interp rounds sqrt(|Ft|) into a float temp // before dividing, so its divide is single-precision and matches native // bit-for-bit. I|SI raised. -// - Positive nonzero divisor: interp computes Fs / sqrt(Ft) in DOUBLE (bare -// libm sqrt returns double, so the divide promotes) then rounds to float, -// while native/x86/hardware stay single-precision. The results disagree by -// exactly <=1 ULP on inexact quotients (~10% of random positive-Ft inputs). -// Native reproduces the single-precision EE FPU / x86 result, so those -// values are asserted JIT-only; the interp double-rounding divergence is -// pinned as a DISABLED tripwire (RsqrtSPositivePathDivergesFromInterp). +// - Positive nonzero divisor: same, since RSQRT_S() (pcsx2/FPU.cpp) stopped +// dividing by the double libm sqrt returns. At the production rounding +// mode the two still part by one ULP, pinned by +// DISABLED_RsqrtSPositivePathDivergesInProductionFpEnv. #include "harness/EeRecTestHarness.h" @@ -81,7 +78,7 @@ u32 fuzzOperand(Lcg& r) // --------------------------------------------------------------------------- // Differential fuzzer over the exactly-matching domain: zero and negative // divisors (interp and native both stay single-precision there). Any Fs. -// Run()'s auto-diff checks the result value; the sticky flags are diffed too. +// The result value and the sticky flags are both diffed. // --------------------------------------------------------------------------- // These three ran green only because the old harness rounded to nearest. Under // the production environment the interpreter's double-rounded RSQRT lands one diff --git a/tests/ctest/core/recompilers/ee_rec_fpu_tests.cpp b/tests/ctest/core/recompilers/ee_rec_fpu_tests.cpp index 7a5bdee4ee..73364edf3d 100644 --- a/tests/ctest/core/recompilers/ee_rec_fpu_tests.cpp +++ b/tests/ctest/core/recompilers/ee_rec_fpu_tests.cpp @@ -429,8 +429,8 @@ TEST(EeRecFpu, CvtWNegativeNanSaturatesToIntMin) // ----- SQRT.S sticky-flag handling ----------------------------------- // -// PS2 SQRT.S clears the I|D cause flags unconditionally and sets I|SI when -// Ft is negative non-zero (interp SQRT_S, FPU.cpp; CHECK_FPU_EXTRA_FLAGS is +// PS2 SQRT.S clears the I|D cause flags unconditionally and sets I|SI whenever +// Ft's sign bit is set (interp SQRT_S, FPU.cpp; CHECK_FPU_EXTRA_FLAGS is // hardcoded on). Run()'s auto-diff does not gate on fprc[31], so assert the // flag bits directly on both snapshots (they must agree — the JIT matches // interp). Result value (sqrt(|Ft|)) is unchanged and stays in the auto-diff. @@ -478,13 +478,11 @@ TEST(EeRecFpu, SqrtSPositiveClearsStaleIDFlags) // Found by a randomized SQRT.S differential, which is why a case this small // went unnoticed: every hand-written SQRT.S test above uses +/-4.0. // -// The FLAG half of this used to be asserted here as "no I|SI, the zero path is -// not the negative path", on nothing but the two engines agreeing. That was -// wrong: ps2autotests' sqrt.expected prints results only, never FCR31, so it -// could not have said either way. A first-party capture that does record FCR31 -// (fpmatrix cases 226/227) shows the console raising I|SI here. It now lives -// in SqrtSInvalidFlagFollowsTheSignBitAlone below, which owns the whole rule; -// this test keeps the value and asserts only that D stays clear. +// The flag half used to be asserted here as "no I|SI", on nothing but the two +// engines agreeing; ps2autotests' sqrt.expected prints results only, never +// FCR31. The console does raise I|SI on -0, and the rule now lives in +// SqrtSInvalidFlagFollowsTheSignBitAlone below. This test keeps the value and +// asserts only that D stays clear. TEST(EeRecFpu, SqrtSOfNegativeZeroIsPositiveZero) { EeRecTestHarness h; @@ -500,16 +498,12 @@ TEST(EeRecFpu, SqrtSOfNegativeZeroIsPositiveZero) } // ----- SQRT.S's I flag keys off the sign bit, not the exponent -------- -// The rule, from a first-party PS2 capture that records FCR31 alongside the -// result (~/.claude/projects/.../captures/fpmatrix, corpus 62dd6882, the 38 -// SQRT.S cases): -// -// SQRT.S raises I|SI whenever Ft's SIGN BIT is set. Full stop. The -// exponent field plays no part. -// -// So -0.0 and the negative denormals -- which are flushed to -0 before the op -// and produce +0, a perfectly ordinary result -- still raise invalid-operation. -// Every capture row agrees; the ten below are the sign x exponent matrix: +// From a first-party PS2 capture that records FCR31 alongside the result +// (corpus 62dd6882, the 38 SQRT.S cases): SQRT.S raises I|SI whenever Ft's +// sign bit is set, whatever the +// exponent. So -0.0 and the negative denormals, flushed to -0 before the op +// and producing an ordinary +0, still raise invalid-operation. Every capture +// row agrees; the ten below are the sign x exponent matrix: // // case 226 sqrt 00000000 -> 00000000/01000001 +0 // case 227 sqrt 80000000 -> 00000000/01020041 -0 <- I|SI @@ -523,23 +517,9 @@ TEST(EeRecFpu, SqrtSOfNegativeZeroIsPositiveZero) // case 241 sqrt 80800000 -> 20000000/01020041 -MIN_NORMAL <- I|SI // // 0x01020041 is I|SI plus FCR31's two always-set bits (0x01000001); 0x01000001 -// is those two bits alone. The positive rows are CONTROLS and they are why this -// is a matrix rather than two rows: the fix is a deletion (a gate on the -// exponent field comes out of the flag test), and a deletion that went too far -// -- dropping the sign test as well -- would raise I on every SQRT.S. Nothing -// but the positive rows would notice. -// -// Those controls are live, not decorative, and that was measured rather than -// assumed: with the sign test also deleted from both engines, all six positive -// rows fail on both and the four negative rows still pass. The two -0/-denormal -// rows were likewise seen failing before the fix (0x01000001 against -// 0x01020041, both engines) and passing after. -// -// Both engines used to gate the flag on `exp != 0 && sign`, so both missed -// exactly the two rows where the sign is set and the exponent is zero. x86's -// recSQRT_S_xmm (iFPU.cpp) has always tested MOVMSKPS's sign bit alone, as has -// the FULL-mode DOUBLE path in iFPUd-arm64.cpp -- upstream's x86 JIT is the -// only column in the capture that got these two rows right. +// is those two bits alone. The positive rows are controls: a deletion that took +// the sign test out along with the exponent gate would raise I on every SQRT.S, +// and nothing but those rows would catch it. namespace { struct SqrtFlagRow { u32 ft, result, fcr31; const char* what; }; constexpr SqrtFlagRow kSqrtFlagRows[] = { @@ -912,13 +892,11 @@ TEST(EeRecFpu, MsubaSSubtractsProductFromAccumulator) // ----- MADDA/MSUBA must NOT clamp the intermediate product ----------- // -// Interp MADD_S/MSUB_S route the fs*ft product through fpuDouble (clamping it -// to +-fMax) before the accumulate, but MADDA_S/MSUBA_S (FPU.cpp) add the raw -// product directly and overflow-check only the final ACC. Clamping the product -// in all four ops diverges when fs*ft overflows: an overflowing product -// clamped to +fMax cancels against an opposite-signed ACC (-> 0) instead of -// overflowing the accumulate (-> +-fMax). Run()'s auto-diff compares ACC, and -// these cases are chosen so JIT and interp agree only without the product clamp. +// A product clamped to +-fMax cancels against an opposite-signed ACC (-> 0) +// instead of overflowing the accumulate (-> +-fMax), and these cases are chosen +// so that only the unclamped behaviour survives. Neither engine clamps it any +// more; the interpreter rounds the product to an EE single and stops there if +// it overflowed (eeMulAccumulate). // The next four all turn on the raw product reaching the accumulator as Inf, // which only happens at round-to-nearest: round-toward-zero, the production // mode, rounds an overflowing product to +/-FLT_MAX instead. Under that mode diff --git a/tests/ctest/core/recompilers/ee_rec_mmi_tests.cpp b/tests/ctest/core/recompilers/ee_rec_mmi_tests.cpp index ff9d2b8fdf..dfad9b39e5 100644 --- a/tests/ctest/core/recompilers/ee_rec_mmi_tests.cpp +++ b/tests/ctest/core/recompilers/ee_rec_mmi_tests.cpp @@ -277,7 +277,8 @@ TEST(EeRecMmi, PexcwAliasedRdEqualsRt) TEST(EeRecMmi, QfsrvAdjacentSourceContiguous) { // Rs == Rt+1 (a1 == a0+1) hits the contiguous-memory path that reads the - // two source registers directly and skips the temp-buffer stores. sa = 4 bytes. + // two source registers directly instead of going through the general + // path's TBL. sa = 4 bytes. EeRecTestHarness h; h.SetMmiPair(reg::a0, 0x1122334455667788ull, 0x99AABBCCDDEEFF00ull); // Rt h.SetMmiPair(reg::a1, 0xAABBCCDD11223344ull, 0x5566778899AABBCCull); // Rs @@ -288,7 +289,7 @@ TEST(EeRecMmi, QfsrvAdjacentSourceContiguous) TEST(EeRecMmi, QfsrvNonAdjacentSource) { - // Rs != Rt+1 (a2 != a0+1) takes the temp-buffer path. Same Rt/Rs values, + // Rs != Rt+1 (a2 != a0+1) takes the general TBL path. Same Rt/Rs values, // sa = 7 bytes. EeRecTestHarness h; h.SetMmiPair(reg::a0, 0x1122334455667788ull, 0x99AABBCCDDEEFF00ull); // Rt