Re: [Qemu-devel] [PATCH v2 10/20] fpu/softfloat: define decompose structures

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

On 9 January 2018 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:
> These structures pave the way for generic softfloat helper routines
> that will operate on fully decomposed numbers.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 59afe81d06..fcba28d3f8 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -83,7 +83,7 @@ this code that are retained.
>   * target-dependent and needs the TARGET_* macros.
>   */
>  #include "qemu/osdep.h"
> -
> +#include "qemu/bitops.h"
>  #include "fpu/softfloat.h"
>
>  /* We only need stdlib for abort() */
> @@ -186,6 +186,74 @@ static inline flag extractFloat64Sign(float64 a)
>      return float64_val(a) >> 63;
>  }
>
> +/*----------------------------------------------------------------------------
> +| Classify a floating point number.
> +*----------------------------------------------------------------------------*/
> +
> +typedef enum {
> +    float_class_unclassified,
> +    float_class_zero,
> +    float_class_normal,
> +    float_class_inf,
> +    float_class_qnan,
> +    float_class_snan,
> +    float_class_dnan,
> +    float_class_msnan, /* maybe silenced */
> +} float_class;
> +
> +/*----------------------------------------------------------------------------
> +| Structure holding all of the decomposed parts of a float.
> +| The exponent is unbiased and the fraction is normalized.
> +*----------------------------------------------------------------------------*/
> +
> +typedef struct {
> +    uint64_t frac   : 64;
> +    int exp         : 32;
> +    float_class cls : 8;
> +    int             : 23;

What is this unnamed 23 bit field for?

> +    bool sign       : 1;

Why are we using a bitfield struct here anyway? uint64_t is 64 bits,
int is 32 bits, we don't care how big the float_class enum is
represented as, and we're not trying to pack together lots of bools
so it doesn't matter much if we have a whole byte for the sign.

> +} decomposed_parts;
> +
> +#define DECOMPOSED_BINARY_POINT    (64 - 2)
> +#define DECOMPOSED_IMPLICIT_BIT    (1ull << DECOMPOSED_BINARY_POINT)
> +#define DECOMPOSED_OVERFLOW_BIT    (DECOMPOSED_IMPLICIT_BIT << 1)
> +
> +/* Structure holding all of the relevant parameters for a format.  */
> +typedef struct {
> +    int exp_bias;
> +    int exp_max;
> +    int frac_shift;
> +    uint64_t frac_lsb;
> +    uint64_t frac_lsbm1;

Why the '1' in the field name? Overall I think some brief
comments about what the fields mean would be helpful.

> +    uint64_t round_mask;
> +    uint64_t roundeven_mask;
> +} decomposed_params;
> +
> +#define FRAC_PARAMS(F)                     \
> +    .frac_shift     = F,                   \
> +    .frac_lsb       = 1ull << (F),         \
> +    .frac_lsbm1     = 1ull << ((F) - 1),   \
> +    .round_mask     = (1ull << (F)) - 1,   \
> +    .roundeven_mask = (2ull << (F)) - 1
> +
> +static const decomposed_params float16_params = {
> +    .exp_bias       = 0x0f,
> +    .exp_max        = 0x1f,
> +    FRAC_PARAMS(DECOMPOSED_BINARY_POINT - 10)
> +};
> +
> +static const decomposed_params float32_params = {
> +    .exp_bias       = 0x7f,
> +    .exp_max        = 0xff,
> +    FRAC_PARAMS(DECOMPOSED_BINARY_POINT - 23)
> +};
> +
> +static const decomposed_params float64_params = {
> +    .exp_bias       = 0x3ff,
> +    .exp_max        = 0x7ff,
> +    FRAC_PARAMS(DECOMPOSED_BINARY_POINT - 52)

Maybe we should hide the DECOMPOSED_BINARY_POINT bit inside the macro?
Then the 10/23/52 are just the number of fraction bits in the format.

thanks
-- PMM