Re: [PATCH] Fix ARM NAN fraction bits

Re: [PATCH] Fix ARM NAN fraction bits

[Maciej W. Rozycki]

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
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

Re: [PATCH] Fix ARM NAN fraction bits

[Joseph S. Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix ARM NAN fraction bits

[Maciej W. Rozycki]

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

Re: [PATCH] Fix ARM NAN fraction bits

[Richard Biener]

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

Re: [PATCH] Fix ARM NAN fraction bits

[Maciej W. Rozycki]

Ian,

On Sat, 17 May 2014, Richard Biener wrote:

> 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.

 I thought I'd double-check with you that it is fine to push this change 
to trunk first.  OK to apply?

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

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

  Maciej

gcc-soft-fp-arm-nanfrac.diff
Index: gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/libgcc/config/arm/sfp-machine.h	2014-05-16 15:59:06.000000000 +0100
+++ gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h	2014-05-20 01:23:36.618434199 +0100
@@ -21,10 +21,10 @@ typedef int __gcc_CMPtype __attribute__ 
 
 /* According to RTABI, QNAN is only with the most significant bit of the
    significand set, and all other significand bits zero.  */
-#define _FP_NANFRAC_H		0
-#define _FP_NANFRAC_S		0
-#define _FP_NANFRAC_D		0, 0
-#define _FP_NANFRAC_Q		0, 0, 0, 0
+#define _FP_NANFRAC_H		_FP_QNANBIT_H
+#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_H		0
 #define _FP_NANSIGN_S		0
 #define _FP_NANSIGN_D		0

Re: [PATCH] Fix ARM NAN fraction bits

[Richard Biener]

On Tue, 20 May 2014, Maciej W. Rozycki wrote:

> Ian,
> 
> On Sat, 17 May 2014, Richard Biener wrote:
> 
> > 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.
> 
>  I thought I'd double-check with you that it is fine to push this change 
> to trunk first.  OK to apply?

Of course it should go to trunk (and 4.9 branch) first, but I'm not
the one to approve that.

Richard.

> 2014-05-20  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	PR libgcc/60166
> 	libgcc/
> 	* config/arm/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
> 	(_FP_NANSIGN_Q): Set the quiet bit.
> 
>   Maciej
> 
> gcc-soft-fp-arm-nanfrac.diff
> Index: gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h
> ===================================================================
> --- gcc-fsf-trunk-quilt.orig/libgcc/config/arm/sfp-machine.h	2014-05-16 15:59:06.000000000 +0100
> +++ gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h	2014-05-20 01:23:36.618434199 +0100
> @@ -21,10 +21,10 @@ typedef int __gcc_CMPtype __attribute__ 
>  
>  /* According to RTABI, QNAN is only with the most significant bit of the
>     significand set, and all other significand bits zero.  */
> -#define _FP_NANFRAC_H		0
> -#define _FP_NANFRAC_S		0
> -#define _FP_NANFRAC_D		0, 0
> -#define _FP_NANFRAC_Q		0, 0, 0, 0
> +#define _FP_NANFRAC_H		_FP_QNANBIT_H
> +#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_H		0
>  #define _FP_NANSIGN_S		0
>  #define _FP_NANSIGN_D		0
> 
> 

Re: [PATCH] Fix ARM NAN fraction bits

[Richard Earnshaw]

On 20/05/14 08:57, Richard Biener wrote:
> On Tue, 20 May 2014, Maciej W. Rozycki wrote:
> 
>> Ian,
>>
>> On Sat, 17 May 2014, Richard Biener wrote:
>>
>>> 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.
>>
>>  I thought I'd double-check with you that it is fine to push this change 
>> to trunk first.  OK to apply?
> 
> Of course it should go to trunk (and 4.9 branch) first, but I'm not
> the one to approve that.
> 

I can...  Based on Joseph's review, OK.

R.

> Richard.
> 
>> 2014-05-20  Maciej W. Rozycki  <macro@codesourcery.com>
>>
>> 	PR libgcc/60166
>> 	libgcc/
>> 	* config/arm/sfp-machine.h (_FP_NANFRAC_S, _FP_NANFRAC_D)
>> 	(_FP_NANSIGN_Q): Set the quiet bit.
>>
>>   Maciej
>>
>> gcc-soft-fp-arm-nanfrac.diff
>> Index: gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h
>> ===================================================================
>> --- gcc-fsf-trunk-quilt.orig/libgcc/config/arm/sfp-machine.h	2014-05-16 15:59:06.000000000 +0100
>> +++ gcc-fsf-trunk-quilt/libgcc/config/arm/sfp-machine.h	2014-05-20 01:23:36.618434199 +0100
>> @@ -21,10 +21,10 @@ typedef int __gcc_CMPtype __attribute__ 
>>  
>>  /* According to RTABI, QNAN is only with the most significant bit of the
>>     significand set, and all other significand bits zero.  */
>> -#define _FP_NANFRAC_H		0
>> -#define _FP_NANFRAC_S		0
>> -#define _FP_NANFRAC_D		0, 0
>> -#define _FP_NANFRAC_Q		0, 0, 0, 0
>> +#define _FP_NANFRAC_H		_FP_QNANBIT_H
>> +#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_H		0
>>  #define _FP_NANSIGN_S		0
>>  #define _FP_NANSIGN_D		0
>>
>>
> 

Re: [PATCH] Fix ARM NAN fraction bits

[Maciej W. Rozycki]

On Tue, 20 May 2014, Richard Earnshaw wrote:

> >>> If it's not broken in 4.8.2 but broken on the branch head then it's OK 
> >>> for the branch.
> >>
> >>  I thought I'd double-check with you that it is fine to push this change 
> >> to trunk first.  OK to apply?
> > 
> > Of course it should go to trunk (and 4.9 branch) first, but I'm not
> > the one to approve that.
> > 
> 
> I can...  Based on Joseph's review, OK.

 I have now applied it to trunk and backported to 4.9 and 4.8.  Thank you 
both for the review.

  Maciej