JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy #9458

JosJuice · 2021-01-20T15:19:33Z

Fixes https://bugs.dolphin-emu.org/issues/12388. Might also fix other games that have problems with float, paired, float loadstore, or paired loadstore instructions in JitArm64, but I haven't tested any.

Left to do:

I need to fix the ARM64 MSVC CMake build
It would be useful if someone could test that ARM64 MSVC builds don't horribly crash when you start a game
~~With this change, a white square shows up on the title screen of Sonic Colors~~ Implement accurate single/double conversion
Make accurate single/double conversion faster
Unit tests for single/double conversion
~~Sonic is falling through grind rails now?~~ fselx needs to handle RW clobbering flags
Get PR PPCAnalyst: Rework the store-safe logic #9486 merged

Source/Core/Core/PowerPC/JitArm64/JitArm64_RegCache.h

Source/Core/Core/PowerPC/JitArm64/JitArm64_BackPatch.cpp

shuffle2 · 2021-01-23T00:05:06Z

Source/Core/Common/ArmFPURoundMode.cpp

+      (2 << 22),  // -inf
+  };
+
+  const u64 base = default_fpcr & ~(0b111 << 22);


is it really OK to use a static const base value here? some reason not to just read the register at write-time?

I think I did it this way just because the x64 equivalent doesn't read the register at write-time – I don't see any reason why reading the register wouldn't work. What would the problem be with using a static const base value?

hypothetically something else could change the bits not being overridden here after process start, and then those settings would be lost. but idk if it's a problem.

Source/Core/Common/ArmFPURoundMode.cpp

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

Tilka · 2021-04-11T11:24:26Z

Source/Core/Core/PowerPC/JitArm64/JitArm64_FloatingPoint.cpp

+    // (if enabled in FPSCR) like almost any float operation does. We accomplish this by adding 0.0,
+    // which should be cheaper than FCVT 32 -> 64 followed by FCVT 64 -> 32.
+    m_float_emit.MOVI(8, EncodeRegToSingle(V0), 0);
+    m_float_emit.FADD(EncodeRegToSingle(VD), EncodeRegToSingle(VB), EncodeRegToSingle(V0));


This will turn -0.0 into 0.0. To avoid this, add -0.0 instead.

Thanks, I was not aware of that :)

Should be fixed now, assuming I've used MOVI correctly.

Exception if FPSCR.RMode is TowardsMinusInfinity, in which case you'd want to add +0.0 to prevent +0.0 from being converted to -0.0.

(Tests?)

Hmm... Maybe I should just go with the double FCVT then. Having to re-emit code when the rounding mode changes seems annoying, and branching based on the rounding mode is probably worse for the performance than double FCVT. This case hopefully won't be triggered often anyway.

Briefly grepping the codebase apparently we don't currently adjust FPCR based on guest fpscr, which is slightly surprising.

That's what the first commit of this PR fixes.

Okay, I've pushed a new approach (as the last commit of this PR this time). It simply adds an additional check to the FMOV path so that we only use it if the register is store-safe.

Source/Core/Core/PowerPC/JitArm64/JitArm64_FloatingPoint.cpp

Fixes https://bugs.dolphin-emu.org/issues/12388. Might also fix other games that have problems with float/paired instructions in JitArm64, but I haven't tested any.

This simplifies some of the following commits. It does require an extra register, but hey, we have 32 of them. Something I think would be nice to add to the register cache in the future is the ability to keep both the single and double version of a guest register in two different host registers when that is useful. That way, the extra register we write to here can be read by a later instruction, saving us from having to perform the same conversion again.

Preparation for following commits. This commit intentionally doesn't touch paired stores, since paired stores are supposed to flush to zero. (Consistent with Jit64.)

Needed because the next commit will make RW clobber flags.

Our old conversion approach became a lot more inaccurate when enabling flush-to-zero, to the point of obviously breaking games.

If we can prove that FCVT will provide a correct conversion, we can use FCVT. This makes the common case a bit faster and the less likely cases (unfortunately including zero, which FCVT actually can convert correctly) a bit slower.

I haven't observed this breaking any game, but it didn't match the behavior of the interpreter as far as I could tell from reading the code, in that denormals weren't being flushed.

JMC47 · 2021-04-25T14:21:59Z

This has been reviewed and has been sitting for another two weeks. This has a lot of important fixes for players on AArch64 devices and if a regression sneaks in, the developer is active to fix any minor regressions.

theofficialgman · 2021-05-20T20:33:58Z

posting this here just so that its documented in the correct place instead of here #9666 and can be referred to as necessary:

I tested both 14066 (the merge associated with this PR) and the previous version 14053 to determine there is significant performance regressions. The performance drop about 13% in the games I've tested (double dash, mario kart wii, and windwaker tested).

JMC47 · 2021-05-20T20:36:22Z

Thank you for documenting it here.

It should be known that accurate floating point math is needed for replays in double dash!! and Mario Kart Wii to sync. The bomb bounces/physics also rely on floating point math in Wind Waker. For the games to behave correctly, these changes are necessary.

This comment has been minimized.

Sign in to view

JosJuice force-pushed the arm-fpu-round branch 3 times, most recently from a3db9e9 to 29aeb17 Compare January 20, 2021 19:59

This comment has been minimized.

Sign in to view

JosJuice force-pushed the arm-fpu-round branch from 29aeb17 to d03946e Compare January 21, 2021 11:54

This comment has been minimized.

Sign in to view

JosJuice changed the title ~~Implement ArmFPURoundMode.cpp~~ JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy Jan 21, 2021

JosJuice force-pushed the arm-fpu-round branch from 19bf663 to 4cc598e Compare January 22, 2021 00:04

JosJuice marked this pull request as draft January 22, 2021 22:20

merryhime suggested changes Jan 22, 2021

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitArm64_RegCache.h Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/JitArm64/JitArm64_BackPatch.cpp Outdated Show resolved Hide resolved

shuffle2 reviewed Jan 23, 2021

View reviewed changes

Source/Core/Common/ArmFPURoundMode.cpp Outdated Show resolved Hide resolved

JosJuice force-pushed the arm-fpu-round branch 3 times, most recently from 634565b to 4d73b95 Compare January 25, 2021 21:43

degasus reviewed Jan 27, 2021

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp Outdated Show resolved Hide resolved

JosJuice force-pushed the arm-fpu-round branch 2 times, most recently from 521dca3 to ebc5743 Compare January 28, 2021 21:52

degasus reviewed Jan 29, 2021

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp Outdated Show resolved Hide resolved

degasus reviewed Jan 29, 2021

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp Outdated Show resolved Hide resolved

JosJuice force-pushed the arm-fpu-round branch 3 times, most recently from 8880fe9 to 1c84a4d Compare February 2, 2021 20:46

JosJuice mentioned this pull request Feb 2, 2021

PPCAnalyst: Rework the store-safe logic #9486

Merged

JosJuice force-pushed the arm-fpu-round branch 2 times, most recently from c376ef0 to 001522b Compare February 13, 2021 17:57

JosJuice marked this pull request as ready for review February 13, 2021 17:58

JosJuice force-pushed the arm-fpu-round branch 2 times, most recently from 4b98d18 to 0b00a21 Compare February 15, 2021 22:02

This comment has been minimized.

Sign in to view

JosJuice force-pushed the arm-fpu-round branch from 85625e7 to e4ea917 Compare April 11, 2021 11:14

Tilka reviewed Apr 11, 2021

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitArm64_FloatingPoint.cpp Outdated Show resolved Hide resolved

JosJuice force-pushed the arm-fpu-round branch 3 times, most recently from 49d9bcd to cb61adf Compare April 11, 2021 13:19

JosJuice force-pushed the arm-fpu-round branch from cb61adf to 4892c9b Compare April 19, 2021 22:54

JosJuice added 12 commits April 25, 2021 15:56

Implement ArmFPURoundMode.cpp

f96ee47

Fixes https://bugs.dolphin-emu.org/issues/12388. Might also fix other games that have problems with float/paired instructions in JitArm64, but I haven't tested any.

JitArm64: Factor out single/double conversion code to functions

949686b

Preparation for following commits. This commit intentionally doesn't touch paired stores, since paired stores are supposed to flush to zero. (Consistent with Jit64.)

JitArm64: Call RW before FCMPE in fselx

39eccf6

Needed because the next commit will make RW clobber flags.

JitArm64: Use accurate single/double conversions

6e0a587

Our old conversion approach became a lot more inaccurate when enabling flush-to-zero, to the point of obviously breaking games.

JitArm64: Optimize ConvertDoubleToSingle

28e4869

JitArm64: Optimize ConvertSingleToDouble, part 1

018e247

JitArm64: Optimize ConvertSingleToDouble, part 2

1d106ce

If we can prove that FCVT will provide a correct conversion, we can use FCVT. This makes the common case a bit faster and the less likely cases (unfortunately including zero, which FCVT actually can convert correctly) a bit slower.

JitArm64: Skip accurate single/double conversion if store-safe

2a9d887

JitArm64: Add unit tests for single/double conversion

9d6263f

JitArm64: Use ConvertSingleToDoubleLower in RW when faster

54451ac

JitArm64: Fix frspx with single precision source

69c14d6

I haven't observed this breaking any game, but it didn't match the behavior of the interpreter as far as I could tell from reading the code, in that denormals weren't being flushed.

JosJuice force-pushed the arm-fpu-round branch from 4892c9b to 69c14d6 Compare April 25, 2021 13:57

JMC47 merged commit 5da85f3 into dolphin-emu:master Apr 25, 2021
10 checks passed

JosJuice deleted the arm-fpu-round branch April 25, 2021 14:24

JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy #9458

JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy #9458

JosJuice commented Jan 20, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shuffle2 Jan 23, 2021

JosJuice Jan 23, 2021

shuffle2 Jan 23, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Tilka Apr 11, 2021 •

edited

JosJuice Apr 11, 2021

JosJuice Apr 11, 2021

merryhime Apr 11, 2021 •

edited

JosJuice Apr 11, 2021 •

edited

merryhime Apr 11, 2021

JosJuice Apr 11, 2021

JosJuice Apr 11, 2021

JMC47 commented Apr 25, 2021

theofficialgman commented May 20, 2021 •

edited

JMC47 commented May 20, 2021

JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy #9458

JitArm64: Set flush-to-zero/rounding mode and improve float/double conversion accuracy #9458

Conversation

JosJuice commented Jan 20, 2021 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Tilka Apr 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merryhime Apr 11, 2021 • edited

Choose a reason for hiding this comment

JosJuice Apr 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMC47 commented Apr 25, 2021

theofficialgman commented May 20, 2021 • edited

JMC47 commented May 20, 2021

JosJuice commented Jan 20, 2021 •

edited

Tilka Apr 11, 2021 •

edited

merryhime Apr 11, 2021 •

edited

JosJuice Apr 11, 2021 •

edited

theofficialgman commented May 20, 2021 •

edited