Re: [Qemu-devel] [V4 PATCH 01/22] softfloat: Fix float64_to_uint64

[Peter Maydell]

On 18 December 2013 20:19, Tom Musta <address@hidden> wrote:
> The comment preceding the float64_to_uint64 routine suggests that
> the implementation is broken.  And this is, indeed, the case.
>
> This patch properly implements the conversion of a 64-bit floating
> point number to an unsigned, 64 bit integer.
>
> This contribution can be licensed under either the softfloat-2a or -2b
> license.
>
> V2: Added softfloat license statement.
>
> V3: Modified to meet QEMU coding conventions.
>
> V4: Fixed incorrect handling of small negatives, which, if rounded
> up to zero should not set the inexact flag.
>
> Signed-off-by: Tom Musta <address@hidden>
> ---
>  fpu/softfloat.c |   98 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 89 insertions(+), 9 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index dbda61b..ec23908 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -161,7 +161,6 @@ static int32 roundAndPackInt32( flag zSign, uint64_t absZ 
> STATUS_PARAM)
>  | exception is raised and the largest positive or negative integer is
>  | returned.
>  
> *----------------------------------------------------------------------------*/
> -
>  static int64 roundAndPackInt64( flag zSign, uint64_t absZ0, uint64_t absZ1 
> STATUS_PARAM)
>  {
>      int8 roundingMode;
> @@ -204,6 +203,56 @@ static int64 roundAndPackInt64( flag zSign, uint64_t 
> absZ0, uint64_t absZ1 STATU
>  }
>
>  
> /*----------------------------------------------------------------------------
> +| Takes the 128-bit fixed-point value formed by concatenating `absZ0' and
> +| `absZ1', with binary point between bits 63 and 64 (between the input 
> words),
> +| and returns the properly rounded 64-bit unsigned integer corresponding to 
> the
> +| input.  Ordinarily, the fixed-point input is simply rounded to an integer,
> +| with the inexact exception raised if the input cannot be represented 
> exactly
> +| as an integer.  However, if the fixed-point input is too large, the invalid
> +| exception is raised and the largest unsigned integer is returned.
> +*----------------------------------------------------------------------------*/

You should probably say in this comment what the behaviour is for
negative inputs.

> +uint64_t float64_to_uint64(float64 a STATUS_PARAM)
> +{
> +    flag aSign;
> +    int_fast16_t aExp, shiftCount;
> +    uint64_t aSig, aSigExtra;
> +    a = float64_squash_input_denormal(a STATUS_VAR);
>
> -    return v - INT64_MIN;
> +    aSig = extractFloat64Frac(a);
> +    aExp = extractFloat64Exp(a);
> +    aSign = extractFloat64Sign(a);
> +    if (aSign && (aExp > 1022)) {
> +        float_raise(float_flag_invalid STATUS_VAR);
> +        return 0;

This incorrectly returns 0 rather than largest-positive-integer
for NaNs with the sign bit set.

> +    }
> +    if (aExp) {
> +        aSig |= LIT64(0x0010000000000000);
> +    }
> +    shiftCount = 0x433 - aExp;
> +    if (shiftCount <= 0) {
> +        if (0x43E < aExp) {
> +            float_raise(float_flag_invalid STATUS_VAR);
> +            return LIT64(0xFFFFFFFFFFFFFFFF);
> +        }
> +        aSigExtra = 0;
> +        aSig <<= -shiftCount;
> +    } else {
> +        shift64ExtraRightJamming(aSig, 0, shiftCount, &aSig, &aSigExtra);
> +    }
> +    return roundAndPackUint64(aSign, aSig, aSigExtra STATUS_VAR);
>  }

Other than that, the code *looks* OK, but it's really easy for
"not quite right" code to slip through here (especially on corner
cases like NaNs, denormals and odd rounding modes). How much
testing have you given this? I really recommend testing by firing a
huge pile of random (and semi random) test vectors at whatever
guest instruction you're implementing and comparing against
results on reference hardware.

thanks
-- PMM