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

[Aurelien Jarno <aurelien@aurel32.net>]

On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
> On Wed, Jan 05, 2011 at 11:21:06AM +0100, Aurelien Jarno wrote:
> 
> > But that's not the subject we are talking about HPPA guest support. 
> > target-hppa fork still (partially) uses dyngen code, which we don't 
> > support in upstream for more than *2 years*.
> 
> I'd agree -- the fork is currently dead.  Whilst I do plan to revive it
> at some point, I would support removal of those particular lines, as:
> 
>    Dead code is noise.  It doesn't contribute anything, so you may as
>    well remove it.
> 
>    It's easy enough to add it back at a later date.
> 
>    Dead code is buggy code.  Previously, the code was quietening
>    signaling NaNs incorrectly.  MIPS now uses a default quiet NaN,
>    but HPPA would need a different fix.
> 
>    It's not even clear which of the two possible fixes should be applied
>    (or whether both should be applied with a means of selecting the
>    behaviour at run-time).  I gather that we need to clear the MSB of
>    the significand, and then set its least-significant-but-one bit,
>    either unconditionally, or perhaps only if the significand would
>    otherwise be zero.  However, I don't yet know which of those two
>    behaviours is implemented by hardware, and even if only one is
>    implemented, I still feel we'd still need a comment explaining that
>    the architecture's specification permits the alternate behaviour.
> 
> A few random thank-yous:
> 
>    I do really appreciate the effort to avoid removing lines that would
>    only be added back at a later date -- if we had an HPPA target fork
>    that was ready to be merged back in a week or two, then I'd argue that
>    there's no point, but that's not the case.
> 
>    Thanks for the comments on sNaN quietening for HPPA.  I would hopefully
>    read the PA1.1 spec thoroughly enough and test on real hardware upon
>    reaching the point where floating point support is somewhere higher
>    up on the TODO list... :-)  However, the comments can hardly hurt!
> 
>    What is important here is that the code be rewritten in a clean manner,
>    so that there are no unnecessary obstacles to modifying SoftFloat to
>    support new targets.  The patches that I've seen definitely seem to
>    move in that direction, so I'm quite happy.  Of course, fixing those
>    architectures that are already upstream is the main objective! -- and
>    if this helps other forks, that can't be a bad thing.
> 
> 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.
> 
>    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.

The problem with these functions is that they are used in target-*/
and not directly part of the softfloat code.

I have reviewed MIPS and PowerPC targets, they both use these functions
correctly.

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