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

[Chen Gang <chengang@emindsoft.com.cn>]

On 12/11/15 01:15, Richard Henderson wrote:
> 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?
>

I referenced set_* functions' declarations in "include/fpu/softfloat.h",
originally.

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

It is really only for one bit test and set, so test_bit/set_bit are
simpler and clearer than deposit/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.
>

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

Oh, really.

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

For me, it is a good idea! :-)


>> +static void ana_bits(float_status *fp_status,
>> +                     float32 fsrca, float32 fsrcb, uint64_t *sfmt)
> 
> Is "ana" supposed to be short for "analyze"?
>

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

Oh, I guess, we can inline ana_bits() to main_calc(), for they are both
simple short functions, and ana_bits() is only called by main_calc().

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed