Fix ARM NAN fraction bits
diff mbox

Message ID alpine.DEB.1.10.1405160245300.12061@tp.orcam.me.uk
State New
Headers show

Commit Message

Maciej W. Rozycki May 16, 2014, 4:57 p.m. UTC
On Thu, 27 Feb 2014, Joey Ye wrote:

> Current ARM soft-float implementation is violating the RTABI
> (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0043d/IHI0043D_rtabi.pd
> f) Section 4.1.1.1:
> 
> When not otherwise specified by IEEE 754, the result on an invalid operation
> should be the quiet NaN bit pattern with only the most significant bit of
> the significand set, and all other significand bits zero. 
> 
> This patch fixes it by setting _FP_NANFRAC_* to zero.
> 
> Ran make check test with –mfloat-abi=soft. No regression.
> 
> OK to checkin?
> 
> 2014-02-27  Joey Ye  <joey.ye@arm.com>
> * sysdeps/arm/soft-fp/sfp-machine.h 
>   (_FP_NANFRAC_S, _FP_NANFRAC_D, _FP_NANFRAC_Q):
>   Set to zero.
> 
> 
> diff --git a/sysdeps/arm/soft-fp/sfp-machine.h
> b/sysdeps/arm/soft-fp/sfp-machine.h
> index 52a08b5..32697fe 100644
> --- a/sysdeps/arm/soft-fp/sfp-machine.h
> +++ b/sysdeps/arm/soft-fp/sfp-machine.h
> @@ -21,9 +21,9 @@
> #define _FP_DIV_MEAT_D(R,X,Y)          _FP_DIV_MEAT_2_udiv(D,R,X,Y)
> #define _FP_DIV_MEAT_Q(R,X,Y)          _FP_DIV_MEAT_4_udiv(Q,R,X,Y)
> 
> -#define _FP_NANFRAC_S                          ((_FP_QNANBIT_S << 1) - 1)
> -#define _FP_NANFRAC_D                         ((_FP_QNANBIT_D << 1) - 1),
> -1
> -#define _FP_NANFRAC_Q                         ((_FP_QNANBIT_Q << 1) - 1),
> -1, -1, -1
> +#define _FP_NANFRAC_S                         0
> +#define _FP_NANFRAC_D                        0, 0
> +#define _FP_NANFRAC_Q                        0, 0, 0, 0
> #define _FP_NANSIGN_S                           0
> #define _FP_NANSIGN_D                          0
> #define _FP_NANSIGN_Q                          0

 This did regrettably, when propagated to libgcc, regress 
gcc.dg/torture/builtin-math-7.c on soft-fp arm-eabi targets, currently 
ARMv6-M (`-march=armv6-m -mthumb') only.  This is because these NANFRAC 
macros have now no bits set and as a result when used to construct a NaN 
in the semi-raw mode, they build an infinity instead.  Consequently 
operations such as (Inf - Inf) now produce Inf rather than NaN.  The 
change worked for the original test case, because division is made in the 
canonical mode, where the quiet bit is set separately, from the fp class.

 Here's a fix making code match the commit description quoted above, that 
is set the most significant bit of the significand.  This is also what 
targets similar in this respect do.

 OK to apply?  OK for libgcc (against libgcc/config/arm/sfp-machine.h), in 
particular for GCC 4.8 and 4.9?

2014-05-16  Maciej W. Rozycki  <macro@codesourcery.com>

	PR libgcc/60166
	* sysdeps/arm/soft-fp/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
	(_FP_NANSIGN_Q): Set the quiet bit.

  Maciej

glibc-soft-fp-arm-nanfrac.diff

Comments

Joseph Myers May 16, 2014, 5:03 p.m. UTC | #1
On Fri, 16 May 2014, Maciej W. Rozycki wrote:

> 2014-05-16  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	PR libgcc/60166
> 	* sysdeps/arm/soft-fp/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
> 	(_FP_NANSIGN_Q): Set the quiet bit.

OK for glibc.
Maciej W. Rozycki May 16, 2014, 10:22 p.m. UTC | #2
On Fri, 16 May 2014, Joseph S. Myers wrote:

> > 2014-05-16  Maciej W. Rozycki  <macro@codesourcery.com>
> > 
> > 	PR libgcc/60166
> > 	* sysdeps/arm/soft-fp/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
> > 	(_FP_NANSIGN_Q): Set the quiet bit.
> 
> OK for glibc.

 Joseph, thanks for your review, this is now in.

 Richard, you wrote yesterday that pushing changes to 4.8 would require 
explicit approval from release managers, however it is not clear to me who 
they are for that branch.  This fix corrects a regression introduced after 
4.8.2.  Can you approve it?  If not, then who can?

  Maciej
Richard Biener May 17, 2014, 8:35 a.m. UTC | #3
On May 17, 2014 12:22:23 AM CEST, "Maciej W. Rozycki" <macro@codesourcery.com> wrote:
>On Fri, 16 May 2014, Joseph S. Myers wrote:
>
>> > 2014-05-16  Maciej W. Rozycki  <macro@codesourcery.com>
>> > 
>> > 	PR libgcc/60166
>> > 	* sysdeps/arm/soft-fp/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
>> > 	(_FP_NANSIGN_Q): Set the quiet bit.
>> 
>> OK for glibc.
>
> Joseph, thanks for your review, this is now in.
>
>Richard, you wrote yesterday that pushing changes to 4.8 would require 
>explicit approval from release managers, however it is not clear to me
>who 
>they are for that branch.  This fix corrects a regression introduced
>after 
>4.8.2.  Can you approve it?  If not, then who can?

If it's not broken in 4.8.2 but broken on the branch head then it's OK for the branch.

Thanks,
Richard.

>  Maciej

Patch
diff mbox

Index: glibc-fsf-trunk-quilt/sysdeps/arm/soft-fp/sfp-machine.h
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/soft-fp/sfp-machine.h	2014-05-16 03:25:52.000000000 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/soft-fp/sfp-machine.h	2014-05-16 03:31:34.451805339 +0100
@@ -21,9 +21,9 @@ 
 #define _FP_DIV_MEAT_D(R,X,Y)	_FP_DIV_MEAT_2_udiv(D,R,X,Y)
 #define _FP_DIV_MEAT_Q(R,X,Y)	_FP_DIV_MEAT_4_udiv(Q,R,X,Y)
 
-#define _FP_NANFRAC_S		0
-#define _FP_NANFRAC_D		0, 0
-#define _FP_NANFRAC_Q		0, 0, 0, 0
+#define _FP_NANFRAC_S		_FP_QNANBIT_S
+#define _FP_NANFRAC_D		_FP_QNANBIT_D, 0
+#define _FP_NANFRAC_Q		_FP_QNANBIT_Q, 0, 0, 0
 #define _FP_NANSIGN_S		0
 #define _FP_NANSIGN_D		0
 #define _FP_NANSIGN_Q		0