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

[Tom Musta]

On 12/19/2013 4:11 PM, Peter Maydell wrote:
> 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
> 

Peter:

I agree with the comments and also with the bug.  I will fix.

I do test like you said ... random patterns with some biasing to try to get 
into corner
cases.  The bug you found was masked in the PowerPC code that wrapped to call to
float64_to_uint64.  I have constructed a variant of my test harness that invokes
float64_to_uint64 directly and it has uncovered the bug.