Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Stuart Brady]

On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
> On 5 January 2011 23:13, Stuart Brady <address@hidden> wrote:
> > I do have a few concerns regarding SoftFloat, though:
> >
> >   FIXMEs should be left in the code (or a document maintained on the
> >   Wiki) to keep track of which architectures have been considered
> >   (which I believe are x86, arm, mips, ppc) and which ones haven't.
> >   This is in reference to one particular FIXME that was removed,
> >   but perhaps shouldn't have been.
> 
> Which one? The only one I know I removed was the one in
> the patch that implemented the thing it was complaining about,
> but perhaps I took out another by accident...

The patch is question was:

   softfloat: Implement flushing input denormals to zero

The comment is:

   /* FIXME: Flush-To-Zero only effects results.  Denormal inputs should
      also be flushed to zero.  */

Do you know whether ARM is the only target architecture supported by
QEMU that requires this behaviour?

If there are any architectures where we simply don't know whether the
current behaviour is correct, we should document that somewhere.

For any target-specific behaviour, I really feel that we should have
architectures listed explicitly rather than having using a default
#else case (and that the #else case should simply contain a #error),
as it may be worth guarding against any newly added targets incorrectly
picking the default behaviour (as has happened in the past).

Presumably, sNaNs on any target where they are indicated by a zero bit
should be quietened by simply setting that bit.  However, if a target
doesn't define SNAN_BIT_IS_ONE, that may simply be because it was
forgotten.  This is why I don't like using #ifndef in general...
much better, IMO, to define as '0' or '1', and then -Werror=undef can
catch anything that gets overlooked.  (Except now the problem is any
erroneous use of #ifdef with such definitions that might creep in...)

> >   Is there any plan to deal with use of float*_is_quiet_nan(), where
> >   float*_is_any_nan() was intended?  These should really either be
> >   fixed (and tested), or if not, a FIXME should be added.
> 
> I was planning to do the ARM related ones, at least (although
> they are in the pretty-much-dead FPE-emulation code for
> linux user-mode).
> 
> I would be more inclined to track that sort of thing in a bug tracker
> than by sprinkling the code with FIXME comments.

I'd definitely agree that FIXMEs are not as good as bug reports, and/or
possibly even a Features/SoftFloat page on the Wiki if appropriate.

Cheers,
-- 
Stuart Brady