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

[Tom Musta <tommusta@gmail.com>]

Peter:  Thanks for your feedback.  Responses below.


On 10/25/2013 6:55 AM, Peter Maydell wrote:
> On 24 October 2013 17:17, Tom Musta <tommusta@gmail.com> wrote:
>> This patch adds routines to the softfloat library that are useful for
>> the PowerPC VSX implementation.  The routines are, however, not specific
>> to PowerPC and are approprriate for softfloat.
>>
>> The following routines are added:
>>
>>    - 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

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

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

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