Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Floating-point arithmetic on 386/x87 (32-bit) #785

Merged
merged 3 commits into from
Jun 20, 2022
Merged

Floating-point arithmetic on 386/x87 (32-bit) #785

merged 3 commits into from
Jun 20, 2022

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Apr 16, 2021

No description provided.

@codecov
Copy link

codecov bot commented Apr 16, 2021 •
edited
Loading

Codecov Report

Merging #785 (2527f21) into master (2527f21) will not change coverage.
The diff coverage is n/a.

❗ Current head 2527f21 differs from pull request most recent head 98bcb85. Consider uploading reports for the commit 98bcb85 to get more accurate results

@@           Coverage Diff           @@
##           master     #785   +/-   ##
=======================================
  Coverage   99.27%   99.27%           
=======================================
  Files          88       88           
  Lines       13184    13184           
=======================================
  Hits        13088    13088           
  Misses         96       96
Flag Coverage Δ
rust 98.47% <0.00%> (ø)
spectests 89.92% <0.00%> (ø)
unittests 99.22% <0.00%> (ø)
unittests-32 99.31% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@chfast chfast force-pushed the fp_32bit branch 2 times, most recently from e5e6532 to 00f95ee Compare April 16, 2021 15:42
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 5, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
axic
axic reviewed May 12, 2022
View reviewed changes
test/testfloat/CMakeLists.txt
@@ -30,6 +30,11 @@ if(TESTFLOAT_GEN)
list(APPEND IGNORE_LIST ui64_to_f64/min)
endif()

if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND CMAKE_SIZEOF_VOID_P EQUAL 4 AND CMAKE_CXX_COMPILER_ID MATCHES GNU)
# TODO: GCC 32-bit i386 build produces -0 for input 0, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100119.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supposedly fixed in gcc 12.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still doesn't work:

138/183 Test #103: fizzy/testfloat/ui32_to_f64/min ......................***Failed    0.01 sec
FAILURE: 8000000000000000 <- 00000000
         0000000000000000 (expected)
FAILURE: 8000000000000000 <- 00000000
         0000000000000000 (expected)
FAILURE: 8000000000000000 <- 00000000
         0000000000000000 (expected)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is indeed fixed in GCC 12.1. Are you sure we are using GCC 12? Maybe there is some other bug lurking here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran it on the CI, which I believed had 12, but now on a second look it is 11.

@axic
Copy link
Member

axic commented May 23, 2022 •
edited
Loading

Rebased on top of #745 (this included the disable NaN checks commit), and then master.

@axic axic force-pushed the fp_32bit branch 6 times, most recently from 880d4af to aca5b5f Compare May 23, 2022 08:50
@axic axic marked this pull request as ready for review May 23, 2022 08:51
axic
axic reviewed May 23, 2022
View reviewed changes
circle.yml Outdated Show resolved Hide resolved
@axic axic requested a review from gumb0 May 23, 2022 08:56
@axic axic force-pushed the fp_32bit branch 3 times, most recently from a06cb2b to 49cf8d8 Compare May 23, 2022 09:21
gumb0
gumb0 approved these changes May 23, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented May 23, 2022

@chfast are you okay merging this?

@chfast
Copy link
Collaborator Author

chfast commented May 23, 2022

With optimized build in GCC 12 I have these failures:

The following tests FAILED:
        126 - fizzy/testfloat/ui32_to_f64/min (Failed)
        182 - fizzy/testfloat/f64_ceil/min (Failed)
        190 - fizzy/testfloat/f64_trunc/min (Failed)
        194 - fizzy/testfloat/f64_nearest/min (Failed)
        421 - fizzy/unittests/execute_floating_point_types/f64.  # TypeParam = double.ceil (Failed)
        423 - fizzy/unittests/execute_floating_point_types/f64.  # TypeParam = double.trunc (Failed)
        424 - fizzy/unittests/execute_floating_point_types/f64.  # TypeParam = double.nearest (Failed)

@axic
Copy link
Member

axic commented May 24, 2022

@chfast let's shelve this then for now

@chfast chfast force-pushed the fp_32bit branch 2 times, most recently from 8d8a178 to 2210c5b Compare June 2, 2022 19:27
axic
axic reviewed Jun 2, 2022
View reviewed changes
CMakeLists.txt
@@ -71,6 +71,12 @@ if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
endif()
endif()

if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND CMAKE_SIZEOF_VOID_P EQUAL 4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x86_64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's different on true 386, but I have not way of checking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure it should be i386 on any non-64-bit x86 (at some point they tried to retrofit ia32 though).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fixed if someone would use such system (I doubt this will ever happen). At this point I'm not sure what name CMake uses because this is not specified except they mention uname.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some search suggest that it may return the following: x86, i386, i686.

See https://boringssl.googlesource.com/boringssl/+/refs/heads/2272/CMakeLists.txt and http://www.autoscool-clermont.com/blum/wp-content/themes/matrix/inc/kirki/assets/js/vendor/codemirror/mode/cmake/index.html (which has set(X86_ALIASES x86 i386 i686 x86_64 amd64)).

axic
axic reviewed Jun 5, 2022
View reviewed changes
CMakeLists.txt
@@ -71,6 +71,12 @@ if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
endif()
endif()

if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND CMAKE_SIZEOF_VOID_P EQUAL 4)
Copy link
Member

@axic axic Jun 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about sorting it out with:

Suggested change
if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND CMAKE_SIZEOF_VOID_P EQUAL 4)
# This is currently is for checking 32-bit mode on x86_64.
# TODO: potentially include x86, i386, i686 here
if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND CMAKE_SIZEOF_VOID_P EQUAL 4)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just realised that basically this check here enables "32-bit mode support" as opposed to supporting x86. Maybe just documenting that too is good.

Copy link
Member

@axic axic Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chfast please add this and then we can merge

Also see #785 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the required changes yourself.

@axic axic force-pushed the fp_32bit branch 3 times, most recently from 1bebb1c to 8c510c9 Compare June 20, 2022 16:45
@axic axic changed the base branch from master to arm64 June 20, 2022 16:45
Base automatically changed from arm64 to master June 20, 2022 16:57
@axic axic merged commit 59737b8 into master Jun 20, 2022
@axic axic deleted the fp_32bit branch June 20, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic approved these changes

@gumb0 gumb0 gumb0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chfast @axic @gumb0