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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Fri, Feb 03, 2017 at 03:39:16PM +0000, Peter Maydell wrote:
> On 3 February 2017 at 15:12, Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > On Fri, Feb 03, 2017 at 02:40:09PM +0000, Peter Maydell wrote:
> >> On 1 February 2017 at 10:49, Bharata B Rao <bharata@linux.vnet.ibm.com> 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 <bharata@linux.vnet.ibm.com>
> >> > ---
> >> >  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).

I checked the original berkeley implementation and I see that
float128_to_uint64 implementation there also is based on
float128_to_int64 implementation with edge cases being different.

To the best of my understanding, the corner cases for unsigned
int are covered in the above implemenation. Could you please
take a re-look at this ?

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

Sure, once we have the correct float128_to_uint64(), I will use that
to implement float64_to_uint32().

Regards,
Bharata.