Re: [PATCH] Add nextup and nextdown math functions

[Joseph Myers <joseph at codesourcery 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