Re: [Qemu-devel] [PATCH v5 04/10] softfloat: Drop [u]int16 types in favor of [u]int_fast16_t

[Aurelien Jarno <aurelien@aurel32.net>]

On Tue, Mar 08, 2011 at 12:02:39AM +0100, Andreas Färber wrote:
> Am 07.03.2011 um 10:56 schrieb Aurelien Jarno:
> 
> >On Mon, Mar 07, 2011 at 01:34:07AM +0100, Andreas Färber wrote:
> >>AIX already didn't use our definition, so let's start ripping
> >>out this one:
> >>As pointed out by Peter Maydell, int16 is currently int on most
> >>supported platforms, so let's replace it with int_fast16_t,
> >>allowing the system to use a wider type if appropriate.
> >
> >Do you have a rationale about the use of the fast version of the
> >types?
> >As you said it allows the compiler to use a wider type if appropriate,
> >and this usually doesn't play very well when shifts are involved,
> >which
> >is the case of the softfloat code. Have you verified that each
> >conversion is correct? On the other hand I really doubt it has a
> >measurable impact on speed.
> >
> >I would really go for uint16_t for now. This might be changed to
> >the fast
> >version in a second step if we are sure everything is correct, but
> >let's
> >don't mix the two steps for now.
> 
> SoftFloat distinguishes between [s]bits* and [u]int*. Wherever
> exact-width semantics were needed, [s]bits* was used (or should have
> been) and this is converted in 03/10. It was Peter's demand [1] that
> we retain this distinction and it was my suggestion to then use
> [u]int_fast*_t, which provides such semantics for POSIX, since the
> colliding use of [u]int* and its accidental uses outside SoftFloat
> are what this series seeks to address. The difference is that with
> POSIX the semantics are system-defined whereas right now they are
> QEMU-defined.

The patch actually changes the semantic. Before int, ie 32 bit was used
for int16. With your changes it can now be 16, 32 or 64 bit depending on
the host. This is the kind of changes we don't want, it means problems
are going to depend on the host for no real reason.

This is actually what you observed later in the series: there are types
mismatch in your patches, though they match on your machine (probably
32-bit), but not on mine.

> Most of the series was crafted in a semi-automatic fashion, doing a
> search and reviewing each hit before replacing. If we drop the type
> distinction and use uint16_t for both bits16 and uint16 then we
> cannot easily do the second step you propose.
> 
> My and, I figured, Peter's intent was to do this conversion without
> changing semantics as far as possible. Softfloat-native turned out
> difficult in that aspect, since it followed the definitions of the
> header rather than the types used in the SoftFloat implementation.
> There, the conversions need more careful review of whether they
> wrongly rely on fixed-width semantics.
> 
> But seeing the persistent rate of softfloat bug fixes, I would ask
> to address the license issue sooner than later. Doesn't need to be
> my patch, if someone feels tempted to do better.
> 

I agree it is something that should be fixed, however given we are
modifying this file for more than 5 years, a few days more or less won't
change anything.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net