Re: [Qemu-ppc] [RFC PATCH v0] softfloat: Add float128_to_uint64_round_to_zero()

[Peter Maydell]

On 3 February 2017 at 15:12, Bharata B Rao <address@hidden> wrote:
> On Fri, Feb 03, 2017 at 02:40:09PM +0000, Peter Maydell wrote:
>> On 1 February 2017 at 10:49, Bharata B Rao <address@hidden> wrote:
>> > Implement float128_to_uint64() and use that to implement
>> > float128_to_uint64_round_to_zero()
>> >
>> > This is required by xscvqpudz instruction of PowerPC ISA 3.0.
>> >
>> > Signed-off-by: Bharata B Rao <address@hidden>
>> > ---
>> >  fpu/softfloat.c         | 65 
>> > +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/fpu/softfloat.h |  2 ++
>> >  2 files changed, 67 insertions(+)
>> >
>> > diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> > index c295f31..49a06c5 100644
>> > --- a/fpu/softfloat.c
>> > +++ b/fpu/softfloat.c
>> > @@ -6110,6 +6110,71 @@ int64_t float128_to_int64_round_to_zero(float128 a, 
>> > float_status *status)
>> >
>> >  
>> > /*----------------------------------------------------------------------------
>> >  | Returns the result of converting the quadruple-precision floating-point
>> > +| value `a' to the 64-bit unsigned integer format.  The conversion
>> > +| is performed according to the IEC/IEEE Standard for Binary 
>> > Floating-Point
>> > +| Arithmetic---which means in particular that the conversion is rounded
>> > +| according to the current rounding mode.  If `a' is a NaN, the largest
>> > +| positive integer is returned.  Otherwise, if the conversion overflows, 
>> > the
>> > +| largest unsigned integer is returned. If 'a' is negative, the value is
>> > +| rounded and zero is returned; negative values that do not round to zero
>> > +| will raise the inexact exception.
>> > +*----------------------------------------------------------------------------*/
>> > +
>> > +uint64_t float128_to_uint64(float128 a, float_status *status)
>> > +{
>> > +    flag aSign;
>> > +    int32_t aExp, shiftCount;
>> > +    uint64_t aSig0, aSig1;
>>
>> I think we should have a float128_squash_input_denormal() function
>> which we call here (compare float64_to_uint64).
>
> I followed float128_to_int64() which doesn't have that _denormal() call.

Ah, I see. I think we've got away without a float128_squash_input_denormal
because the set of targets that use float128 and the set that want
flush_inputs_to_zero happen to be disjoint. Since none of the other
float128 functions do denormal-squashing you're right that we shouldn't
add it in this patch.

>>
>> > +
>> > +    aSig1 = extractFloat128Frac1( a );
>>
>> Can you use the QEMU coding style for this rather than following
>> the softfloat weird one, please?
>
> Sure, I will henceforth switch to QEMU coding style, I was under the
> impression that we should stick to the existing style since almost
> entire softfloat.c is in different style.

Generally we go for "convert bits we touch" for most of the
codebase. softfloat.c has a lot of the old style still because we
haven't needed to touch it very much mostly.

>>
>> > +    aSig0 = extractFloat128Frac0( a );
>> > +    aExp = extractFloat128Exp( a );
>> > +    aSign = extractFloat128Sign( a );
>> > +    if ( aExp ) aSig0 |= LIT64( 0x0001000000000000 );
>> > +    shiftCount = 0x402F - aExp;
>> > +    if ( shiftCount <= 0 ) {
>> > +        if ( 0x403E < aExp ) {
>> > +            float_raise(float_flag_invalid, status);
>> > +            if (    ! aSign
>> > +                 || (    ( aExp == 0x7FFF )
>> > +                      && ( aSig1 || ( aSig0 != LIT64( 0x0001000000000000 
>> > ) ) )
>> > +                    )
>> > +               ) {
>> > +                return LIT64( 0xFFFFFFFFFFFFFFFF );
>> > +            }
>> > +            return 0;
>> > +        }
>> > +        shortShift128Left( aSig0, aSig1, - shiftCount, &aSig0, &aSig1 );
>> > +    }
>> > +    else {
>> > +        shift64ExtraRightJamming( aSig0, aSig1, shiftCount, &aSig0, 
>> > &aSig1 );
>> > +    }
>> > +    return roundAndPackUint64(aSign, aSig0, aSig1, status);
>>
>> I'm finding this a bit difficult to understand, because it doesn't
>> follow the code pattern of (for instance) float64_to_uint64().
>> Is it based on some other existing function?
>
> As I said above, it is based on float128_to_int64()

Ah, right. I think that's probably a bad model to copy because
it's a conversion to signed integer, not a conversion to
unsigned integer (so the edge cases are different).

> Actually what I really need is float128_to_uint64_round_to_zero().
>
> However it is a bit confusing as to which existing routine to follow here.
> I see there are 3 different ways floatXX_to_uintYY_round_to_zero is done:
>
> 1. Eg float64_to_uint32_round_to_zero()
>    Uses float64_to_uint64_round_to_zero()
>
> 2. Eg float128_to_int32_round_to_zero()
>    Doesn't use float128_to_int32() but instead is implemented
>    fully separately.
>
> 3. Eg float64_to_uint64_round_to_zero()
>    Sets the rounding mode to round-to-zero
>    Uses float64_to_uint64()
>
> I don't know if the above variants came about during different points
> in time or they are actually implemented that way due to some
> subtlety involved. I am following the 3rd pattern above as
> I found it to be easier for this particular case (float128_to_uint128)

They're mostly different for historical accident. Typically the
original softfloat code implemented conversion functions directly.
When we've added others later we've tended to use one of the
other functions where we can, because it's much easier to review
and be confident that an implementation like that is correct than
one which does direct manipulation of the various fields of the float.

> In fact I need float128_to_uint32() also next, but haven't yet been
> able to figure out which way to do it.

I would do this via float128_to_uint64(), the same way that
float64_to_uint32() does.

thanks
-- PMM