Patchwork [2/6] softfloat: fix float{32, 64}_maybe_silence_nan() for MIPS

login
register
mail settings
Submitter Aurelien Jarno
Date Jan. 3, 2011, 2:34 p.m.
Message ID <1294065273-30274-3-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/77276/
State New
Headers show

Comments

Aurelien Jarno - Jan. 3, 2011, 2:34 p.m.
On targets that define sNaN with the sNaN bit as one, simply clearing
this bit may correspond to an infinite value.

Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
When other CPU of this type are added, this might be updated to include
more cases.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Peter Maydell - Jan. 3, 2011, 3:15 p.m.
On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On targets that define sNaN with the sNaN bit as one, simply clearing
> this bit may correspond to an infinite value.
>
> Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
> the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
> When other CPU of this type are added, this might be updated to include
> more cases.

This patch doesn't apply to master:

> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  fpu/softfloat-specialize.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index f23bd6a..31481e7 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -107,13 +107,13 @@ int float32_is_signaling_nan( float32 a_ )
>  float32 float32_maybe_silence_nan( float32 a_ )
>  {
>     if (float32_is_signaling_nan(a_)) {
> -        bits32 a = float32_val(a_);

...on master this line is
        uint32_t a = float32_val(a_);

(different type) so the patch doesn't apply.

Other than that, looks OK. I think I'd like a comment somewhere
along the lines of
/* Rules for silencing a signaling NaN are target-specific. Typically
 * targets with !SNAN_BIT_IS_ONE use the rule that the NaN
 * is silenced by setting the bit. Targets where SNAN_BIT_IS_ONE
 * must do something more complicated, because clearing the
 * bit might turn a NaN into an infinity. This code is correct for
 * MIPS but new targets might need something different.
 */

Or you could have the #ifdefs be on TARGET_whatever so
that it's clear (because it won't compile) that adding a new
TARGET_FOO means you have to check behaviour in this
area. But I don't feel very strongly about that.

-- PMM
Aurelien Jarno - Jan. 3, 2011, 3:24 p.m.
Peter Maydell a écrit :
> On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> On targets that define sNaN with the sNaN bit as one, simply clearing
>> this bit may correspond to an infinite value.
>>
>> Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
>> the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
>> When other CPU of this type are added, this might be updated to include
>> more cases.
> 
> This patch doesn't apply to master:
> 
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  fpu/softfloat-specialize.h |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> index f23bd6a..31481e7 100644
>> --- a/fpu/softfloat-specialize.h
>> +++ b/fpu/softfloat-specialize.h
>> @@ -107,13 +107,13 @@ int float32_is_signaling_nan( float32 a_ )
>>  float32 float32_maybe_silence_nan( float32 a_ )
>>  {
>>     if (float32_is_signaling_nan(a_)) {
>> -        bits32 a = float32_val(a_);
> 
> ...on master this line is
>         uint32_t a = float32_val(a_);
> 
> (different type) so the patch doesn't apply.

Oops, yes, my patch series should have started by a patch fixing types,
but i made a mistake selecting the commits to send. Will fix that in a v2.

> Other than that, looks OK. I think I'd like a comment somewhere
> along the lines of
> /* Rules for silencing a signaling NaN are target-specific. Typically
>  * targets with !SNAN_BIT_IS_ONE use the rule that the NaN
>  * is silenced by setting the bit. Targets where SNAN_BIT_IS_ONE
>  * must do something more complicated, because clearing the
>  * bit might turn a NaN into an infinity. This code is correct for
>  * MIPS but new targets might need something different.
>  */
> 
> Or you could have the #ifdefs be on TARGET_whatever so
> that it's clear (because it won't compile) that adding a new
> TARGET_FOO means you have to check behaviour in this
> area. But I don't feel very strongly about that.
> 

Ok, thanks for the review, will fix that.

Patch

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index f23bd6a..31481e7 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -107,13 +107,13 @@  int float32_is_signaling_nan( float32 a_ )
 float32 float32_maybe_silence_nan( float32 a_ )
 {
     if (float32_is_signaling_nan(a_)) {
-        bits32 a = float32_val(a_);
 #if SNAN_BIT_IS_ONE
-        a &= ~(1 << 22);
+        return float32_default_nan;
 #else
+        bits32 a = float32_val(a_);
         a |= (1 << 22);
-#endif
         return make_float32(a);
+#endif
     }
     return a_;
 }
@@ -321,13 +321,13 @@  int float64_is_signaling_nan( float64 a_ )
 float64 float64_maybe_silence_nan( float64 a_ )
 {
     if (float64_is_signaling_nan(a_)) {
-        bits64 a = float64_val(a_);
 #if SNAN_BIT_IS_ONE
-        a &= ~LIT64( 0x0008000000000000 );
+        return float64_default_nan;
 #else
+        bits64 a = float64_val(a_);
         a |= LIT64( 0x0008000000000000 );
-#endif
         return make_float64(a);
+#endif
     }
     return a_;
 }