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~