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

[Tom Musta]

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 <address@hidden> 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.