Re: [Qemu-devel] [PATCH v3 2/4] target-tilegx: Add single floating point implementation

[Richard Henderson <rth@twiddle.net>]

On 12/10/2015 06:15 AM, Chen Gang wrote:
> +#define TILEGX_F_CALC_CVT   0     /* convert int to fsingle */
> +#define TILEGX_F_CALC_NCVT  1     /* Not convertion */
> +
> +static uint32_t get_f32_exp(float32 f)
> +{
> +    return extract32(float32_val(f), 23, 8);
> +}
> +
> +static void set_f32_exp(float32 *f, uint32_t exp)
> +{
> +    *f = make_float32(deposit32(float32_val(*f), 23, 8, exp));
> +}

Why take a pointer instead of returning the new value?

> +static inline uint32_t get_fsingle_sign(uint64_t n)
> +{
> +    return test_bit(10, &n);
> +}
> +
> +static inline void set_fsingle_sign(uint64_t *n)
> +{
> +    set_bit(10, n);
> +}

Why are you using test_bit and set_bit here, rather than continuing to use
deposit and extract?

> +static float32 sfmt_to_float32(uint64_t sfmt, float_status *fp_status)
> +{
> +    float32 f;
> +    uint32_t sign = get_fsingle_sign(sfmt);
> +    uint32_t man = get_fsingle_man(sfmt);
> +
> +    if (get_fsingle_calc(sfmt) == TILEGX_F_CALC_CVT) {
> +        if (sign) {
> +            return int32_to_float32(0 - man, fp_status);
> +        } else {
> +            return uint32_to_float32(man, fp_status);
> +        }
> +    } else {
> +        f = float32_set_sign(float32_zero, sign);
> +        f |= create_f32_man(man >> 8);
> +        set_f32_exp(&f, get_fsingle_exp(sfmt));
> +    }

I'm not especially keen on this calc bit.  I'd much rather that we always pack
and round properly.

In particular, if gcc decided to optimize fractional fixed-point types, it
would do something very similar to the current floatsisf2 code sequence, except
that it wouldn't use 0x9e as the exponent; it would use something smaller, so
that some number of low bits of the mantessa would be below the radix point.

Therefore, I think that fsingle_pack2 should do the following: Take the
(sign,exp,man) tuple and slot them into a double -- recall that a single only
has 23 bits in its mantessa, and this temp format has 32 -- then convert the
double to a single.  Pre-rounded single results from fsingle_* will be
unchanged, while integer data that gcc has constructed will be properly rounded.

E.g.

  uint32_t sign = get_fsingle_sign(sfmt);
  uint32_t exp = get_fsingle_exp(sfmt);
  uint32_t man = get_fsingle_man(sfmt);
  uint64_t d;

  /* Adjust the exponent for double precision, preserving Inf/NaN.  */
  if (exp == 0xff) {
    exp = 0x7ff;
  } else {
    exp += 1023 - 127;
  }

  d = (uint64_t)sign << 63;
  d = deposit64(d, 53, 11, exp);
  d = deposit64(d, 21, 32, man);
  return float64_to_float32(d, fp_status);

Note that this does require float32_to_sfmt to store the mantissa
left-justified. That is, not in bits [54-32] as you're doing now, but in bits
[63-41].

> +static void ana_bits(float_status *fp_status,
> +                     float32 fsrca, float32 fsrcb, uint64_t *sfmt)

Is "ana" supposed to be short for "analyze"?

> +{
> +    if (float32_eq(fsrca, fsrcb, fp_status)) {
> +        *sfmt |= create_fsfd_flag_eq();
> +    } else {
> +        *sfmt |= create_fsfd_flag_ne();
> +    }
> +
> +    if (float32_lt(fsrca, fsrcb, fp_status)) {
> +        *sfmt |= create_fsfd_flag_lt();
> +    }
> +    if (float32_le(fsrca, fsrcb, fp_status)) {
> +        *sfmt |= create_fsfd_flag_le();
> +    }
> +
> +    if (float32_lt(fsrcb, fsrca, fp_status)) {
> +        *sfmt |= create_fsfd_flag_gt();
> +    }
> +    if (float32_le(fsrcb, fsrca, fp_status)) {
> +        *sfmt |= create_fsfd_flag_ge();
> +    }
> +
> +    if (float32_unordered(fsrca, fsrcb, fp_status)) {
> +        *sfmt |= create_fsfd_flag_un();
> +    }
> +}

Again, I think it's better to return the new sfmt value than modify a pointer.


r~