Re: [Qemu-devel] [PATCH 01/19] Add New softfloat Routines for VSX

[Peter Maydell]

On 25 October 2013 14:01, Tom Musta <address@hidden> wrote:
> On 10/25/2013 6:55 AM, Peter Maydell wrote:
>> On 24 October 2013 17:17, Tom Musta <address@hidden> wrote:
>>>    - float32_is_denormal() returns true if the 32-bit floating point
>>> number
>>>      is denormalized.
>>>    - float64_is_denormal() returns true if the 64-bit floating point
>>> number
>>>      is denormalized.
>>
>>
>> Can you point me at the patches which use these, please?
>> I couldn't find them with a quick search in my email client.
>
>
> Please see
> http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html

Thanks. For that code you can just use the existing
is_zero_or_denormal function if you like, since you've
already ruled out "is this zero?" by the time you're
checking for "is this denormal?". (In fact that logic
seems to do a number of pointless checks for "is this
zero?" when it's already ruled that case out very early;
it should probably be rephrased.)

However I don't think there's any harm in our providing some
*_is_denormal() functions in our softfloat API if the code
seems clearer if it's written to use them. It does fill
out an odd gap in the API shape, as you note below.

>>>    - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit
>>>      floating point number.
>>>    - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit
>>>      floating point number.
>>
>>
>> These look rather odd to me, and again I can't find the uses in
>> your patchset. Returning just the exponent is a bit odd and
>> suggests that maybe the split between target code and softfloat
>> is in the wrong place.
>
>
> Please see
> http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html
> and http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03107.html
> and also the corresponding definitions of those instructions in the Power
> ISA.
>
> What is odd here is the PowerPC instruction(s) :)
>
> But given that softfloat code extracts exponents in numerous places, I do
> not find
> it odd at all that a floating point instruction model for a non-standard
> operation might have to do the same.
>
> These functions can easily be kept within the PowerPC code proper if there
> are
> objections to them being added to softfloat.  I would rename them, of
> course, so
> that they do not look like softfloat routines.

Mmm. You'll notice that your calling code has to know rather
a lot about the format of the IEEE floats (in that it has
to know the min/max exponent and mantissa width). So I think
I'd just opencode these in the PPC routines. (This is what we
do in target-arm, see recpe_f32 and rsqrte_f32 for examples.)

>>>    - float32_to_uint64() converts a 32-bit floating point number to an
>>>      unsigned 64 bit number.
>>
>>
>> I would put this in its own patch, personally.
>
>
> Fair enough.  Just so that I am clear ... do you mean submit this as a patch
> just by itself (not as part of a series of VSX additions)?

I mean "in its own patch email so it is a separate commit and
clearly separated from other things for code review purposes".
You probably still keep it as part of this patch series. (In
fact it would also be a good idea to include the previous
patch this one depends on, if that has not yet been committed.)

>
>>>
>>> +INLINE int float32_is_denormal(float32 a)
>>> +{
>>> +    return ((float32_val(a) & 0x7f800000) == 0) &&
>>> +           ((float32_val(a) & 0x007fffff) != 0);
>>> +}
>>
>>
>> return float32_is_zero_or_denormal(a) && !float32_is_zero(a);
>>
>> is easier to review and less duplicative of code.
>>
>> thanks
>
>
> It surprised me that there were is_zero and is_zero_or_denormal functions
> but
> not is_denormal functions.  I would find it more normal to implement the two
> primitive functions and then construct is_zero_or_denormal to be the OR of
> those two.  Until you look at efficiency of the implementation.

I think also the original uses of these functions didn't
need to distinguish zero from denormal, so it was a more
natural API for those uses.

-- PMM