[v1] target/mips: Fix minor bug in FPU

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

Posted by Mateja Marjanovic 6 years, 8 months ago

From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

Wrong type of NaN was generated 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 16c0bcb..647bfbc 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -500,7 +500,7 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
      */
     if (infzero) {
         float_raise(float_flag_invalid, status);
-        return 3;
+        return 2;
     }
 
     if (snan_bit_is_one(status)) {
-- 
2.7.4

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

Posted by Alex Bennée 6 years, 8 months ago

Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:

> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> Wrong type of NaN was generated 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 16c0bcb..647bfbc 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -500,7 +500,7 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
>       */
>      if (infzero) {
>          float_raise(float_flag_invalid, status);
> -        return 3;
> +        return 2;

Hi,

This changes the behaviour documented above which says:

    /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
     * the default NaN
     */

So if the behaviour is incorrect please update the comment as well.
Bonus points for a reference to the canonical reference document that
describes this.

--
Alex Bennée

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

Posted by Aleksandar Markovic 6 years, 8 months ago

> From: Alex Bennée <alex.bennee@linaro.org>
> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>
> Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:
> 
> > From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> >
> > Wrong type of NaN was generated 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> > index 16c0bcb..647bfbc 100644
> > --- a/fpu/softfloat-specialize.h
> > +++ b/fpu/softfloat-specialize.h
> > @@ -500,7 +500,7 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
> >       */
> >      if (infzero) {
> >          float_raise(float_flag_invalid, status);
> > -        return 3;
> > +        return 2;
> 
> Hi,
> 
> This changes the behaviour documented above which says:
> 
>     /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
>      * the default NaN
>      */
> 
> So if the behaviour is incorrect please update the comment as well.
> Bonus points for a reference to the canonical reference document that
> describes this.
> 

Alex,

I tested this case (0*inf+nan fused multiply-add/multiply-subtract) on a MIPS
hardware system, and this patch is right - after the patch QEMU and hardware
(MIPS64R6 board) behaviors match. The comment you mentioned probably
just reflects the code, it looks not to be the reference source of information.
If the patch is accepted, that comments must be changed in the same patch.

This is a MIPS-only-specific change (under "#if defined (TARGET_MIPS)"), and,
from your point of view, could this be included in MIPS pull request (if the validity
of the patch is confirmed)?

Sincerely,
Aleksandar

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

Posted by Alex Bennée 6 years, 8 months ago

Aleksandar Markovic <amarkovic@wavecomp.com> writes:

>> From: Alex Bennée <alex.bennee@linaro.org>
>> Subject: Re: [PATCH] target/mips: Fix minor bug in FPU
>>
>> Mateja Marjanovic <mateja.marjanovic@rt-rk.com> writes:
>>
>> > From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>> >
>> > Wrong type of NaN was generated 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 | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> > index 16c0bcb..647bfbc 100644
>> > --- a/fpu/softfloat-specialize.h
>> > +++ b/fpu/softfloat-specialize.h
>> > @@ -500,7 +500,7 @@ static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
>> >       */
>> >      if (infzero) {
>> >          float_raise(float_flag_invalid, status);
>> > -        return 3;
>> > +        return 2;
>>
>> Hi,
>>
>> This changes the behaviour documented above which says:
>>
>>     /* For MIPS, the (inf,zero,qnan) case sets InvalidOp and returns
>>      * the default NaN
>>      */
>>
>> So if the behaviour is incorrect please update the comment as well.
>> Bonus points for a reference to the canonical reference document that
>> describes this.
>>
>
> Alex,
>
> I tested this case (0*inf+nan fused multiply-add/multiply-subtract) on a MIPS
> hardware system, and this patch is right - after the patch QEMU and hardware
> (MIPS64R6 board) behaviors match. The comment you mentioned probably
> just reflects the code, it looks not to be the reference source of information.
> If the patch is accepted, that comments must be changed in the same
> patch.

I'm perfectly happy to take your word the fix is fine for MIPS - as you
say this is part of the rich tapestry of IMPDEF variations on NaN
handling ;-)

I did have a brief browse through the MIPS Architecture for Programmers
(Document Number: MD00083) but couldn't find a decent line about the NaN
propagation but if it's somewhere else it would be worth mentioning in
the comment.

>
> This is a MIPS-only-specific change (under "#if defined (TARGET_MIPS)"), and,
> from your point of view, could this be included in MIPS pull request (if the validity
> of the patch is confirmed)?

I'd be happy for you to included it (with the matching comment change)
in your next PR:

Acked-by: Alex Bennée <alex.bennee@linaro.org>

>
> Sincerely,
> Aleksandar


--
Alex Bennée