This is the mail archive of the
`libc-alpha@sourceware.org`
mailing list for the glibc project.

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |

Other format: | [Raw text] |

*From*: Joseph Myers <joseph at codesourcery dot com>*To*: Rajalakshmi Srinivasaraghavan <raji at linux dot vnet dot ibm dot com>*Cc*: <libc-alpha at sourceware dot org>*Date*: Fri, 20 May 2016 12:15:06 +0000*Subject*: Re: [PATCH] Add nextup and nextdown math functions*Authentication-results*: sourceware.org; auth=none*References*: <1463727477-1909-1-git-send-email-raji at linux dot vnet dot ibm dot com>

On Fri, 20 May 2016, Rajalakshmi Srinivasaraghavan wrote: > This patch adds nextup and nextdown functions from TS 18661-1. New functions need NEWS entries. I think the patch submission (which will become the commit log entry) should also say more about these functions and the rationale for adding these particular functions on their own, i.e. that TS 18661-3 does not include nexttoward analogues for new floating-point types, but instead adds nextup and nextdown functions alongside nextafter, so to provide support for float128 equivalent to that for other floating-point types it's natural to add nextup and nextdown first. It should also discuss the design choices in the implementation. > diff --git a/conform/data/math.h-data b/conform/data/math.h-data > index 7153333..e96bb4e 100644 > --- a/conform/data/math.h-data > +++ b/conform/data/math.h-data > @@ -128,6 +128,8 @@ function int ilogb (double) > function double log1p (double) > function double logb (double) > function double nextafter (double, double) > +function double nextdown (double) > +function double nextup (double) This is not correct. These functions are not in ISO C so must not be expected to be in ISO C, or any of the existing standards supported by conformtest. Since you correctly condition the functions on __USE_GNU (absent a framework for supporting ISO-style feature test macros), I'd have expected this to cause test failures - did you not run the tests for conform/? > diff --git a/conform/data/tgmath.h-data b/conform/data/tgmath.h-data > index 5f72502..71256f9 100644 > --- a/conform/data/tgmath.h-data > +++ b/conform/data/tgmath.h-data > @@ -48,7 +48,9 @@ macro lrint > macro lround > macro nearbyint > macro nextafter > +macro nextdown > macro nexttoward > +macro nextup Again, not correct. > +@comment math.h > +@comment ISO > +@deftypefun double nextdown (double @var{x}) > +@comment math.h > +@comment ISO > +@deftypefunx float nextdownf (float @var{x}) > +@comment math.h > +@comment ISO > +@deftypefunx {long double} nextdownl (long double @var{x}) Right now, we use "ISO" to mean ISO C not these extensions. See how issignaling is documented as GNU. > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}} > +The @code{nextdown} function returns the next representable value > +less than @var{x}. If @var{x} is the positive number of least > +magnitude the function returns +0 if the type has signed zeros > +and is 0 otherwise. If @var{x} is zero, the function returns > +the negative number of least magnitude in the type of @var{x}. > +nextdown(@code{-HUGE_VAL}) is @code{-HUGE_VAL} I don't think we need to talk about types without signed zero. > +This function is defined in @w{IEC 559} (and the appendix with > +recommended functions in @w{IEEE 754}/@w{IEEE 854}). No. It's ISO/IEC/IEEE 60559 in the current edition that defines this and this is not about an appendix, nextUp/nextDown are in the main body. > +@comment math.h > +@comment ISO > +@deftypefun double nextup (double @var{x}) > +@comment math.h > +@comment ISO > +@deftypefunx float nextupf (float @var{x}) > +@comment math.h > +@comment ISO > +@deftypefunx {long double} nextupl (long double @var{x}) > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}} Same comments. > diff --git a/math/bits/mathcalls.h b/math/bits/mathcalls.h > index 9a7b3f0..5a7651d 100644 > --- a/math/bits/mathcalls.h > +++ b/math/bits/mathcalls.h > @@ -287,13 +287,19 @@ __BEGIN_NAMESPACE_C99 > /* Return the integer nearest X in the direction of the > prevailing rounding mode. */ > __MATHCALL (rint,, (_Mdouble_ __x)); > - > /* Return X + epsilon if X < Y, X - epsilon if X > Y. */ Gratuitous change of unrelated code. > +static const struct test_f_f_data nextup_test_data[] = > + { > + TEST_f_f (nextup, minus_zero, min_subnorm_value, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, 0, min_subnorm_value, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, plus_zero, min_subnorm_value, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, -min_subnorm_value, minus_zero, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_ERANGE), > + TEST_f_f (nextup, max_value, plus_infty, INEXACT_EXCEPTION|OVERFLOW_EXCEPTION|ERRNO_ERANGE), > + TEST_f_f (nextup, plus_infty, plus_infty, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, minus_infty, -max_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, qnan_value, qnan_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED), > + TEST_f_f (nextup, -qnan_value, qnan_value, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED), Many of these expectations are wrong. nextup and nextdown are *not* the same as nextafter regarding exceptions; to quote IEEE 754-2008 / ISO/IEC/IEEE 60559:2011, "nextUp(x) is quiet except for sNaNs.". > +static const struct test_f_f_data nextdown_test_data[] = Likewise. > + Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997. No "Contributed by" in new files. > +/* > + * nextup(x) > + * return the least floating-point number in the > + * format of x that compares greater than x. Badly formatted comment. > +FLOAT > +INTERNAL(NEXTUP) (FLOAT x) > +{ > +#ifdef USE_AS_NEXTDOWN > + return NEXTAFTER (x, -INFINITY); > +#else > + return NEXTAFTER (x, INFINITY); > +#endif As discussed, this is incorrect. nextafter, as in the appendix to IEEE 754-1985, signals underflow and overflow in certain cases (but is still independent of the rounding mode), but nextup / nextdown do not. So you can't implement these functions in terms of nextafter (without saving / restoring exceptions and making special allowance to ensure exceptions are still raised for sNaN, which is clearly an excessively inefficient way of doing things). > diff --git a/math/tgmath.h b/math/tgmath.h > index ea2b611..62ae907 100644 > --- a/math/tgmath.h > +++ b/math/tgmath.h > @@ -391,6 +391,10 @@ > /* Return the integer nearest X in the direction of the > prevailing rounding mode. */ > #define rint(Val) __TGMATH_UNARY_REAL_ONLY (Val, rint) > +/* Return X - epsilon. */ > +#define nextdown(Val) __TGMATH_UNARY_REAL_ONLY (Val, nextdown) > +/* Return X + epsilon. */ > +#define nextup(Val) __TGMATH_UNARY_REAL_ONLY (Val, nextup) You need __USE_GNU conditionals here. > diff --git a/sysdeps/unix/sysv/linux/alpha/libm.abilist b/sysdeps/unix/sysv/linux/alpha/libm.abilist > index 611dfe1..0a5b83c 100644 > --- a/sysdeps/unix/sysv/linux/alpha/libm.abilist > +++ b/sysdeps/unix/sysv/linux/alpha/libm.abilist > @@ -563,3 +563,10 @@ GLIBC_2.4 truncl F > GLIBC_2.4 y0l F > GLIBC_2.4 y1l F > GLIBC_2.4 ynl F > +GLIBC_2.24 GLIBC_2.24 A > +GLIBC_2.24 nextdown F > +GLIBC_2.24 nextdownf F > +GLIBC_2.24 nextdownl F > +GLIBC_2.24 nextup F > +GLIBC_2.24 nextupf F > +GLIBC_2.24 nextupl F These files are sorted strictly according to LC_ALL=C collation. In this one and many others, you have placed the new entries out of order, so the ABI tests would fail. > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libm.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libm.abilist > index 6c7fc9b..13a4633 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libm.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libm.abilist > @@ -508,3 +508,10 @@ GLIBC_2.4 truncl F > GLIBC_2.4 y0l F > GLIBC_2.4 y1l F > GLIBC_2.4 ynl F > +GLIBC_2.24 GLIBC_2.24 A Since this is one of the files out of order, and since you said you tested on powerpc64, I'd have expected you to get test failures there. Just testing isn't enough without understanding any failures and making sure they are unrelated to your patch.... I think you should also update sysdeps/ieee754/ldbl-opt/ so that there are versions of nextdownl and nextupl in libnldbl.a (that's for use when linking programs built with -mlong-double-64 and either their own declarations of long double functions, or including <math.h> with a compiler not supporting asm redirections). -- Joseph S. Myers joseph@codesourcery.com

**References**:**[PATCH] Add nextup and nextdown math functions***From:*Rajalakshmi Srinivasaraghavan

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |