diff mbox

[2/3] PowerPC: remove wrong nearbyintl implementation for, PowerPC64

Message ID 532309C7.6070303@linux.vnet.ibm.com
State New
Headers show

Commit Message

Adhemerval Zanella March 14, 2014, 1:53 p.m. UTC 
The nearbyintl assembly implementation
(sysdeps/powerpc/powerpc64/fpu/s_nearbyintl.S)
returns wrong results for some inputs where first double is a exact
integer and the precision is determined by second long double.

Checking on implementation comments and history, I am very confident the
assembly implementation was based on a version before commit
5c68d401698a58cf7da150d9cce769fa6679ba5f that fixes BZ#2423 (Errors in
long double (ldbl-128ibm) rounding functions in glibc-2.4).

By just removing the implementation and make the build select
sysdeps/ieee754/ldbl-128ibm/s_nearbyintl.c instead fixes the failing math.

Fixes BZ#16706.

Test on PPC64 and PPC64LE. If no one opposes it, I'll commit in a couple
of hours.

--

2014-03-14  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>

	[BZ #16706]
	* sysdeps/powerpc/powerpc64/fpu/s_nearbyintl.S: Remove wrong
	implementation.
	* math/libm-test.inc (nearbyint_test_data): Add more tests.

---

Comments

Joseph Myers March 14, 2014, 2:34 p.m. UTC | #1 
On Fri, 14 Mar 2014, Adhemerval Zanella wrote:

> +#ifdef TEST_LDOUBLE
> +    /* Check cases where first double is a exact integer higher than 2^52 and
> +       the precision is determined by second long double for IBM long double.  */
> +    TEST_f_f (nearbyint,  34503599627370498.515625L, 34503599627370499.0L),
> +    TEST_f_f (nearbyint,  1192568192774434123539907640624.484375L, 1192568192774434123539907640624.0L),
> +    TEST_f_f (nearbyint, -34503599627370498.515625L, -34503599627370499.0L),
> +    TEST_f_f (nearbyint, -1192568192774434123539907640624.484375L, -1192568192774434123539907640624.0L),
> +#endif

What are the precision requirements on long double for these inputs to be 
representable?  If any require more than 64-bit mantissa, they should be 
conditional appropriately on LDBL_MANT_DIG (which may be 64, 106 or 113).
Joseph Myers March 14, 2014, 2:48 p.m. UTC | #2 
On Fri, 14 Mar 2014, Joseph S. Myers wrote:

> On Fri, 14 Mar 2014, Adhemerval Zanella wrote:
> 
> > +#ifdef TEST_LDOUBLE
> > +    /* Check cases where first double is a exact integer higher than 2^52 and
> > +       the precision is determined by second long double for IBM long double.  */
> > +    TEST_f_f (nearbyint,  34503599627370498.515625L, 34503599627370499.0L),
> > +    TEST_f_f (nearbyint,  1192568192774434123539907640624.484375L, 1192568192774434123539907640624.0L),
> > +    TEST_f_f (nearbyint, -34503599627370498.515625L, -34503599627370499.0L),
> > +    TEST_f_f (nearbyint, -1192568192774434123539907640624.484375L, -1192568192774434123539907640624.0L),
> > +#endif
> 
> What are the precision requirements on long double for these inputs to be 
> representable?  If any require more than 64-bit mantissa, they should be 
> conditional appropriately on LDBL_MANT_DIG (which may be 64, 106 or 113).

Note that this applies to the other functions as well.  In some cases a 
test may accidentally pass without such a conditional, if both the input 
and expected output round-to-nearest to the same long double value.  But 
that won't help for these tests when I make rint and nearbyint use the 
same tests (once 
<https://sourceware.org/ml/libc-alpha/2014-03/msg00214.html> is reviewed), 
with the tests for rint systematically expecting "inexact".  (These 
nearbyint tests in your patch are also missing NO_INEXACT_EXCEPTION, which 
should be specified as that's the whole difference between rint and 
nearbyint.)
Adhemerval Zanella March 14, 2014, 2:54 p.m. UTC | #3 
On 14-03-2014 11:48, Joseph S. Myers wrote:
> On Fri, 14 Mar 2014, Joseph S. Myers wrote:
>
>> On Fri, 14 Mar 2014, Adhemerval Zanella wrote:
>>
>>> +#ifdef TEST_LDOUBLE
>>> +    /* Check cases where first double is a exact integer higher than 2^52 and
>>> +       the precision is determined by second long double for IBM long double.  */
>>> +    TEST_f_f (nearbyint,  34503599627370498.515625L, 34503599627370499.0L),
>>> +    TEST_f_f (nearbyint,  1192568192774434123539907640624.484375L, 1192568192774434123539907640624.0L),
>>> +    TEST_f_f (nearbyint, -34503599627370498.515625L, -34503599627370499.0L),
>>> +    TEST_f_f (nearbyint, -1192568192774434123539907640624.484375L, -1192568192774434123539907640624.0L),
>>> +#endif
>> What are the precision requirements on long double for these inputs to be 
>> representable?  If any require more than 64-bit mantissa, they should be 
>> conditional appropriately on LDBL_MANT_DIG (which may be 64, 106 or 113).
> Note that this applies to the other functions as well.  In some cases a 
> test may accidentally pass without such a conditional, if both the input 
> and expected output round-to-nearest to the same long double value.  But 
> that won't help for these tests when I make rint and nearbyint use the 
> same tests (once 
> <https://sourceware.org/ml/libc-alpha/2014-03/msg00214.html> is reviewed), 
> with the tests for rint systematically expecting "inexact".  (These 
> nearbyint tests in your patch are also missing NO_INEXACT_EXCEPTION, which 
> should be specified as that's the whole difference between rint and 
> nearbyint.)
>
Yes I check here and although 34503599627370498.515625L is representable with 64 bits mantissa,
1192568192774434123539907640624.484375 it is not. I'll fix it accordingly and also on
other ibm ldbl128 rounding fixes. Thanks for catching it.
diff mbox

Patch

diff --git a/NEWS b/NEWS
index 16a17c8..c9bfcb8 100644
--- a/NEWS
+++ b/NEWS
@@ -11,7 +11,7 @@  Version 2.20
 
   15347, 15804, 15894, 16447, 16532, 16545, 16574, 16600, 16609, 16610,
   16611, 16613, 16623, 16632, 16639, 16642, 16670, 16674, 16677, 16683,
-  16689, 16695, 16701.
+  16689, 16695, 16701, 16706.
 
 * The am33 port, which had not worked for several years, has been removed
   from ports.
diff --git a/math/libm-test.inc b/math/libm-test.inc
index 59cfe61..ee0a353 100644
--- a/math/libm-test.inc
+++ b/math/libm-test.inc
@@ -9519,6 +9519,14 @@  static const struct test_f_f_data nearbyint_test_data[] =
     TEST_f_f (nearbyint, -562949953421312.75, -562949953421313.0, NO_INEXACT_EXCEPTION),
     TEST_f_f (nearbyint, -1125899906842624.75, -1125899906842625.0, NO_INEXACT_EXCEPTION),
 #endif
+#ifdef TEST_LDOUBLE
+    /* Check cases where first double is a exact integer higher than 2^52 and
+       the precision is determined by second long double for IBM long double.  */
+    TEST_f_f (nearbyint,  34503599627370498.515625L, 34503599627370499.0L),
+    TEST_f_f (nearbyint,  1192568192774434123539907640624.484375L, 1192568192774434123539907640624.0L),
+    TEST_f_f (nearbyint, -34503599627370498.515625L, -34503599627370499.0L),
+    TEST_f_f (nearbyint, -1192568192774434123539907640624.484375L, -1192568192774434123539907640624.0L),
+#endif
   };
 
 static void
diff --git a/sysdeps/powerpc/powerpc64/fpu/s_nearbyintl.S b/sysdeps/powerpc/powerpc64/fpu/s_nearbyintl.S
deleted file mode 100644
index acd95da..0000000
--- a/sysdeps/powerpc/powerpc64/fpu/s_nearbyintl.S
+++ /dev/null
@@ -1,113 +0,0 @@ 
-/* nearbyint long double.
-   IBM extended format long double version.
-   Copyright (C) 2004-2014 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sysdep.h>
-#include <math_ldbl_opt.h>
-
-	.section	".toc","aw"
-.LC0:	/* 2**52 */
-	.tc FD_43300000_0[TC],0x4330000000000000
-	.section	".text"
-
-/* long double [fp1,fp2] nearbyintl (long double x [fp1,fp2])
-   IEEE 1003.1 nearbyintl function.  nearbyintl is similar to the rintl
-   but does raise the "inexact" exception.  This implementation is
-   based on rintl but explicitly masks the inexact exception on entry
-   and clears any pending inexact before restoring the exception mask
-   on exit.
-
-   PowerPC64 long double uses the IBM extended format which is
-   represented two 64-floating point double values. The values are
-   non-overlapping giving an effective precision of 106 bits. The first
-   double contains the high order bits of mantissa and is always rounded
-   to represent a normal rounding of long double to double. Since the
-   long double value is sum of the high and low values, the low double
-   normally has the opposite sign to compensate for the this rounding.
-
-   For long double there are two cases:
-   1) |x| < 2**52, all the integer bits are in the high double.
-      floor the high double and set the low double to -0.0.
-   2) |x| >= 2**52, Rounding involves both doubles.
-      See the comment before label .L2 for details.
-   */
-ENTRY (__nearbyintl)
-	mffs	fp11		/* Save current FPSCR.  */
-	lfd	fp13,.LC0@toc(2)
-	fabs	fp0,fp1
-	mtfsb0  28		/* Disable "inexact" exceptions.  */
-	fsub	fp12,fp13,fp13	/* generate 0.0  */
-	fabs	fp9,fp2
-	fcmpu	cr7,fp0,fp13	/* if (fabs(x) > TWO52)  */
-	fcmpu	cr6,fp1,fp12	/* if (x > 0.0)  */
-	bnl-	cr7,.L2
-	fmr	fp2,fp12
-	bng-	cr6,.L4
-	fadd	fp1,fp1,fp13	/* x+= TWO52;  */
-	fsub	fp1,fp1,fp13	/* x-= TWO52;  */
-	b	.L9
-.L4:
-	bnl-	cr6,.L9		/* if (x < 0.0)  */
-	fsub	fp1,fp13,fp1	/* x = TWO52 - x;  */
-	fsub	fp0,fp1,fp13	/* x = - (x - TWO52);  */
-	fneg	fp1,fp0
-.L9:
-	mtfsb0	6		/* Clear any pending "inexact" exceptions.  */
-	mtfsf	0x01,fp11	/* restore exception mask.  */
-	blr
-
-/* The high double is > TWO52 so we need to round the low double and
-   perhaps the high double.  This gets a bit tricky so we use the
-   following algorithm:
-
-   tau = floor(x_high/TWO52);
-   x0 = x_high - tau;
-   x1 = x_low + tau;
-   r1 = nearbyint(x1);
-   y_high = x0 + r1;
-   y_low = r1 - tau;
-   return y;  */
-.L2:
-	fcmpu	cr7,fp9,fp13	/* if (|x_low| > TWO52)  */
-	fcmpu	cr0,fp9,fp12	/* || (|x_low| == 0.0)  */
-	bge-	cr7,.L9		/*   return x;	*/
-	beq-  cr0,.L9
-	fdiv	fp8,fp1,fp13	/* x_high/TWO52  */
-	fctidz	fp0,fp8
-	fcfid	fp8,fp0		/* tau = floor(x_high/TWO52);  */
-	fsub	fp3,fp1,fp8	/* x0 = x_high - tau;  */
-	fadd	fp4,fp2,fp8	/* x1 = x_low + tau;  */
-
-	fcmpu	cr6,fp4,fp12	/* if (x1 > 0.0)  */
-	bng-	cr6,.L8
-	fadd	fp5,fp4,fp13	/* r1 = x1 + TWO52;  */
-	fsub	fp5,fp5,fp13	/* r1 = r1 - TWO52;  */
-	b	.L6
-.L8:
-	fmr	fp5,fp4
-	bge-	cr6,.L6		/* if (x1 < 0.0)  */
-	fsub	fp5,fp13,fp4	/* r1 = TWO52 - x1;  */
-	fsub	fp0,fp5,fp13	/* r1 = - (r1 - TWO52);  */
-	fneg	fp5,fp0
-.L6:
-	fadd	fp1,fp3,fp5	/* y_high = x0 + r1;  */
-	fsub	fp2,fp5,fp8	/* y_low = r1 - tau;  */
-	b	.L9
-END (__nearbyintl)
-
-long_double_symbol (libm, __nearbyintl, nearbyintl)