Re: [Qemu-devel] [PATCH V2] softfloat: Rename float*_is_nan() functions to float*_is_quiet_nan()

[Andreas Färber]

Am 18.12.2010 um 13:15 schrieb Peter Maydell:

> On 18 December 2010 11:49, Andreas Färber <address@hidden>  
> wrote:
> > IMO a lot of code in QEMU is cryptic because someone thinks that  
> > someone
> > else must've thought something particular when doing it that way  
> > and is thus
> > reluctant to touch it...
>
> > For a fact, [u]int8 und [u]int64 remain unchanged width-wise.
> > For [u]int16, only malc may know what that maps to on AIX, for  
> > which they
> > are #ifndef'ed out. I doubt it's an int.
> > Unless there's an ILP64 platform we support, [u]int32 would stay  
> > the same
> > width, too.
> > That's why I was saying, putting, e.g., a 33rd bit into int32 has  
> > undefined
> > semantics, just as for the POSIX int_least32_t type. I don't see a  
> > win in
> > declaring that information.
>
> It's saying "it's OK to provide more than 32 bits if that would be  
> faster",
> and indeed that's exactly how we typedef it. Some parts of softfloat
> that use bits32 do rely on there being exactly 32 bits and not 64.

Okay, then I'll use int_least*_t instead. Performance is always a good  
argument. :)

> > My patch tries to do three things in one:
>
> > 1.) Fix mismatches between headers and sources, i.e. float32
> > int32_to_float32(int); vs. float32 int32_to_float32(int32) {...} etc.
>
> In this case we do know the rationale for this mismatch:
> http://www.jhauser.us/arithmetic/SoftFloat-2b/SoftFloat-source.txt
> ---begin---
> Unlike the actual function definitions in `softfloat.c', the  
> declarations
> in `softfloat.h' do not use any of the types defined by the  
> `processors'
> header file.  This is done so that clients will not have to include  
> the
> `processors' header file in order to use SoftFloat.  Nevertheless, the
> target-specific declarations in `softfloat.h' must match what  
> `softfloat.c'
> expects.  For example, if `int32' is defined as `int' in the  
> `processors'
> header file, then in `softfloat.h' the output of `float32_to_int32'  
> should
> be stated as `int', although in `softfloat.c' it is given in target-
> independent form as `int32'.
> ---endit---
>
> ...which doesn't really apply to the way qemu has taken softfloat,
> so I agree it makes sense to change things here.

So, how far are we from the original? Given that the latest version is  
from 2002, would it make any sense to move this to a Git submodule?

> > 2.) Drop the unnecessary custom integer types in favor of standard  
> > ones.
>
> ...but it doesn't do this because it isn't touching the 'bits32'  
> types which
> are the ones which are exact synonyms for the posix int32_t &c.
> If your aim is to remove unnecessary custom types this is the first  
> and
> easiest target because you can do it as a pure search-and-replace.

OK, will do.

> > 3.) Fix instances of lazyness where _t was forgotten and the  
> > mistake was
> > hidden by the softfloat typedefs.
> >
> > Renaming int32 to qint32 would defeat the second purpose. I got  
> > around the
> > Haiku issue for now, so that's not a pressing need.
> >
> > Had the softfloat code not been a real refactoring-unfriendly mess  
> > of int,
> > int* and int*_t, I would've offered to do this in multiple steps  
> > per type. I
> > could try splitting out part 1 above. Part 3 can easily be split  
> > off by
> > cut-and-paste and could be applied independently.
>
> I certainly think it would be easier to review as separate patches  
> which
> did different things.
>
> > Promoting int16[_t] to int for things like shift counts is beyond  
> > the scope
> > of my patch.
>
> ...but your patch changes what is currently an 'int' (hidden behind  
> the int16
> typedef) to int16_t, so it is making a change here. It's a safe  
> change but
> it doesn't actually make any sense.

No, it's an implementation detail and is only the way you describe  
outside AIX currently. It may well be int16_t already there.
And yes, it doesn't make sense, but that's not my fault, it's the  
author's use of int16 that's unnecessary from the start.

Andreas