Re: [Qemu-devel] [PATCH v4 16/22] fpu/softfloat: re-factor round_to_int

[Peter Maydell <peter.maydell@linaro.org>]

On 6 February 2018 at 16:48, Alex Bennée <alex.bennee@linaro.org> wrote:
> We can now add float16_round_to_int and use the common round_decomposed and
> canonicalize functions to have a single implementation for
> float16/32/64 round_to_int functions.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> ---
> v3
>   - rename structures and functions
> v4
>   - move NaN handling to return NaN
> ---
>  fpu/softfloat.c         | 322 ++++++++++++++++++++++--------------------------
>  include/fpu/softfloat.h |   1 +
>  2 files changed, 148 insertions(+), 175 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index ae4ba6de51..5d04e65538 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -560,7 +560,26 @@ static bool is_qnan(FloatClass c)
>      return c == float_class_qnan;
>  }
>
> -static FloatParts pick_nan(FloatParts a, FloatParts b, float_status *s)
> +static inline FloatParts return_nan(FloatParts a, float_status *s)
> +{
> +    switch (a.cls) {
> +    case float_class_snan:
> +        s->float_exception_flags |= float_flag_invalid;
> +        a.cls = float_class_msnan;
> +        /* FALLTHRU */

"/* fall through */" is the usual way of spelling it in QEMU
(255 instances vs 60).

> +    case float_class_qnan:
> +        if (s->default_nan_mode) {
> +            a.cls = float_class_dnan;
> +        }
> +        break;
> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +    return a;
> +}
> +
> +static inline FloatParts pick_nan(FloatParts a, FloatParts b, float_status *s)
>  {
>      if (is_snan(a.cls) || is_snan(b.cls)) {
>          s->float_exception_flags |= float_flag_invalid;

This hunk looks a bit weird because the patch is also adding
an "inline" qualifier to pick_nan(). Should that have been in an
earlier patch (eg "fpu/softfloat: re-factor add/sub" where the
pick_nan() function was added) ?

> @@ -1175,6 +1194,133 @@ float64 float64_div(float64 a, float64 b, float_status *status)
>      return float64_round_pack_canonical(pr, status);
>  }
>
> +/*
> + * Rounds the floating-point value `a' to an integer, and returns the
> + * result as a floating-point value. The operation is performed
> + * according to the IEC/IEEE Standard for Binary Floating-Point
> + * Arithmetic.
> + */
> +
> +static FloatParts round_to_int(FloatParts a, int rounding_mode, float_status *s)
> +{
> +    if (is_nan(a.cls)) {
> +        return return_nan(a, s);
> +    }
> +
> +    switch (a.cls) {
> +    case float_class_zero:
> +    case float_class_inf:
> +    case float_class_qnan:
> +        /* already "integral" */
> +        break;
> +    case float_class_normal:
> +        if (a.exp >= DECOMPOSED_BINARY_POINT) {
> +            /* already integral */
> +            break;
> +        }
> +        if (a.exp < 0) {
> +            bool one;
> +            /* all fractional */
> +            s->float_exception_flags |= float_flag_inexact;
> +            switch (rounding_mode) {
> +            case float_round_nearest_even:
> +                one = a.exp == -1 && a.frac > DECOMPOSED_IMPLICIT_BIT;
> +                break;
> +            case float_round_ties_away:
> +                one = a.exp == -1 && a.frac >= DECOMPOSED_IMPLICIT_BIT;
> +                break;
> +            case float_round_to_zero:
> +                one = false;
> +                break;
> +            case float_round_up:
> +                one = !a.sign;
> +                break;
> +            case float_round_down:
> +                one = a.sign;
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +
> +            if (one) {
> +                a.frac = DECOMPOSED_IMPLICIT_BIT;
> +                a.exp = 0;
> +            } else {
> +                a.cls = float_class_zero;
> +            }
> +        } else {
> +            uint64_t frac_lsb, frac_lsbm1, round_mask, roundeven_mask, inc;
> +
> +            frac_lsb = DECOMPOSED_IMPLICIT_BIT >> a.exp;
> +            frac_lsbm1 = frac_lsb >> 1;
> +            roundeven_mask = (frac_lsb - 1) | frac_lsb;
> +            round_mask = roundeven_mask >> 1;
> +
> +            switch (rounding_mode) {
> +            case float_round_nearest_even:
> +                inc = ((a.frac & roundeven_mask) != frac_lsbm1 ? frac_lsbm1 : 0);

This is that long line...

> +                break;

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM