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

[Richard Henderson <rth@twiddle.net>]

On 12/11/2015 03:38 PM, Chen Gang wrote:
>
> On 12/11/15 05:17, Richard Henderson wrote:
>> On 12/10/2015 06:15 AM, Chen Gang wrote:
>>> +#define TILEGX_F_MAN_HBIT   (1ULL << 59)
>> ...
>>> +static uint64_t fr_to_man(float64 d)
>>> +{
>>> +    uint64_t val = get_f64_man(d) << 7;
>>> +
>>> +    if (get_f64_exp(d)) {
>>> +        val |= TILEGX_F_MAN_HBIT;
>>> +    }
>>> +
>>> +    return val;
>>> +}
>>
>> One presumes that "HBIT" is the ieee implicit one bit.
>> A better name or better comments would help there.
>>
>
> OK, thanks. And after think of again, I guess, the real hardware does
> not use HBIT internally (use the full 64 bits as mantissa without HBIT).

It must do.  Otherwise the arithmetic doesn't work out.

> But what I have done is still OK (use 59 bits + 1 HBIT as mantissa), for
> 59 bits are enough for double mantissa (52 bits). It makes the overflow
> processing easier, but has to process mul operation specially.

What you have works.  But the mul operation isn't as special as you make it out 
-- aside from requiring at least 104 bits as intermediate -- in that when one 
implements what the hardware does, subtraction also may require significant 
normalization.

> According to floatsidf, it seems "4", but after I expanded the bits, I
> guess, it is "7".
>
> /*
>   * Double exp analyzing: (0x21b00 << 1) - 0x37(55) = 0x3ff
>   *
>   *   17  16  15  14  13  12  11  10   9   8   7    6   5   4   3   2   1   0
>   *
>   *    1   0   0   0   0   1   1   0   1   1   0    0   0   0   0   0   0   0
>   *
>   *    0   0   0   0   0   1   1   0   1   1   1    => 0x37(55)
>   *
>   *    0   1   1   1   1   1   1   1   1   1   1    => 0x3ff
>   *
>   */

That's the exponent within the flags temporary.  It has nothing to do with the 
position of the extracted mantissa.

FWIW, the minimum shift would be 3, in order to properly implement rounding; if 
the hardware uses a shift of 4, that's fine too.

What I would love to know is if the shift present in floatsidf is not really 
required; equally valid to adjust 0x21b00 by 4.  Meaning normalization would do 
a proper job with the entire given mantissa.  This would require better 
documentation, or access to hardware to verify.

>>> +uint64_t helper_fdouble_addsub(CPUTLGState
> And for my current implementation (I guess, it should be correct):
>
> typedef union TileGXFPDFmtV {
>      struct {
>          uint64_t mantissa : 60;   /* mantissa */
>          uint64_t overflow : 1;    /* carry/overflow bit for absolute add/mul */
>          uint64_t unknown1 : 3;    /* unknown */

I personally like to call all 4 of the top bits overflow.  But I have no idea 
what the real hardware actually does.

> In helper_fdouble_addsub(), both dest and srca are unpacked, so they are
> within 60 bits. So one time absolute add are within 61 bits, so let bit
> 61 as overflow bit is enough.

True.  But if all 4 top bits are considered overflow, then one could implement 
floatdidf fairly easily.  But I suspect that real hw doesn't work that way, or 
it would have already been done.


r~