Issue 577450043: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (Closed)
|
Created:
4 years, 12 months ago by thomasmorley651 Modified:
4 years, 11 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionInline assembler fallback for _FPU_SETCW() missing in MINGW libraries
Issue 4943
As Issue 4943 on Windows compilation was triggered by
missing _FPU_SETCW() in the MINGW libraries, an alternate call to
initiate the X87 FPU setup as an inline-assembler command is added
- for MINGW 32 Bit compilation only.
Patch Set 1 #
Total comments: 3
MessagesTotal messages: 10
Sign in to reply to this message.
The comment could be more helpful. Explaining what this code does is more important than explaining that it replaces something that no longer exists. I've found this documentation on the x87 FPU Control Word: http://home.agh.edu.pl/~amrozek/x87.pdf Section 8.1.4. It looks like the value 0x027f changes the Precision Control (PC) field from the default of "Double Extended Precision (64 bits)" (value 3) to "Double Precision (53 bits)" (value 2).
Sign in to reply to this message.
While this answer is specific, it could be clearer, IMO: Reviewing the Intel Manuals at https://software.intel.com/sites/default/files/managed/a4/60/253665-sdm-vol-1... Section 4.2.2. We can see that the 64 bit significand of Double Extended Precision refers to an 80-bit floating point representation, and the 53-bit significand of Double Precision refers to a 64-bit floating point representation. I think if we just leave the 64/53 bit numbers in the comment, the reader might think we are not using 64-bit floating-point representations.
Sign in to reply to this message.
On 2020/01/29 22:36:03, Carl wrote: > While this answer is specific, it could be clearer, IMO: > > Reviewing the Intel Manuals at > https://software.intel.com/sites/default/files/managed/a4/60/253665-sdm-vol-1... > Section 4.2.2. > > We can see that the 64 bit significand of Double Extended Precision refers to an > 80-bit floating point representation, and the 53-bit significand of Double > Precision refers to a 64-bit floating point representation. > > I think if we just leave the 64/53 bit numbers in the comment, the reader might > think we are not using 64-bit floating-point representations. Carl, I'm shepherding this patch for Arnold. I'll point him to the review-comments, can't say anything about it myself.
Sign in to reply to this message.
https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc File lily/main.cc (right): https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode174 lily/main.cc:174: /* x86 defaults to using 80-bit extended precision arithmetic. This can cause I missed this comment the first time around because Rietveld hid it. This explains the situation well. https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode210 lily/main.cc:210: "\n mov $0x027f, %eax" It would be very nice if this magic number and the one in line 186 were not separately defined.
Sign in to reply to this message.
https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc File lily/main.cc (right): https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode174 lily/main.cc:174: /* x86 defaults to using 80-bit extended precision arithmetic. This can cause On 2020/01/29 23:54:27, Dan Eble wrote: > I missed this comment the first time around because Rietveld hid it. This > explains the situation well. Well, this comment was not part of current patch
Sign in to reply to this message.
How about this patch?
Sorry, not tested.
If I understand correctly,
this patch solves not only Issue 4943 but also Issue 4975.
These issues do not only occur on Windows, but on all x86 platforms except
Linux.
`defined (__MINGW32__)` is not necessary.
For Linux-x86, it uses the original precision setting. i.e. _FPU_SETCW ().
For other x86 platforms, it uses Arnold's inline asm for setting precision.
```
diff --git a/lily/main.cc b/lily/main.cc
index 9145345fff..cba6856159 100644
--- a/lily/main.cc
+++ b/lily/main.cc
@@ -209,19 +209,40 @@ char const *LILYPOND_DATADIR = PACKAGE_DATADIR "/"
TOPLEVEL_VERSION;
unpredictable places. To get around this, we tell the x87 FPU to use only
double precision. Note that this is not needed for x86_64 because that uses
the SSE unit by default instead of the x87 FPU. */
-#if ((defined (__x86__) || defined (__i386__)) \
- && defined (HAVE_FPU_CONTROL_H) && (HAVE_FPU_CONTROL_H == 1))
+#if defined (__x86__) || defined (__i386__)
+// This environment is x86.
+// It is necessary that setting precision by setting x87 FPU control word.
+#if defined (HAVE_FPU_CONTROL_H) && (HAVE_FPU_CONTROL_H == 1)
#include <fpu_control.h>
+#endif
+
static void
configure_fpu ()
{
+#if defined (HAVE_FPU_CONTROL_H) && (HAVE_FPU_CONTROL_H == 1)
+ // This environment has fpu_control.h. (e.g. Linux glibc)
+ // We use _FPU_SETCW () to set x87 FPU control word.
fpu_control_t fpu_control = 0x027f;
_FPU_SETCW (fpu_control);
+#else
+ // This environment doesn't have fpu_control.h. (e.g. MinGW)
+ // We use inline asm to set x87 FPU control word.
+ asm(
+ " push %ebp"
+ "\n mov $0x027f, %eax"
+ "\n push %eax"
+ "\n mov %esp, %ebp"
+ "\n fldcw (%ebp)"
+ "\n pop %eax"
+ "\n pop %ebp"
+ );
+#endif
}
#else
-
+// This environment isn't x86. (e.g. x86_64)
+// It is unnecessary that setting precision.
static void
configure_fpu ()
{
--
```
Sign in to reply to this message.
On Jan 30, 2020, at 05:17, trueroad@gmail.com wrote: > > How about this patch? > Sorry, not tested. Bringing the _FPU_SETCW() branch and the asm() branch closer together is an improvement. (I don't have the resources to test this patch.)
Sign in to reply to this message.
Hello Arnold. Thank you for your patch. If I understand correctly, we only need check the definitions of `__x86__` and `__i386__` check. In x86_64 environment, neither `__x86__` nor `__i386__` are defined. ``` $ echo | x86_64-w64-mingw32-gcc -dM -E - | grep "__x86__" $ echo | x86_64-w64-mingw32-gcc -dM -E - | grep "__i386__" $ echo | i686-w64-mingw32-gcc -dM -E - | grep "__x86__" $ echo | i686-w64-mingw32-gcc -dM -E - | grep "__i386__" #define __i386__ 1 $ ``` Therefore, `defined (__code_model_32__)` is not necessary. If `__SSE2_MATH__` is defined, it only indicates that main.cc is compiled with SSE2 math enabled. Shared libraries such as libguile may still use x86 FPU. If the floating point calculation inside GUILE uses x86 FPU, we need to set the precision even if C++ uses SSE2. Therefore, `defined (__SSE2_MATH__)` is not necessary. Furthermore, if the floating-point operations of all modules use SSE2, setting the x86 FPU precision has no effect because it is not used. In other words, there is no problem even if the precision is set on a platform that does not need to set. Therefore, environment definitions such as `defined (__MINGW32__)` is not necessary.
Sign in to reply to this message.
On 2020/01/31 10:45:42, trueroad wrote: > Hello Arnold. > Thank you for your patch. > > If I understand correctly, we only need check the definitions of `__x86__` and > `__i386__` check. > > In x86_64 environment, neither `__x86__` nor `__i386__` are defined. > > ``` > $ echo | x86_64-w64-mingw32-gcc -dM -E - | grep "__x86__" > > $ echo | x86_64-w64-mingw32-gcc -dM -E - | grep "__i386__" > > $ echo | i686-w64-mingw32-gcc -dM -E - | grep "__x86__" > > $ echo | i686-w64-mingw32-gcc -dM -E - | grep "__i386__" > #define __i386__ 1 > > $ > ``` > > Therefore, `defined (__code_model_32__)` is not necessary. > > If `__SSE2_MATH__` is defined, it only indicates that main.cc is compiled with > SSE2 math enabled. > Shared libraries such as libguile may still use x86 FPU. > If the floating point calculation inside GUILE uses x86 FPU, we need to set the > precision even if C++ uses SSE2. > > Therefore, `defined (__SSE2_MATH__)` is not necessary. > > Furthermore, if the floating-point operations of all modules use SSE2, setting > the x86 FPU precision has no effect because it is not used. > In other words, there is no problem even if the precision is set on a platform > that does not need to set. > > Therefore, environment definitions such as `defined (__MINGW32__)` is not > necessary. Per accident I've done a new Rietveld issue while updating current patch witch Masamichi-san's suggestions (Hopefully without mistakes). Please look here: https://codereview.appspot.com/575600043/
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
