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

[Mateja Marjanovic]

On 18.3.19. 19:55, Aleksandar Markovic wrote:
> Hi, Mateja,
>
> > From: Mateja Marjanovic <address@hidden>
> > Subject: [PATCH] target/mips: Fix minor bug in FPU
> >
> > From: Mateja Marjanovic <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >   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