[RFC,v0] softfloat: Add float128_to_uint64_round_to_zero()

Commit Message

Bharata B Rao Feb. 1, 2017, 10:49 a.m. UTC

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 <[e-mail]>
---
 fpu/softfloat.c         | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/fpu/softfloat.h |  2 ++
 2 files changed, 67 insertions(+)

Comments

[e-mail] Feb. 1, 2017, 10:53 a.m. UTC | #1

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [RFC PATCH v0] softfloat: Add float128_to_uint64_round_to_zero()
Message-id: [e-mail]

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
Switched to a new branch 'test'
7d6d31a softfloat: Add float128_to_uint64_round_to_zero()

=== OUTPUT BEGIN ===
Checking PATCH 1/1: softfloat: Add float128_to_uint64_round_to_zero()...
ERROR: space prohibited after that open parenthesis '('
#38: FILE: fpu/softfloat.c:6129:
+    aSig1 = extractFloat128Frac1( a );

ERROR: space prohibited before that close parenthesis ')'
#38: FILE: fpu/softfloat.c:6129:
+    aSig1 = extractFloat128Frac1( a );

ERROR: space prohibited after that open parenthesis '('
#39: FILE: fpu/softfloat.c:6130:
+    aSig0 = extractFloat128Frac0( a );

ERROR: space prohibited before that close parenthesis ')'
#39: FILE: fpu/softfloat.c:6130:
+    aSig0 = extractFloat128Frac0( a );

ERROR: space prohibited after that open parenthesis '('
#40: FILE: fpu/softfloat.c:6131:
+    aExp = extractFloat128Exp( a );

ERROR: space prohibited before that close parenthesis ')'
#40: FILE: fpu/softfloat.c:6131:
+    aExp = extractFloat128Exp( a );

ERROR: space prohibited after that open parenthesis '('
#41: FILE: fpu/softfloat.c:6132:
+    aSign = extractFloat128Sign( a );

ERROR: space prohibited before that close parenthesis ')'
#41: FILE: fpu/softfloat.c:6132:
+    aSign = extractFloat128Sign( a );

ERROR: space prohibited after that open parenthesis '('
#42: FILE: fpu/softfloat.c:6133:
+    if ( aExp ) aSig0 |= LIT64( 0x0001000000000000 );

ERROR: space prohibited before that close parenthesis ')'
#42: FILE: fpu/softfloat.c:6133:
+    if ( aExp ) aSig0 |= LIT64( 0x0001000000000000 );

ERROR: trailing statements should be on next line
#42: FILE: fpu/softfloat.c:6133:
+    if ( aExp ) aSig0 |= LIT64( 0x0001000000000000 );

ERROR: braces {} are necessary for all arms of this statement
#42: FILE: fpu/softfloat.c:6133:
+    if ( aExp ) aSig0 |= LIT64( 0x0001000000000000 );
[...]

