[Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU

Mateja Marjanovic posted 1 patch 5 years ago
Test docker-clang@ubuntu passed
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1552933587-20810-2-git-send-email-mateja.marjanovic@rt-rk.com
Maintainers: Aleksandar Rikalo <arikalo@wavecomp.com>, Aleksandar Markovic <amarkovic@wavecomp.com>, Peter Maydell <peter.maydell@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, "Alex Bennée" <alex.bennee@linaro.org>
There is a newer version of this series
fpu/softfloat-specialize.h | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Mateja Marjanovic 5 years ago
From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

Wrong type of NaN was generated for IEEE754-2008 by maddf and
msubf insturctions when the arguments were inf, zero, nan or
zero, inf, nan respectively.

Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
---
 fpu/softfloat-specialize.h | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 16c0bcb..56256be 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -495,15 +495,15 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
         return 1;
     }
 #elif defined(TARGET_MIPS)
-    /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
-     * the default NaN
-     */
-    if (infzero) {
-        float_raise(float_flag_invalid, status);
-        return 3;
-    }
-
     if (snan_bit_is_one(status)) {
+        /*
+         * For MIPS systems that conform to IEEE754-1985, the (inf,zero,qnan)
+         * case sets InvalidOp and returns the default NaN
+         */
+        if (infzero) {
+            float_raise(float_flag_invalid, status);
+            return 3;
+        }
         /* Prefer sNaN over qNaN, in the a, b, c order. */
         if (is_snan(a_cls)) {
             return 0;
@@ -519,6 +519,14 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
             return 2;
         }
     } else {
+        /*
+         * For MIPS systems that conform to IEEE754-2008, the (inf,zero,qnan)
+         * case sets InvalidOp and returns the default NaN
+         */
+        if (infzero) {
+            float_raise(float_flag_invalid, status);
+            return 2;
+        }
         /* Prefer sNaN over qNaN, in the c, a, b order. */
         if (is_snan(c_cls)) {
             return 2;
-- 
2.7.4


Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Peter Maydell 5 years ago
On Mon, 18 Mar 2019 at 18:26, Mateja Marjanovic
<mateja.marjanovic@rt-rk.com> wrote:
>
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> Wrong type of NaN was generated for IEEE754-2008 by maddf and
> msubf insturctions when the arguments were inf, zero, nan or
> zero, inf, nan respectively.
>
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>

> @@ -519,6 +519,14 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
>              return 2;
>          }
>      } else {
> +        /*
> +         * For MIPS systems that conform to IEEE754-2008, the (inf,zero,qnan)
> +         * case sets InvalidOp and returns the default NaN
> +         */
> +        if (infzero) {
> +            float_raise(float_flag_invalid, status);
> +            return 2;

The comment says we return the default NaN, but the code says
we return the input NaN (ie the input value 'c'). Which is correct?

> +        }
>          /* Prefer sNaN over qNaN, in the c, a, b order. */
>          if (is_snan(c_cls)) {
>              return 2;

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Mateja Marjanovic 5 years ago
On 18.3.19. 19:30, Peter Maydell wrote:
> On Mon, 18 Mar 2019 at 18:26, Mateja Marjanovic
> <mateja.marjanovic@rt-rk.com> wrote:
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Wrong type of NaN was generated for IEEE754-2008 by maddf and
>> msubf insturctions when the arguments were inf, zero, nan or
>> zero, inf, nan respectively.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> @@ -519,6 +519,14 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
>>               return 2;
>>           }
>>       } else {
>> +        /*
>> +         * For MIPS systems that conform to IEEE754-2008, the (inf,zero,qnan)
>> +         * case sets InvalidOp and returns the default NaN
>> +         */
>> +        if (infzero) {
>> +            float_raise(float_flag_invalid, status);
>> +            return 2;
> The comment says we return the default NaN, but the code says
> we return the input NaN (ie the input value 'c'). Which is correct?
It should be the input value 'c', I wrote that by mistake. It will be 
corrected in v3.
>> +        }
>>           /* Prefer sNaN over qNaN, in the c, a, b order. */
>>           if (is_snan(c_cls)) {
>>               return 2;
> thanks
> -- PMM
Thanks for noticing.
Regards,
Mateja

Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Aleksandar Markovic 5 years ago
Hi, Mateja,

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH] target/mips: Fix minor bug in FPU
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Wrong type of NaN was generated for IEEE754-2008 by maddf and
> msubf insturctions when the arguments were inf, zero, nan or
> zero, inf, nan respectively.
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---
>  fpu/softfloat-specialize.h | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 16c0bcb..56256be 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -495,15 +495,15 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, > FloatClass c_cls,
>          return 1;
>      }
>  #elif defined(TARGET_MIPS)
> -    /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
> -     * the default NaN
> -     */
> -    if (infzero) {
> -        float_raise(float_flag_invalid, status);
> -        return 3;
> -    }
> -
>      if (snan_bit_is_one(status)) {
> +        /*
> +         * For MIPS systems that conform to IEEE754-1985, the (inf,zero,qnan)

"IEEE754-1985" should be "IEEE 754-1985".

"(inf,zero,qnan)" should be "(inf,zero,nan)" (the "c" can be both kinds of NaN at
the moment of execution of this block, as it is visible from the code surrounding
invocations of pickNaNMulAdd()).

> +         * case sets InvalidOp and returns the default NaN
> +         */
> +        if (infzero) {
> +            float_raise(float_flag_invalid, status);
> +            return 3;
> +        }
>          /* Prefer sNaN over qNaN, in the a, b, c order. */
>          if (is_snan(a_cls)) {
>              return 0;
> @@ -519,6 +519,14 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, > FloatClass c_cls,
>              return 2;
>          }
>      } else {
> +        /*
> +         * For MIPS systems that conform to IEEE754-2008, the (inf,zero,qnan)

A similar comment applies, like for the block above.

> +         * case sets InvalidOp and returns the default NaN

This line is incorrect, and, instead of "the default NaN", there should be "c".

> +         */
> +        if (infzero) {
> +            float_raise(float_flag_invalid, status);
> +            return 2;
> +        }
>          /* Prefer sNaN over qNaN, in the c, a, b order. */
>          if (is_snan(c_cls)) {
>              return 2;
> --
> 2.7.4

Reference to the justification in documentation in the commit message would be
useful and nice.

Thanks,
Aleksandar
Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Mateja Marjanovic 5 years ago
On 18.3.19. 19:55, Aleksandar Markovic wrote:
> Hi, Mateja,
>
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH] target/mips: Fix minor bug in FPU
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Wrong type of NaN was generated for IEEE754-2008 by maddf and
>> msubf insturctions when the arguments were inf, zero, nan or
>> zero, inf, nan respectively.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> ---
>>   fpu/softfloat-specialize.h | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> index 16c0bcb..56256be 100644
>> --- a/fpu/softfloat-specialize.h
>> +++ b/fpu/softfloat-specialize.h
>> @@ -495,15 +495,15 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, > FloatClass c_cls,
>>           return 1;
>>       }
>>   #elif defined(TARGET_MIPS)
>> -    /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
>> -     * the default NaN
>> -     */
>> -    if (infzero) {
>> -        float_raise(float_flag_invalid, status);
>> -        return 3;
>> -    }
>> -
>>       if (snan_bit_is_one(status)) {
>> +        /*
>> +         * For MIPS systems that conform to IEEE754-1985, the (inf,zero,qnan)
> "IEEE754-1985" should be "IEEE 754-1985".
>
> "(inf,zero,qnan)" should be "(inf,zero,nan)" (the "c" can be both kinds of NaN at
> the moment of execution of this block, as it is visible from the code surrounding
> invocations of pickNaNMulAdd()).
It was a copy-paste error. In the previous version I thought the if-else 
statement reffers to the snan and qnan.
Later I found out it actually checks if it is IEEE754-1985 or 
IEEE754-2008. It will be corrected as soon as possible.
>
>> +         * case sets InvalidOp and returns the default NaN
>> +         */
>> +        if (infzero) {
>> +            float_raise(float_flag_invalid, status);
>> +            return 3;
>> +        }
>>           /* Prefer sNaN over qNaN, in the a, b, c order. */
>>           if (is_snan(a_cls)) {
>>               return 0;
>> @@ -519,6 +519,14 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, > FloatClass c_cls,
>>               return 2;
>>           }
>>       } else {
>> +        /*
>> +         * For MIPS systems that conform to IEEE754-2008, the (inf,zero,qnan)
> A similar comment applies, like for the block above.
Same goes for this.
>
>> +         * case sets InvalidOp and returns the default NaN
> This line is incorrect, and, instead of "the default NaN", there should be "c".
Yes, I noticed that, Peter already wrote me, but thanks anyways.
>
>> +         */
>> +        if (infzero) {
>> +            float_raise(float_flag_invalid, status);
>> +            return 2;
>> +        }
>>           /* Prefer sNaN over qNaN, in the c, a, b order. */
>>           if (is_snan(c_cls)) {
>>               return 2;
>> --
>> 2.7.4
> Reference to the justification in documentation in the commit message would be
> useful and nice.
I agree, it will be added.
>
> Thanks,
> Aleksandar
Thanks,
Mateja

Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Aleksandar Markovic 5 years ago
> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH] target/mips: Fix minor bug in FPU
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Wrong type of NaN was generated for IEEE754-2008 by maddf and
> msubf insturctions when the arguments were inf, zero, nan or
> zero, inf, nan respectively.
> 

"insturctions" -> "instructions".

The word "respectively" is here wrong and spurious.

The meaning that you convey with the current wording is that
the "inf, zero, nan" case is related to "maddf" (only), and the
"zero, inf, nan" to "msubf".

Please use full instruction names with possible Backus-Naur
shorthands, like "MADDF.<D|S>", "MSUBF.<D|S>".

Thanks,
Aleksandar
Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Mateja Marjanovic 5 years ago
On 18.3.19. 20:08, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH] target/mips: Fix minor bug in FPU
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Wrong type of NaN was generated for IEEE754-2008 by maddf and
>> msubf insturctions when the arguments were inf, zero, nan or
>> zero, inf, nan respectively.
>>
> "insturctions" -> "instructions".
Typo, I will correct that.
>
> The word "respectively" is here wrong and spurious.
>
> The meaning that you convey with the current wording is that
> the "inf, zero, nan" case is related to "maddf" (only), and the
> "zero, inf, nan" to "msubf".
Could you tell me what do you suggest?

