[v2,2/8] softfloat: use bits32 instead of uint32

Use bits32 instead of uint32 when manipulating floating point values
directly for consistency reasons.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-native.c     |    4 ++--
 fpu/softfloat-specialize.h |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

On 4 January 2011 15:15, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Use bits32 instead of uint32 when manipulating floating point values
> directly for consistency reasons.

I'm not convinced this patch is particularly worthwhile, especially since
Andreas is working on a patchset which will convert all the bits32
uses back into uint32_t anyway, which is the direction to go if we
want to make the fpu/ code consistent about its type usage.

If you do want to do this, the commit message should be "uint32_t"
not "uint32" (which is a different type!)

>  int float32_is_quiet_nan( float32 a1 )
>  {
>     float32u u;
> -    uint64_t a;
> +    bits32 a;
>     u.f = a1;
>     a = u.i;
>     return ( 0xFF800000 < ( a<<1 ) );

This change is actually changing the type: shouldn't it be bits64 ?

It seems a bit inconsistent to change softfloat-native.c:float32_is_quiet_nan()
but not softfloat-native.c:float64_is_quiet_nan() (which uses uint64_t).

Personally I'd just drop this patch.

-- PMM

On Tue, Jan 04, 2011 at 03:51:37PM +0000, Peter Maydell wrote:
> On 4 January 2011 15:15, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > Use bits32 instead of uint32 when manipulating floating point values
> > directly for consistency reasons.
> 
> I'm not convinced this patch is particularly worthwhile, especially since
> Andreas is working on a patchset which will convert all the bits32
> uses back into uint32_t anyway, which is the direction to go if we
> want to make the fpu/ code consistent about its type usage.

I don't know in which direction we should go (bits32 or uint32_t), but
we should go for more consistency. When everything is consistent, it's
just a regex to go switch the type.

> If you do want to do this, the commit message should be "uint32_t"
> not "uint32" (which is a different type!)

Correct.

> >  int float32_is_quiet_nan( float32 a1 )
> >  {
> >     float32u u;
> > -    uint64_t a;
> > +    bits32 a;
> >     u.f = a1;
> >     a = u.i;
> >     return ( 0xFF800000 < ( a<<1 ) );
> 
> This change is actually changing the type: shouldn't it be bits64 ?

Yes, I should have mentioned it in the changelog. For me this looks like
a typo, as we are manipulating 32 bits values. Look at
float32_is_signaling_nan().

> It seems a bit inconsistent to change softfloat-native.c:float32_is_quiet_nan()
> but not softfloat-native.c:float64_is_quiet_nan() (which uses uint64_t).

I guess you meant softfloat-native.c:float32_is_quiet_nan(). It looks
like I missed this one, and that it should be changed too.

> Personally I'd just drop this patch.

I'll drop it for now and wait to see what Andreas offers.

On 4 January 2011 16:11, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Jan 04, 2011 at 03:51:37PM +0000, Peter Maydell wrote:
>> >  int float32_is_quiet_nan( float32 a1 )
>> >  {
>> >     float32u u;
>> > -    uint64_t a;
>> > +    bits32 a;
>> >     u.f = a1;
>> >     a = u.i;
>> >     return ( 0xFF800000 < ( a<<1 ) );
>>
>> This change is actually changing the type: shouldn't it be bits64 ?
>
> Yes, I should have mentioned it in the changelog. For me this looks like
> a typo, as we are manipulating 32 bits values. Look at
> float32_is_signaling_nan().

Well, it does affect whether that left-shift of a loses the sign bit
or not. However having thought about it it doesn't make any
difference to the result of the comparison, so bits32 would be OK.

On the other hand, this should be a "<=" compare, not "<", I think.
0x7FC00000 is a valid QNaN but this function will return false for
it. Contrast the softfloat-specialize.h float32_is_quiet_nan()
implementation for !SNAN_BIT_IS_ONE, which does use <=.

(On the third hand if you're using softfloat-native then you
probably didn't care about the niceties of NaN usage :-))

-- PMM

Am 04.01.2011 um 17:11 schrieb Aurelien Jarno:

> On Tue, Jan 04, 2011 at 03:51:37PM +0000, Peter Maydell wrote:
>> On 4 January 2011 15:15, Aurelien Jarno <aurelien@aurel32.net> wrote:
>>> Use bits32 instead of uint32 when manipulating floating point values
>>> directly for consistency reasons.
>>
>> I'm not convinced this patch is particularly worthwhile, especially  
>> since
>> Andreas is working on a patchset which will convert all the bits32
>> uses back into uint32_t anyway, which is the direction to go if we
>> want to make the fpu/ code consistent about its type usage.
>
> I don't know in which direction we should go (bits32 or uint32_t), but
> we should go for more consistency. When everything is consistent, it's
> just a regex to go switch the type.

Please don't introduce new uses of bits*. There's simply no reason to  
have such types in a C99 project.

Unfortunately the new year started with hardware failure for me, so I  
haven't found the time to continue work on the series yet. I'll send a  
rebased v4 shortly, patch 5/7 v3 was broken by a change to softfloat- 
specialize.h.

Andreas