From 0125d9b0999834dc2de4e526fe9823722a00877a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 26 May 2018 18:53:19 -0400 Subject: [PATCH 1/3] Interpreter_FloatingPoint: Factor out common code from fctiw and fctiwz fctiwz functions in the same manner as fctiw, with the difference being that fctiwz always assumes the rounding mode being towards zero. Because of this, we can implement fctiwz in terms of fctiw's code, but modify it to accept a rounding mode, allowing us to preserve proper behavior for both instructions. We also move Helper_UpdateCR1 to a temporary home in Interpreter_FPUtils.h for the time being. It would be more desirable to move it to a new common header for all the helpers, so that even JITs can use them if they so wish, however, this and the following changes are intended to only touch the interpreter to keep changes minimal for fixing instruction behavior. JitCommon already duplicates the Helper_Mask function within JitBase.cpp/.h, and the ARM JIT includes the Interpreter header in order to call Helper_Carry. So a follow up is best suited here, as this touches two other CPU backends. --- .../Core/PowerPC/Interpreter/Interpreter.h | 1 - .../PowerPC/Interpreter/Interpreter_FPUtils.h | 5 + .../Interpreter/Interpreter_FloatingPoint.cpp | 206 ++++++++---------- 3 files changed, 92 insertions(+), 120 deletions(-) diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter.h b/Source/Core/Core/PowerPC/Interpreter/Interpreter.h index dd4c88d94c..b1ea851041 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter.h +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter.h @@ -288,7 +288,6 @@ private: // flag helper static void Helper_UpdateCR0(u32 value); - static void Helper_UpdateCR1(); // address helper static u32 Helper_Get_EA(const UGeckoInstruction inst); diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FPUtils.h b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FPUtils.h index 2d16cbea84..0c98d3a939 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FPUtils.h +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FPUtils.h @@ -52,6 +52,11 @@ inline void UpdateFPSCR() (FPSCR.ZX & FPSCR.ZE) | (FPSCR.XX & FPSCR.XE); } +inline void Helper_UpdateCR1() +{ + PowerPC::SetCRField(1, (FPSCR.FX << 3) | (FPSCR.FEX << 2) | (FPSCR.VX << 1) | FPSCR.OX); +} + inline double ForceSingle(double value) { // convert to float... diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp index de4f453654..f8f64ed37f 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp @@ -11,12 +11,93 @@ #include "Core/PowerPC/Interpreter/Interpreter_FPUtils.h" #include "Core/PowerPC/PowerPC.h" -// Extremely rare - actually, never seen. -// Star Wars : Rogue Leader spams that at some point :| -void Interpreter::Helper_UpdateCR1() +namespace { - PowerPC::SetCRField(1, (FPSCR.FX << 3) | (FPSCR.FEX << 2) | (FPSCR.VX << 1) | FPSCR.OX); +// Apply current rounding mode +enum class RoundingMode +{ + Nearest = 0b00, + TowardsZero = 0b01, + TowardsPositiveInfinity = 0b10, + TowardsNegativeInfinity = 0b11 +}; + +void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) +{ + const double b = rPS0(inst.FB); + u32 value; + + if (b > static_cast(0x7fffffff)) + { + value = 0x7fffffff; + SetFPException(FPSCR_VXCVI); + FPSCR.FI = 0; + FPSCR.FR = 0; + } + else if (b < -static_cast(0x80000000)) + { + value = 0x80000000; + SetFPException(FPSCR_VXCVI); + FPSCR.FI = 0; + FPSCR.FR = 0; + } + else + { + s32 i = 0; + switch (rounding_mode) + { + case RoundingMode::Nearest: + { + const double t = b + 0.5; + i = static_cast(t); + + if (t - i < 0 || (t - i == 0 && b > 0)) + { + i--; + } + break; + } + case RoundingMode::TowardsZero: + i = static_cast(b); + break; + case RoundingMode::TowardsPositiveInfinity: + i = static_cast(b); + if (b - i > 0) + { + i++; + } + break; + case RoundingMode::TowardsNegativeInfinity: + i = static_cast(b); + if (b - i < 0) + { + i--; + } + break; + } + value = static_cast(i); + const double di = i; + if (di == b) + { + FPSCR.FI = 0; + FPSCR.FR = 0; + } + else + { + SetFI(1); + FPSCR.FR = fabs(di) > fabs(b); + } + } + + // Based on HW tests + // FPRF is not affected + riPS0(inst.FD) = 0xfff8000000000000ull | value; + if (value == 0 && std::signbit(b)) + riPS0(inst.FD) |= 0x100000000ull; + if (inst.Rc) + Helper_UpdateCR1(); } +} // Anonymous namespace void Interpreter::Helper_FloatCompareOrdered(UGeckoInstruction inst, double fa, double fb) { @@ -103,127 +184,14 @@ void Interpreter::fcmpu(UGeckoInstruction inst) Helper_FloatCompareUnordered(inst, rPS0(inst.FA), rPS0(inst.FB)); } -// Apply current rounding mode void Interpreter::fctiwx(UGeckoInstruction inst) { - const double b = rPS0(inst.FB); - u32 value; - - if (b > (double)0x7fffffff) - { - value = 0x7fffffff; - SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else if (b < -(double)0x80000000) - { - value = 0x80000000; - SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else - { - s32 i = 0; - switch (FPSCR.RN) - { - case 0: // nearest - { - double t = b + 0.5; - i = (s32)t; - - if (t - i < 0 || (t - i == 0 && b > 0)) - { - i--; - } - break; - } - case 1: // zero - i = (s32)b; - break; - case 2: // +inf - i = (s32)b; - if (b - i > 0) - { - i++; - } - break; - case 3: // -inf - i = (s32)b; - if (b - i < 0) - { - i--; - } - break; - } - value = (u32)i; - double di = i; - if (di == b) - { - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else - { - SetFI(1); - FPSCR.FR = fabs(di) > fabs(b); - } - } - - // based on HW tests - // FPRF is not affected - riPS0(inst.FD) = 0xfff8000000000000ull | value; - if (value == 0 && std::signbit(b)) - riPS0(inst.FD) |= 0x100000000ull; - if (inst.Rc) - Helper_UpdateCR1(); + ConvertToInteger(inst, static_cast(FPSCR.RN)); } -// Always round toward zero void Interpreter::fctiwzx(UGeckoInstruction inst) { - const double b = rPS0(inst.FB); - u32 value; - - if (b > (double)0x7fffffff) - { - value = 0x7fffffff; - SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else if (b < -(double)0x80000000) - { - value = 0x80000000; - SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else - { - s32 i = (s32)b; - double di = i; - if (di == b) - { - FPSCR.FI = 0; - FPSCR.FR = 0; - } - else - { - SetFI(1); - FPSCR.FR = fabs(di) > fabs(b); - } - value = (u32)i; - } - - // based on HW tests - // FPRF is not affected - riPS0(inst.FD) = 0xfff8000000000000ull | value; - if (value == 0 && std::signbit(b)) - riPS0(inst.FD) |= 0x100000000ull; - if (inst.Rc) - Helper_UpdateCR1(); + ConvertToInteger(inst, RoundingMode::TowardsZero); } void Interpreter::fmrx(UGeckoInstruction inst) From 8c4aa133ca2aac4e45e7d03add5288925b11e849 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 26 May 2018 19:18:18 -0400 Subject: [PATCH 2/3] Interpreter_FloatingPoint: Handle NaN flag setting within fctiw and fctiwz If a NaN of any type is passed as the operand to either of these instructions, we shouldn't go down the regular code path, as we end up potentially setting the wrong flags. For example, we wouldn't set the FPSCR.VXCVI bit properly. We'd also set FPSCR.FI, when in actuality it should be unset. If an SNaN is passed as an operand, we also need to set the FPSCR.VXSNAN bit as well. The flag setting behavior for these can be found in Appendix C.4.2 in PowerPC Microprocessor Family: The Programming Environments Manual for 32 and 64-bit Microprocessors. --- .../Interpreter/Interpreter_FloatingPoint.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp index f8f64ed37f..aa245c8662 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp @@ -22,13 +22,27 @@ enum class RoundingMode TowardsNegativeInfinity = 0b11 }; +// Note that the convert to integer operation is defined +// in Appendix C.4.2 in PowerPC Microprocessor Family: +// The Programming Environments Manual for 32 and 64-bit Microprocessors void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) { const double b = rPS0(inst.FB); u32 value; - if (b > static_cast(0x7fffffff)) + if (std::isnan(b)) { + if (Common::IsSNAN(b)) + SetFPException(FPSCR_VXSNAN); + + value = 0x80000000; + SetFPException(FPSCR_VXCVI); + FPSCR.FI = 0; + FPSCR.FR = 0; + } + else if (b > static_cast(0x7fffffff)) + { + // Positive large operand or +inf value = 0x7fffffff; SetFPException(FPSCR_VXCVI); FPSCR.FI = 0; @@ -36,6 +50,7 @@ void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) } else if (b < -static_cast(0x80000000)) { + // Negative large operand or -inf value = 0x80000000; SetFPException(FPSCR_VXCVI); FPSCR.FI = 0; @@ -84,6 +99,7 @@ void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) } else { + // Also sets FPSCR[XX] SetFI(1); FPSCR.FR = fabs(di) > fabs(b); } From 78a934bb12bc5f209242445bb58073b08ff3aad7 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 26 May 2018 19:58:19 -0400 Subject: [PATCH 3/3] Interpreter_FloatingPoint: Handle cases when FPSCR.VE is set and exceptions occur in fctiw and fctiwz If invalid operation exceptions are enabled and an invalid operation occurs, then the destination value remains untouched. This fixes issues that may arise when using these two instructions where the destination gets steamrolled by an infinity or NaN value. --- .../Interpreter/Interpreter_FloatingPoint.cpp | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp index aa245c8662..e3d194ee4f 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_FloatingPoint.cpp @@ -29,6 +29,7 @@ void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) { const double b = rPS0(inst.FB); u32 value; + bool exception_occurred = false; if (std::isnan(b)) { @@ -37,24 +38,21 @@ void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) value = 0x80000000; SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; + exception_occurred = true; } else if (b > static_cast(0x7fffffff)) { // Positive large operand or +inf value = 0x7fffffff; SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; + exception_occurred = true; } else if (b < -static_cast(0x80000000)) { // Negative large operand or -inf value = 0x80000000; SetFPException(FPSCR_VXCVI); - FPSCR.FI = 0; - FPSCR.FR = 0; + exception_occurred = true; } else { @@ -105,11 +103,21 @@ void ConvertToInteger(UGeckoInstruction inst, RoundingMode rounding_mode) } } - // Based on HW tests - // FPRF is not affected - riPS0(inst.FD) = 0xfff8000000000000ull | value; - if (value == 0 && std::signbit(b)) - riPS0(inst.FD) |= 0x100000000ull; + if (exception_occurred) + { + FPSCR.FI = 0; + FPSCR.FR = 0; + } + + if (!exception_occurred || FPSCR.VE == 0) + { + // Based on HW tests + // FPRF is not affected + riPS0(inst.FD) = 0xfff8000000000000ull | value; + if (value == 0 && std::signbit(b)) + riPS0(inst.FD) |= 0x100000000ull; + } + if (inst.Rc) Helper_UpdateCR1(); }