Does "Wrong type of NaN was generated for IEEE754-2008 by maddf and msubf

insturctions when the arguments were (inf, zero, nan) or

(zero, inf, nan)." seem ok?

>
> Please use full instruction names with possible Backus-Naur
> shorthands, like "MADDF.<D|S>", "MSUBF.<D|S>".
Ok, I will use that from now on.
>
> Thanks,
> Aleksandar
Thanks,
Mateja

Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Aleksandar Markovic 5 years ago
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
> 
> 
> On 18.3.19. 20:08, Aleksandar Markovic wrote:
> >> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> >> Subject: [PATCH] target/mips: Fix minor bug in FPU
> >>
> >> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> >>
> >> Wrong type of NaN was generated for IEEE754-2008 by maddf and
> >> msubf insturctions when the arguments were inf, zero, nan or
> >> zero, inf, nan respectively.
> >>
> > "insturctions" -> "instructions".
> Typo, I will correct that.
> >
> > The word "respectively" is here wrong and spurious.
> >
> > The meaning that you convey with the current wording is that
> > the "inf, zero, nan" case is related to "maddf" (only), and the
> > "zero, inf, nan" to "msubf".
> Could you tell me what do you suggest?
> 
> Does "Wrong type of NaN was generated for IEEE754-2008 by maddf and msubf
> 
> insturctions when the arguments were (inf, zero, nan) or
> 
> (zero, inf, nan)." seem ok?

