From 5bb4d089dd13d7f33225c77d95e9547dff3057df Mon Sep 17 00:00:00 2001 From: Ewan Crawford Date: Tue, 18 Aug 2020 08:00:14 +0100 Subject: [PATCH] Specify GCC flag `-frounding-math` on x86 (#873) * Specify GCC flag `-frounding-math` on x86 We have been seeing fails in `test_conversions` on x86_64 when converting to the following integer types from `cl_double` with a non-default rounding mode: ``` char_rtn_double char_rtp_double int_rtn_double int_rtp_double long_rtn_double long_rtp_double long_sat_rtn_double long_sat_rtp_double short_rtn_double short_rtp_double uchar_rtp_double uint_rtp_double ulong_rtp_double ushort_rtp_double ``` After investigation it was discovered that `rint()` was incorrectly rounding `cl_double` inputs despite the rounding mode being correctly set using `fesetround()` beforehand. E.g For 'char_rtn_double' `-3.5` was getting rounding to `-3.0` rather than `-4.0`. This is a gcc issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92164 when using the builtin `rint()` implementation, rather than `std::rint()`, that only presents itself when compiling a Release build with `-O2` rather than Debug. Adding the compiler flag `-fno-builtin-rint` to the CMake works around this problem, however based on the discussion in the gcc ticket using `-frounding-math` appears to be a more comprehensive fix. As `-frounding-math` tells gcc that the code will be modifying the rounding mode, removing the assumption that the same rounding mode is in effect everywhere. * Set FENV_ACCESS ON in rounding_mode.h Inform compilers which are aware of the `FENV_ACCESS` pragma that TUs which include `harness/rounding_mode.h` may manipulate the floating point environment. * Remove FENV_ACCESS pragma from test_conversions.cpp This pragma is now set in the included header `test_common/harness/rounding_mode.h` Co-authored-by: Kenneth Benzie (Benie) --- CMakeLists.txt | 2 ++ test_common/harness/rounding_mode.h | 2 ++ test_conformance/conversions/test_conversions.cpp | 2 -- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 33296910..ce71604c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -166,6 +166,8 @@ if(CMAKE_COMPILER_IS_GNUCC OR "${CMAKE_CXX_COMPILER_ID}" MATCHES "(Apple)?Clang" STREQUAL "x86") set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -msse -msse2 -mfpmath=sse") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -msse -msse2 -mfpmath=sse") + + add_cxx_flag_if_supported(-frounding-math) endif() else() set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /D__SSE__") diff --git a/test_common/harness/rounding_mode.h b/test_common/harness/rounding_mode.h index 8c0e8a13..abbb5afe 100644 --- a/test_common/harness/rounding_mode.h +++ b/test_common/harness/rounding_mode.h @@ -16,6 +16,8 @@ #ifndef __ROUNDING_MODE_H__ #define __ROUNDING_MODE_H__ +#pragma STDC FENV_ACCESS ON + #include "compat.h" #if (defined(_WIN32) && defined (_MSC_VER)) diff --git a/test_conformance/conversions/test_conversions.cpp b/test_conformance/conversions/test_conversions.cpp index b7280b4f..db5491fd 100644 --- a/test_conformance/conversions/test_conversions.cpp +++ b/test_conformance/conversions/test_conversions.cpp @@ -50,8 +50,6 @@ #include "Sleep.h" #include "basic_test_conversions.h" -#pragma STDC FENV_ACCESS ON - #if (defined(_WIN32) && defined (_MSC_VER)) // need for _controlfp_s and rouinding modes in RoundingMode #include "harness/testHarness.h"