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

[Aleksandar Markovic]

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

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