Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1703)

Issue 577450043: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by thomasmorley651
Modified:
4 years, 1 month ago
Reviewers:
Dan Eble, dan, carl.d.sorensen, trueroad
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Inline 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -0 lines) Patch
M lily/main.cc View 1 chunk +24 lines, -0 lines 3 comments Download

Messages

Total messages: 10
thomasmorley651
4 years, 2 months ago (2020-01-29 21:56:38 UTC) #1
Dan Eble
The comment could be more helpful. Explaining what this code does is more important than ...
4 years, 2 months ago (2020-01-29 22:23:41 UTC) #2
Carl
While this answer is specific, it could be clearer, IMO: Reviewing the Intel Manuals at ...
4 years, 2 months ago (2020-01-29 22:36:03 UTC) #3
thomasmorley651
On 2020/01/29 22:36:03, Carl wrote: > While this answer is specific, it could be clearer, ...
4 years, 2 months ago (2020-01-29 22:52:18 UTC) #4
Dan Eble
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. ...
4 years, 2 months ago (2020-01-29 23:54:27 UTC) #5
thomasmorley651
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. ...
4 years, 2 months ago (2020-01-30 00:42:56 UTC) #6
trueroad
How about this patch? Sorry, not tested. If I understand correctly, this patch solves not ...
4 years, 2 months ago (2020-01-30 10:17:14 UTC) #7
dan_faithful.be
On Jan 30, 2020, at 05:17, trueroad@gmail.com wrote: > > How about this patch? > ...
4 years, 1 month ago (2020-01-30 14:51:06 UTC) #8
trueroad
Hello Arnold. Thank you for your patch. If I understand correctly, we only need check ...
4 years, 1 month ago (2020-01-31 10:45:42 UTC) #9
thomasmorley651
4 years, 1 month ago (2020-02-01 00:39:41 UTC) #10
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b