ERROR: space prohibited after that open parenthesis '('
#44: FILE: fpu/softfloat.c:6135:
+    if ( shiftCount <= 0 ) {

ERROR: space prohibited before that close parenthesis ')'
#44: FILE: fpu/softfloat.c:6135:
+    if ( shiftCount <= 0 ) {

ERROR: space prohibited after that open parenthesis '('
#45: FILE: fpu/softfloat.c:6136:
+        if ( 0x403E < aExp ) {

ERROR: space prohibited before that close parenthesis ')'
#45: FILE: fpu/softfloat.c:6136:
+        if ( 0x403E < aExp ) {

ERROR: space prohibited after that '!' (ctx:WxW)
#47: FILE: fpu/softfloat.c:6138:
+            if (    ! aSign
                     ^

ERROR: space prohibited after that open parenthesis '('
#47: FILE: fpu/softfloat.c:6138:
+            if (    ! aSign

ERROR: space prohibited after that open parenthesis '('
#48: FILE: fpu/softfloat.c:6139:
+                 || (    ( aExp == 0x7FFF )

ERROR: space prohibited before that close parenthesis ')'
#48: FILE: fpu/softfloat.c:6139:
+                 || (    ( aExp == 0x7FFF )

ERROR: space prohibited after that open parenthesis '('
#49: FILE: fpu/softfloat.c:6140:
+                      && ( aSig1 || ( aSig0 != LIT64( 0x0001000000000000 ) ) )

ERROR: space prohibited before that close parenthesis ')'
#49: FILE: fpu/softfloat.c:6140:
+                      && ( aSig1 || ( aSig0 != LIT64( 0x0001000000000000 ) ) )

ERROR: space prohibited after that open parenthesis '('
#52: FILE: fpu/softfloat.c:6143:
+                return LIT64( 0xFFFFFFFFFFFFFFFF );

ERROR: space prohibited before that close parenthesis ')'
#52: FILE: fpu/softfloat.c:6143:
+                return LIT64( 0xFFFFFFFFFFFFFFFF );

ERROR: space prohibited after that '-' (ctx:WxW)
#56: FILE: fpu/softfloat.c:6147:
+        shortShift128Left( aSig0, aSig1, - shiftCount, &aSig0, &aSig1 );
                                          ^

ERROR: space prohibited after that open parenthesis '('
#56: FILE: fpu/softfloat.c:6147:
+        shortShift128Left( aSig0, aSig1, - shiftCount, &aSig0, &aSig1 );

ERROR: space prohibited before that close parenthesis ')'
#56: FILE: fpu/softfloat.c:6147:
+        shortShift128Left( aSig0, aSig1, - shiftCount, &aSig0, &aSig1 );

ERROR: else should follow close brace '}'
#58: FILE: fpu/softfloat.c:6149:
+    }
+    else {

ERROR: space prohibited after that open parenthesis '('
#59: FILE: fpu/softfloat.c:6150:
+        shift64ExtraRightJamming( aSig0, aSig1, shiftCount, &aSig0, &aSig1 );

ERROR: space prohibited before that close parenthesis ')'
#59: FILE: fpu/softfloat.c:6150:
+        shift64ExtraRightJamming( aSig0, aSig1, shiftCount, &aSig0, &aSig1 );

total: 30 errors, 0 warnings, 79 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [e-mail]

Peter Maydell Feb. 3, 2017, 2:40 p.m. UTC | #2

On 1 February 2017 at 10:49, Bharata B Rao <[e-mail]> 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 <[e-mail]>
> ---
>  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).

> +
> +    aSig1 = extractFloat128Frac1( a );

Can you use the QEMU coding style for this rather than following
the softfloat weird one, please?

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

> +}
> +
> +/*----------------------------------------------------------------------------
> +| 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, except that the conversion is always rounded toward zero.
> +| 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_round_to_zero(float128 a, float_status *status)
> +{
> +    signed char current_rounding_mode = status->float_rounding_mode;
> +    set_float_rounding_mode(float_round_to_zero, status);
> +    uint64_t v = float128_to_uint64(a, status);
> +    set_float_rounding_mode(current_rounding_mode, status);
> +    return v;
> +}

This function is OK, though our coding style would suggest putting
the declaration of 'uint64_t v;' at the top of the function.

> +
> +/*----------------------------------------------------------------------------
> +| Returns the result of converting the quadruple-precision floating-point
>  | value `a' to the single-precision floating-point format.  The conversion
>  | is performed according to the IEC/IEEE Standard for Binary Floating-Point
>  | Arithmetic.
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index 842ec6b..4e99253 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -712,6 +712,8 @@ int32_t float128_to_int32(float128, float_status *status);
>  int32_t float128_to_int32_round_to_zero(float128, float_status *status);
>  int64_t float128_to_int64(float128, float_status *status);
>  int64_t float128_to_int64_round_to_zero(float128, float_status *status);
> +uint64_t float128_to_uint64(float128, float_status *status);
> +uint64_t float128_to_uint64_round_to_zero(float128, float_status *status);
>  float32 float128_to_float32(float128, float_status *status);
>  float64 float128_to_float64(float128, float_status *status);
>  floatx80 float128_to_floatx80(float128, float_status *status);
> --
> 2.7.4

thanks
-- PMM

Bharata B Rao Feb. 3, 2017, 3:12 p.m. UTC | #3

On Fri, Feb 03, 2017 at 02:40:09PM +0000, Peter Maydell wrote:
> On 1 February 2017 at 10:49, Bharata B Rao <[e-mail]> 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 <[e-mail]>
> > ---
> >  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.

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

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

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)

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

> 
> > +}
> > +
> > +/*----------------------------------------------------------------------------
> > +| 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, except that the conversion is always rounded toward zero.
> > +| 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_round_to_zero(float128 a, float_status *status)
> > +{
> > +    signed char current_rounding_mode = status->float_rounding_mode;
> > +    set_float_rounding_mode(float_round_to_zero, status);
> > +    uint64_t v = float128_to_uint64(a, status);
> > +    set_float_rounding_mode(current_rounding_mode, status);
> > +    return v;
> > +}
> 
> This function is OK, though our coding style would suggest putting
> the declaration of 'uint64_t v;' at the top of the function.

Sure will change in the next iteration when I switch to QEMU style.

Regards,
Bharata.

Peter Maydell Feb. 3, 2017, 3:39 p.m. UTC | #4

On 3 February 2017 at 15:12, Bharata B Rao <[e-mail]> wrote:
> On Fri, Feb 03, 2017 at 02:40:09PM +0000, Peter Maydell wrote:
>> On 1 February 2017 at 10:49, Bharata B Rao <[e-mail]> 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 <[e-mail]>
>> > ---
>> >  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

Bharata B Rao Feb. 6, 2017, 8:58 a.m. UTC | #5

On Fri, Feb 03, 2017 at 03:39:16PM +0000, Peter Maydell wrote:
> On 3 February 2017 at 15:12, Bharata B Rao <[e-mail]> wrote:
> > On Fri, Feb 03, 2017 at 02:40:09PM +0000, Peter Maydell wrote:
> >> On 1 February 2017 at 10:49, Bharata B Rao <[e-mail]> 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 <[e-mail]>
> >> > ---
> >> >  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.

Peter Maydell Feb. 6, 2017, 10:31 a.m. UTC | #6

On 6 February 2017 at 08:58, Bharata B Rao <[e-mail]> wrote:
> On Fri, Feb 03, 2017 at 03:39:16PM +0000, Peter Maydell wrote:
>> On 3 February 2017 at 15:12, Bharata B Rao <[e-mail]> wrote:
>> > 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 ?

OK, but that's about 5 times harder to review because I have
to work everything out from scratch rather than being able to
say "yes, this is doing everything the same way that our
existing known-to-be-good other function is doing it".

thanks
-- PMM

Bharata B Rao Feb. 6, 2017, 12:04 p.m. UTC | #7

On Mon, Feb 06, 2017 at 10:31:49AM +0000, Peter Maydell wrote:
> On 6 February 2017 at 08:58, Bharata B Rao <[e-mail]> wrote:
> > On Fri, Feb 03, 2017 at 03:39:16PM +0000, Peter Maydell wrote:
> >> On 3 February 2017 at 15:12, Bharata B Rao <[e-mail]> wrote:
> >> > 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 ?
> 
> OK, but that's about 5 times harder to review because I have
> to work everything out from scratch rather than being able to
> say "yes, this is doing everything the same way that our
> existing known-to-be-good other function is doing it".

Fair enough. I will come up with a version of
float128_to_uint64_round_to_zero() based on the existing
float64_to_uint32_round_to_zero().

Regards,
Bharata.

Patch

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;
+
+    aSig1 = extractFloat128Frac1( a );
+    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);
+}
+
+/*----------------------------------------------------------------------------
+| 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, except that the conversion is always rounded toward zero.
+| 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_round_to_zero(float128 a, float_status *status)
+{
+    signed char current_rounding_mode = status->float_rounding_mode;
+    set_float_rounding_mode(float_round_to_zero, status);
+    uint64_t v = float128_to_uint64(a, status);
+    set_float_rounding_mode(current_rounding_mode, status);
+    return v;
+}
+
+/*----------------------------------------------------------------------------
+| Returns the result of converting the quadruple-precision floating-point
 | value `a' to the single-precision floating-point format.  The conversion
 | is performed according to the IEC/IEEE Standard for Binary Floating-Point
 | Arithmetic.
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 842ec6b..4e99253 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -712,6 +712,8 @@  int32_t float128_to_int32(float128, float_status *status);
 int32_t float128_to_int32_round_to_zero(float128, float_status *status);
 int64_t float128_to_int64(float128, float_status *status);
 int64_t float128_to_int64_round_to_zero(float128, float_status *status);
+uint64_t float128_to_uint64(float128, float_status *status);
+uint64_t float128_to_uint64_round_to_zero(float128, float_status *status);
 float32 float128_to_float32(float128, float_status *status);
 float64 float128_to_float64(float128, float_status *status);
 floatx80 float128_to_floatx80(float128, float_status *status);