I'm afraid it doesn't, it contains several errors that I already
objected to. I suggest that you read the comments thoroughly.

Aleksandar

> 
> >
> > Please use full instruction names with possible Backus-Naur
> > shorthands, like "MADDF.<D|S>", "MSUBF.<D|S>".
> Ok, I will use that from now on.
> >
> > Thanks,
> > Aleksandar
> Thanks,
> Mateja
> 
Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Aleksandar Markovic 5 years ago
> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH] target/mips: Fix minor bug in FPU

"v2" is missing in the subject line, and now v1 and v2 are incorrectly
linked on mailing list website.

An isolated patch is perhaps better sent without cover letter - 
especially since your cover letter doesn't add any information
that is not it the commit message (they are actually identical).

What is missing is also the history of changes.

Please read:

https://wiki.qemu.org/Contribute/SubmitAPatch

and

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297

carefully.

Thanks,
Aleksandar
Re: [Qemu-devel] [PATCH] target/mips: Fix minor bug in FPU
Posted by Mateja Marjanovic 5 years ago
On 18.3.19. 20:42, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH] target/mips: Fix minor bug in FPU
> "v2" is missing in the subject line, and now v1 and v2 are incorrectly
> linked on mailing list website.
I accidentally forgot to add v2 in the subject line.
>
> An isolated patch is perhaps better sent without cover letter -
> especially since your cover letter doesn't add any information
> that is not it the commit message (they are actually identical).
I didn't know it could be without the cover letter, I won't make that 
mistake again.
>
> What is missing is also the history of changes.
>
> Please read:
>
> https://wiki.qemu.org/Contribute/SubmitAPatch
>
> and
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
>
> carefully.
I will read them, thanks.
>
> Thanks,
> Aleksandar
Thanks,
Mateja