From 6965049700fa367d0cfe6fba211bac0ff53263ba Mon Sep 17 00:00:00 2001 From: Jordan Henderson Date: Fri, 1 Mar 2024 17:20:49 -0600 Subject: Fixed some _Float16 support issues Fixed some conversion issues with Clang due to problematic undefined behavior when casting a negative floating-point value to an integer Fixed a bug in the library's software integer to floating-point conversion function where a user's conversion exception function returning H5T_CONV_UNHANDLED in the case of overflows would result in incorrect data after conversion Added configure checks for functions and macros related to _Float16 usage since some compilers expose the datatype but not the functions or macros Fixed a dt_arith test failure when H5_WANT_DCONV_EXCEPTION isn't defined Fixed a few warnings from not explicitly casting some _Float16 variables upwards --- config/cmake/ConfigureChecks.cmake | 25 +++++++++++++- config/cmake/H5pubconf.h.in | 3 ++ configure.ac | 53 +++++++++++++++++++++++++---- src/H5Tconv.c | 35 ++++++++++++++++--- src/H5private.h | 30 ++++++++++++----- test/dt_arith.c | 69 +++++++++++++++++++++++++++++--------- test/ntypes.c | 5 +-- tools/lib/h5diff_array.c | 2 +- 8 files changed, 184 insertions(+), 38 deletions(-) diff --git a/config/cmake/ConfigureChecks.cmake b/config/cmake/ConfigureChecks.cmake index 9ccb7a4..14e8b75 100644 --- a/config/cmake/ConfigureChecks.cmake +++ b/config/cmake/ConfigureChecks.cmake @@ -367,7 +367,30 @@ HDF_CHECK_TYPE_SIZE (time_t ${HDF_PREFIX}_SIZEOF_TIME_T) #----------------------------------------------------------------------------- HDF_CHECK_TYPE_SIZE (_Float16 ${HDF_PREFIX}_SIZEOF__FLOAT16) if (${HDF_PREFIX}_SIZEOF__FLOAT16) - set (${HDF_PREFIX}_HAVE__FLOAT16 1) + # Ask for _Float16 support + set (CMAKE_REQUIRED_DEFINITIONS ${CMAKE_REQUIRED_DEFINITIONS} "-D__STDC_WANT_IEC_60559_TYPES_EXT__") + + # Some compilers expose the _Float16 datatype, but not the macros and + # functions used with the datatype. We need the macros for proper + # datatype conversion support. The main function we're interested in + # is fabsf16. Check for these here. + CHECK_SYMBOL_EXISTS (FLT16_EPSILON "float.h" h5_have_flt16_epsilon) + CHECK_SYMBOL_EXISTS (FLT16_MIN "float.h" h5_have_flt16_min) + CHECK_SYMBOL_EXISTS (FLT16_MAX "float.h" h5_have_flt16_max) + CHECK_SYMBOL_EXISTS (FLT16_MIN_10_EXP "float.h" h5_have_flt16_min_10_exp) + CHECK_SYMBOL_EXISTS (FLT16_MAX_10_EXP "float.h" h5_have_flt16_max_10_exp) + CHECK_SYMBOL_EXISTS (FLT16_MANT_DIG "float.h" h5_have_flt16_mant_dig) + + if (h5_have_flt16_epsilon AND h5_have_flt16_min AND + h5_have_flt16_max AND h5_have_flt16_min_10_exp AND + h5_have_flt16_max_10_exp AND h5_have_flt16_mant_dig) + set (${HDF_PREFIX}_HAVE__FLOAT16 1) + + # Check if we can use fabsf16 + CHECK_FUNCTION_EXISTS (fabsf16 ${HDF_PREFIX}_HAVE_FABSF16) + else () + set (${HDF_PREFIX}_HAVE__FLOAT16 0) + endif () else () set (${HDF_PREFIX}_HAVE__FLOAT16 0) endif () diff --git a/config/cmake/H5pubconf.h.in b/config/cmake/H5pubconf.h.in index b5dddba..5be6dc4 100644 --- a/config/cmake/H5pubconf.h.in +++ b/config/cmake/H5pubconf.h.in @@ -128,6 +128,9 @@ /* Define if library information should be embedded in the executables */ #cmakedefine H5_HAVE_EMBEDDED_LIBINFO @H5_HAVE_EMBEDDED_LIBINFO@ +/* Define to 1 if you have the `fabsf16' function. */ +#cmakedefine H5_HAVE_FABSF16 @H5_HAVE_FABSF16@ + /* Define to 1 if you have the `fcntl' function. */ #cmakedefine H5_HAVE_FCNTL @H5_HAVE_FCNTL@ diff --git a/configure.ac b/configure.ac index 1d26fb5..279d488 100644 --- a/configure.ac +++ b/configure.ac @@ -545,16 +545,57 @@ AC_CHECK_SIZEOF([long long]) AC_CHECK_SIZEOF([float]) AC_CHECK_SIZEOF([double]) AC_CHECK_SIZEOF([long double]) -AC_CHECK_SIZEOF([_Float16]) -# Define HAVE__FLOAT16 macro for H5pubconf.h if _Float16 is available. -# Also define HAVE__FLOAT16 value to substitute into other files for -# conditional testing +## ---------------------------------------------------------------------- +## Check if _Float16 support is available +## HAVE__FLOAT16="no" +AC_CHECK_SIZEOF([_Float16]) if test "$ac_cv_sizeof__Float16" != 0; then - HAVE__FLOAT16="yes" - AC_DEFINE([HAVE__FLOAT16], [1], [Determine if _Float16 is available]) + # Some compilers expose the _Float16 datatype, but not the macros and + # functions used with the datatype. We need the macros for proper + # datatype conversion support. The main function we're interested in + # is fabsf16. Check for these here. + AC_CHECK_DECL([FLT16_EPSILON], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + AC_CHECK_DECL([FLT16_MIN], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + AC_CHECK_DECL([FLT16_MAX], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + AC_CHECK_DECL([FLT16_MIN_10_EXP], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + AC_CHECK_DECL([FLT16_MAX_10_EXP], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + AC_CHECK_DECL([FLT16_MANT_DIG], [], [], [[ + #define __STDC_WANT_IEC_60559_TYPES_EXT__ + #include ]]) + + if test "X$ac_cv_have_decl_FLT16_EPSILON" = "Xyes" && + test "X$ac_cv_have_decl_FLT16_MIN" = "Xyes" && + test "X$ac_cv_have_decl_FLT16_MAX" = "Xyes" && + test "X$ac_cv_have_decl_FLT16_MIN_10_EXP" = "Xyes" && + test "X$ac_cv_have_decl_FLT16_MAX_10_EXP" = "Xyes" && + test "X$ac_cv_have_decl_FLT16_MANT_DIG" = "Xyes" ; then + HAVE__FLOAT16="yes" + + # Check if we can use fabsf16 + AC_CHECK_FUNC([fabsf16], [AC_DEFINE([HAVE_FABSF16], [1], + [Define if has fabsf16 function])], []) + + # Define HAVE__FLOAT16 macro for H5pubconf.h if _Float16 is available. + AC_DEFINE([HAVE__FLOAT16], [1], [Determine if _Float16 is available]) + fi + + AC_MSG_CHECKING([if _Float16 support is available]) + AC_MSG_RESULT([$HAVE__FLOAT16]) fi + +# Define HAVE__FLOAT16 value to substitute into other files for conditional testing AC_SUBST([HAVE__FLOAT16]) ## ---------------------------------------------------------------------- diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 33852a9..d1cc307 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -753,14 +753,38 @@ { \ if (*(S) > (ST)(D_MAX)) \ *(D) = H5_GLUE3(H5T_NATIVE_, DTYPE, _POS_INF_g); \ - else if (*(S) < (ST)(D_MIN)) \ - *(D) = H5_GLUE3(H5T_NATIVE_, DTYPE, _NEG_INF_g); \ - else \ - *(D) = (DT)(*(S)); \ + else { \ + intmax_t s_cast = (intmax_t)(*(S)); \ + intmax_t d_cast = (intmax_t)(D_MAX); \ + \ + /* Check if source value would underflow destination. Do NOT do this \ + * by comparing against D_MIN casted to type ST here, as this will \ + * generally be undefined behavior (casting negative float value <= 1.0 \ + * to integer) for all floating point types and some compilers optimize \ + * this in a way that causes unexpected behavior. Instead, grab the \ + * absolute value of the source value first, then compare it to D_MAX. \ + */ \ + if (s_cast != INTMAX_MIN) \ + s_cast = imaxabs(s_cast); \ + else { \ + /* Handle two's complement integer representations where abs(INTMAX_MIN) \ + * can't be represented. Other representations will fall here as well, \ + * but this should be fine. \ + */ \ + s_cast = INTMAX_MAX; \ + d_cast -= 1; \ + } \ + \ + if (s_cast > d_cast) \ + *(D) = H5_GLUE3(H5T_NATIVE_, DTYPE, _NEG_INF_g); \ + else \ + *(D) = (DT)(*(S)); \ + } \ } #define H5T_CONV_Xf(STYPE, DTYPE, ST, DT, D_MIN, D_MAX) \ do { \ + HDcompile_assert(sizeof(ST) >= sizeof(DT)); \ H5T_CONV(H5T_CONV_Xf, STYPE, DTYPE, ST, DT, D_MIN, D_MAX, Y) \ } while (0) @@ -9141,7 +9165,8 @@ H5T__conv_i_f(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz goto padding; } } - else { + + if (!cb_struct.func || (except_ret == H5T_CONV_UNHANDLED)) { /*make destination infinity by setting exponent to maximal number and *mantissa to zero.*/ expo = expo_max; diff --git a/src/H5private.h b/src/H5private.h index 0cbda2e..e47dbb3 100644 --- a/src/H5private.h +++ b/src/H5private.h @@ -27,7 +27,6 @@ #include #include #include -#include #include #include #include @@ -38,6 +37,7 @@ /* Define __STDC_WANT_IEC_60559_TYPES_EXT__ for _FloatN support, if available */ #define __STDC_WANT_IEC_60559_TYPES_EXT__ #include +#include /* POSIX headers */ #ifdef H5_HAVE_SYS_TIME_H @@ -462,10 +462,26 @@ #define H5_DBL_ABS_EQUAL(X, Y) (fabs((X) - (Y)) < DBL_EPSILON) #define H5_LDBL_ABS_EQUAL(X, Y) (fabsl((X) - (Y)) < LDBL_EPSILON) +#ifdef H5_HAVE__FLOAT16 +#ifdef H5_HAVE_FABSF16 +#define H5_FLT16_ABS_EQUAL(X, Y) (fabsf16((X) - (Y)) < FLT16_EPSILON) +#else +#define H5_FLT16_ABS_EQUAL(X, Y) H5_FLT_ABS_EQUAL((float)X, (float)Y) +#endif +#endif + #define H5_FLT_REL_EQUAL(X, Y, M) (fabsf(((Y) - (X)) / (X)) < (M)) #define H5_DBL_REL_EQUAL(X, Y, M) (fabs(((Y) - (X)) / (X)) < (M)) #define H5_LDBL_REL_EQUAL(X, Y, M) (fabsl(((Y) - (X)) / (X)) < (M)) +#ifdef H5_HAVE__FLOAT16 +#ifdef H5_HAVE_FABSF16 +#define H5_FLT16_REL_EQUAL(X, Y, M) (fabsf16(((Y) - (X)) / (X)) < (M)) +#else +#define H5_FLT16_REL_EQUAL(X, Y, M) H5_FLT_REL_EQUAL((float)X, (float)Y, M) +#endif +#endif + /* KiB, MiB, GiB, TiB, PiB, EiB - Used in profiling and timing code */ #define H5_KB (1024.0F) #define H5_MB (1024.0F * 1024.0F) @@ -543,16 +559,14 @@ #define H5_GCC_CLANG_DIAG_ON(x) #endif -/* Create a typedef for library usage of the _Float16 type - * to avoid issues when compiling the library with the - * -pedantic flag or similar where we get warnings about - * _Float16 not being an ISO C type. +/* If necessary, create a typedef for library usage of the + * _Float16 type to avoid issues when compiling the library + * with the -pedantic flag or similar where we get warnings + * about _Float16 not being an ISO C type. */ #ifdef H5_HAVE__FLOAT16 #if defined(__GNUC__) __extension__ typedef _Float16 H5__Float16; -#elif defined(__clang__) -/* TODO */ #else typedef _Float16 H5__Float16; #endif @@ -893,7 +907,7 @@ H5_DLL H5_ATTR_CONST int Nflock(int fd, int operation); #ifdef H5_HAVE_VASPRINTF #define HDvasprintf(RET, FMT, A) vasprintf(RET, FMT, A) #else -H5_DLL int HDvasprintf(char **bufp, const char *fmt, va_list _ap); +H5_DLL int HDvasprintf(char **bufp, const char *fmt, va_list _ap); #endif #endif diff --git a/test/dt_arith.c b/test/dt_arith.c index 653a575..65dc240 100644 --- a/test/dt_arith.c +++ b/test/dt_arith.c @@ -611,22 +611,30 @@ test_particular_fp_integer(void) int flag; double src_d = (double)SCHAR_MAX; signed char dst_c; - unsigned char *buf1 = NULL, *buf2 = NULL, *buf3 = NULL; - unsigned char *saved_buf1 = NULL, *saved_buf2 = NULL, *saved_buf3 = NULL; - size_t src_size1, src_size2; - size_t dst_size1, dst_size2; - float src_f = (float)INT_MAX; - int dst_i; - int fill_value = 13; + unsigned char *buf1 = NULL; + unsigned char *saved_buf1 = NULL; + size_t src_size1; + size_t dst_size1; int endian; /*endianness */ unsigned int fails_this_test = 0; size_t j; +#ifdef H5_WANT_DCONV_EXCEPTION + unsigned char *buf2 = NULL; + unsigned char *saved_buf2 = NULL; + size_t src_size2; + size_t dst_size2; + float src_f = (float)INT_MAX; + int fill_value = 13; + int dst_i; #ifdef H5_HAVE__FLOAT16 - H5__Float16 src_half = (H5__Float16)SHRT_MAX; - short s_fill_val = 25; - short dst_s; - size_t src_size3; - size_t dst_size3; + unsigned char *buf3 = NULL; + unsigned char *saved_buf3 = NULL; + H5__Float16 src_half = (H5__Float16)SHRT_MAX; + short s_fill_val = 25; + short dst_s; + size_t src_size3; + size_t dst_size3; +#endif #endif TESTING("hard particular floating number -> integer conversions"); @@ -688,6 +696,8 @@ test_particular_fp_integer(void) printf(" %29d\n", y); } +/* Only run this part of the test if conversion exceptions are enabled */ +#ifdef H5_WANT_DCONV_EXCEPTION /* Test conversion from float (the value is INT_MAX) to int. */ src_size2 = H5Tget_size(H5T_NATIVE_FLOAT); dst_size2 = H5Tget_size(H5T_NATIVE_INT); @@ -777,6 +787,7 @@ test_particular_fp_integer(void) printf(" %29d\n", (int)y); } #endif +#endif if (fails_this_test) goto error; @@ -789,16 +800,27 @@ test_particular_fp_integer(void) if (buf1) free(buf1); + +#ifdef H5_WANT_DCONV_EXCEPTION if (buf2) free(buf2); +#ifdef H5_HAVE__FLOAT16 if (buf3) free(buf3); +#endif +#endif + if (saved_buf1) free(saved_buf1); + +#ifdef H5_WANT_DCONV_EXCEPTION if (saved_buf2) free(saved_buf2); +#ifdef H5_HAVE__FLOAT16 if (saved_buf3) free(saved_buf3); +#endif +#endif PASSED(); return 0; @@ -812,16 +834,27 @@ error: H5E_END_TRY if (buf1) free(buf1); + +#ifdef H5_WANT_DCONV_EXCEPTION if (buf2) free(buf2); +#ifdef H5_HAVE__FLOAT16 if (buf3) free(buf3); +#endif +#endif + if (saved_buf1) free(saved_buf1); + +#ifdef H5_WANT_DCONV_EXCEPTION if (saved_buf2) free(saved_buf2); +#ifdef H5_HAVE__FLOAT16 if (saved_buf3) free(saved_buf3); +#endif +#endif /* Restore the default error handler (set in h5_reset()) */ h5_restore_err(); @@ -3383,15 +3416,21 @@ test_conv_flt_1(const char *name, int run_test, hid_t src, hid_t dst) /* Suppress warning about non-standard floating-point literal suffix */ H5_GCC_CLANG_DIAG_OFF("pedantic") - if (underflow && fabsf(x) <= (float)FLT16_MIN && fabsf(hw_half) <= (float)FLT16_MIN) +#ifdef H5_HAVE_FABSF16 + if (underflow && fabsf16(x) <= FLT16_MIN && fabsf16(hw_half) <= FLT16_MIN) continue; /* all underflowed, no error */ +#else + if (underflow && fabsf((float)x) <= (float)FLT16_MIN && + fabsf((float)hw_half) <= (float)FLT16_MIN) + continue; /* all underflowed, no error */ +#endif H5_GCC_CLANG_DIAG_ON("pedantic") if (overflow && my_isinf(dendian, buf + j * sizeof(H5__Float16), dst_size, dst_mpos, dst_msize, dst_epos, dst_esize)) continue; /* all overflowed, no error */ - check_mant[0] = (double)frexpf(x, check_expo + 0); - check_mant[1] = (double)frexpf(hw_half, check_expo + 1); + check_mant[0] = (double)frexpf((float)x, check_expo + 0); + check_mant[1] = (double)frexpf((float)hw_half, check_expo + 1); #else assert(0 && "Should not reach this point!"); #endif diff --git a/test/ntypes.c b/test/ntypes.c index 8439018..5d6b371 100644 --- a/test/ntypes.c +++ b/test/ntypes.c @@ -3124,13 +3124,14 @@ test__Float16(hid_t file) /* Check that the values read are the same as the values written */ for (size_t i = 0; i < DIM0; i++) - for (size_t j = 0; j < DIM1; j++) - if (!H5_FLT_ABS_EQUAL(ipoints->arr[i][j], icheck->arr[i][j])) { + for (size_t j = 0; j < DIM1; j++) { + if (!H5_FLT16_ABS_EQUAL(ipoints->arr[i][j], icheck->arr[i][j])) { H5_FAILED(); printf(" Read different values than written.\n"); printf(" At index %zu,%zu\n", i, j); goto error; } /* end if */ + } if (H5Sclose(space) < 0) TEST_ERROR; diff --git a/tools/lib/h5diff_array.c b/tools/lib/h5diff_array.c index 86d873d..dc658e3 100644 --- a/tools/lib/h5diff_array.c +++ b/tools/lib/h5diff_array.c @@ -2375,7 +2375,7 @@ diff_float16_element(unsigned char *mem1, unsigned char *mem2, hsize_t elem_idx, *------------------------------------------------------------------------- */ else { - if (equal_float(temp1_float16, temp2_float16, opts) == false) { + if (equal_float((float)temp1_float16, (float)temp2_float16, opts) == false) { opts->print_percentage = 0; print_pos(opts, elem_idx, 0); if (print_data(opts)) { -- cgit v0.12