Re: [PATCH v3] Add nextup and nextdown math functions

[Joseph Myers <joseph at codesourcery dot com>]

On Mon, 6 Jun 2016, Rajalakshmi Srinivasaraghavan wrote:

> Addressed the comments on v2.
> Tested on powerpc64 and powerpc64le.I have not validated TEST_COND_binary128
> tests added in math/libm-test.inc.

Given there is a whole function implementation for ldbl-128, I think it 
should be tested.  I presume you have access to S/390 systems at IBM (as 
one example of an architecture using ldbl-128).

> +* nextupl, nextup, nextupf, nextdownl, nextdown and nextdownf are added
> +  to libm. They are defined by TS 18661 and IEEE754-2008. The nextup functions

Two spaces after "." at end of sentence (twice on this line).

> +  return the next representable value in the direction of positive infinity
> +  and the nextdown functions return the next representable value in the
> +  direction of negative infinity.  These are currently enabled as GNU
> +  extension.

"extensions".

> +@var{x} in the direction of positive infinity.  If @math{@var{x} =  @code{0}}

Should have one space after "=", not two.

> +#if TEST_COND_m68k96 || TEST_COND_intel96
> +    TEST_ff_f (nextafter, 1.0L, 2.0L, 0x8.0000000000000010p-3L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, 1.0L, -2.0L, 0xf.fffffffffffffff0p-4L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, -1.0L, -2.0L, -0x8.0000000000000010p-3L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, -1.0L, 2.0L, -0xf.fffffffffffffff0p-4L, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, 0x0.00000040000000000000p-16385L, -0.1L, 0x0.0000003ffffffff00000p-16385L, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_ERANGE),
> +    TEST_ff_f (nextafter, 0x0.00000040000000000000p-16385L, 1.0L, 0x0.0000004000000010p-16385L, INEXACT_EXCEPTION|UNDERFLOW_EXCEPTION|ERRNO_ERANGE),

For subnormals, the tests for TEST_COND_m68k96 and TEST_COND_intel96 need 
to be separate, as the m68k version of this format has one extra bit of 
precision for subnormals (via the exponent represented as 0 acting as 
exponent -16383, so that -16383 is a normal exponent and exponents -16384 
and below are the subnormal exponents).

> +#if TEST_COND_binary128
> +    TEST_ff_f (nextafter, 1.0L, 10.0L, 0x1.0000000000001p0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, 1.0L, -10.0L, 0x0.fffffffffffff8p0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, -1.0L, INFINITY, -0x1.0000000000001p0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_ff_f (nextafter, -1.0L, -INFINITY, -0x0.fffffffffffff8p0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),

And these tests are wrong.  Should have the "L" suffix on the expectations 
(until the patch to add suffixes automatically is in, anyway); tests 
conventionally use plus_infty / minus_infty; and the expectations should 
be actual values for 113-bit mantissas, not the 53-bit examples here.

Same applies to the tests for nextup and nextdown (and it might be a good 
idea to include my recently added nextafter test for bug 20205 in the 
nextup tests).

> diff --git a/math/s_nextup.c b/math/s_nextup.c

Although being in math/ matches what's done with nextafter, I think what's 
done with nextafter is a mistake; s_nextafter.c is very clearly specific 
to binary64 representation and so belongs in sysdeps/ieee754/dbl-64.  (Not 
that I think we should consider supporting float and double types other 
than binary32 and binary64, but putting it in sysdeps/ieee754/dbl-64 would 
be consistent with other functions.)

> diff --git a/sysdeps/ieee754/ldbl-64-128/s_nextupl.c b/sysdeps/ieee754/ldbl-64-128/s_nextupl.c
> new file mode 100644
> index 0000000..4a04bd5
> --- /dev/null
> +++ b/sysdeps/ieee754/ldbl-64-128/s_nextupl.c
> @@ -0,0 +1,5 @@
> +#include <math_ldbl_opt.h>
> +#undef weak_alias
> +#define weak_alias(n,a)
> +#include <sysdeps/ieee754/ldbl-128/s_nextupl.c>
> +long_double_symbol (libm, __nextupl, nextupl);

I'm not sure whether this is correct or not for symbols added after the 
move from long double = double.  S/390 testing should show whether it is 
OK.  In any case, I'm pretty sure it's not needed - even if nothing goes 
wrong as a result of having this file, because the functions postdate that 
move you don't actually need special versioning for them.

-- 
Joseph S. Myers
joseph@codesourcery.com