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

[Stuart Brady]

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.

   Perhaps it would be worth documenting the IEEE 754-2008 behaviour,
   especially in any cases where we already happen to implement that
   behaviour?  (Once I've actually looked at IEEE 754-2008, I might
   be able to contribute something in this regard.)

   I do appreciate that there's still work to be done -- my intent here is
   just to make sure that nothing's in danger of slipping through the net.

Regarding the TARGET_HPPA definition in linux-user:

   In my opinion, the reference to TARGET_HPPA in the definition of
   target_flock64 in linux-user/syscall_defs.h should also be ditched,
   although whether it's worth anyone's time submitting a patch to remove
   that, I'm not really sure.

   I would also argue that the handling of padding in linux-user is less
   than ideal.  The problem here seems to be that as fields have different
   sizes (or may be absent entirely) on different architectures, different
   padding is required.  Rather than a laundry list of targets, this would
   best handled automatically, even if this would required rather evil
   macros or code generation... perhaps I should give this a go?

Cheers,
-- 
Stuart Brady