Patchwork [1/6] softfloat: remove HPPA specific code

login
register
mail settings
Submitter Aurelien Jarno
Date Jan. 3, 2011, 2:34 p.m.
Message ID <1294065273-30274-2-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/77275/
State New
Headers show

Comments

Aurelien Jarno - Jan. 3, 2011, 2:34 p.m.
We don't have any HPPA target, so let's remove HPPA specific code. It
can be re-added when someone adds an HPPA target.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
Peter Maydell - Jan. 3, 2011, 3:22 p.m.
On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> We don't have any HPPA target, so let's remove HPPA specific code. It
> can be re-added when someone adds an HPPA target.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Do we want to get rid of the one remaining TARGET_HPPA which
is in linux-user/syscall_defs.h?

-- PMM
Aurelien Jarno - Jan. 3, 2011, 3:26 p.m.
Peter Maydell a écrit :
> On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> We don't have any HPPA target, so let's remove HPPA specific code. It
>> can be re-added when someone adds an HPPA target.
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Do we want to get rid of the one remaining TARGET_HPPA which
> is in linux-user/syscall_defs.h?
> 

Thanks for the review. I personnally don't have a lot of interest in
linux-user, so I will let the linux-user maintainer (Cc) to decide.
Andreas Färber - Jan. 4, 2011, 7:54 p.m.
Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:

> We don't have any HPPA target, so let's remove HPPA specific code. It
> can be re-added when someone adds an HPPA target.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

There actually is such a project on SourceForge [1, 2].

Does it really hurt to leave TARGET_HPPA around?

Andreas

[1] http://hppaqemu.sourceforge.net/
[2] http://wiki.qemu.org/Features/HPPA

> ---
> fpu/softfloat-specialize.h |    6 +-----
> 1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index f8f36f3..f23bd6a 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -30,7 +30,7 @@ these four paragraphs for those parts of this code  
> that are retained.
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ====================================================================*/
>
> -#if defined(TARGET_MIPS) || defined(TARGET_HPPA)
> +#if defined(TARGET_MIPS)
> #define SNAN_BIT_IS_ONE		1
> #else
> #define SNAN_BIT_IS_ONE		0
> @@ -63,8 +63,6 @@ typedef struct {
> #define float32_default_nan make_float32(0x7FFFFFFF)
> #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) ||  
> defined(TARGET_ALPHA)
> #define float32_default_nan make_float32(0x7FC00000)
> -#elif defined(TARGET_HPPA)
> -#define float32_default_nan make_float32(0x7FA00000)
> #elif SNAN_BIT_IS_ONE
> #define float32_default_nan make_float32(0x7FBFFFFF)
> #else
> @@ -275,8 +273,6 @@ static float32 propagateFloat32NaN( float32 a,  
> float32 b STATUS_PARAM)
> #define float64_default_nan make_float64(LIT64( 0x7FFFFFFFFFFFFFFF ))
> #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) ||  
> defined(TARGET_ALPHA)
> #define float64_default_nan make_float64(LIT64( 0x7FF8000000000000 ))
> -#elif defined(TARGET_HPPA)
> -#define float64_default_nan make_float64(LIT64( 0x7FF4000000000000 ))
> #elif SNAN_BIT_IS_ONE
> #define float64_default_nan make_float64(LIT64( 0x7FF7FFFFFFFFFFFF ))
> #else
> -- 
> 1.7.2.3
>
>
Aurelien Jarno - Jan. 4, 2011, 8:07 p.m.
On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>
>> We don't have any HPPA target, so let's remove HPPA specific code. It
>> can be re-added when someone adds an HPPA target.
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>
> There actually is such a project on SourceForge [1, 2].

The project hasn't seen any commit for 1.5 year. It looks like dead.

> Does it really hurt to leave TARGET_HPPA around?

It means writing code for this target, in the current case for
floatXX_maybe_silence_NaN(). I don't see the point of writing and
maintaining unused code if we don't get the insurance the target is
going to be merged later. Unless someone volunteer to maintain this
code.
Andreas Färber - Jan. 4, 2011, 10:53 p.m.
Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:

> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>
>>> We don't have any HPPA target, so let's remove HPPA specific code.  
>>> It
>>> can be re-added when someone adds an HPPA target.
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>> There actually is such a project on SourceForge [1, 2].
>
> The project hasn't seen any commit for 1.5 year. It looks like dead.

As we have begun to collect in the forks thread, there's many multi- 
month-old repos around with features not in upstream. Even "dead"  
doesn't mean useless. Considering that linux-user is incomplete even  
on amd64, I don't see why we shouldn't have target-hppa in master.  
Then it would at least allow for compile-testing and would benefit  
from general refactoring rather than bitrotting. All it takes is  
someone to step up for upstreaming patches, and I do not feel  
qualified to volunteer for that architecture.

>> Does it really hurt to leave TARGET_HPPA around?
>
> It means writing code for this target, in the current case for
> floatXX_maybe_silence_NaN(). I don't see the point of writing and
> maintaining unused code if we don't get the insurance the target is
> going to be merged later. Unless someone volunteer to maintain this
> code.

For new code,

#elif defined(TARGET_HPPA)
#error Target not supported yet.
...

shouldn't be too much work if you already handle architecture  
specifics and is different from ripping out existing code, as you  
seemed to suggest for linux-user.

Andreas
Aurelien Jarno - Jan. 4, 2011, 11:56 p.m.
On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
> Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
>
>> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>>
>>>> We don't have any HPPA target, so let's remove HPPA specific code.  
>>>> It
>>>> can be re-added when someone adds an HPPA target.
>>>>
>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>
>>> There actually is such a project on SourceForge [1, 2].
>>
>> The project hasn't seen any commit for 1.5 year. It looks like dead.
>
> As we have begun to collect in the forks thread, there's many multi- 
> month-old repos around with features not in upstream. Even "dead"  
> doesn't mean useless. Considering that linux-user is incomplete even on 
> amd64, I don't see why we shouldn't have target-hppa in master. Then it 
> would at least allow for compile-testing and would benefit from general 
> refactoring rather than bitrotting. All it takes is someone to step up 
> for upstreaming patches, and I do not feel qualified to volunteer for 
> that architecture.

You are comparing apples and oranges, forks and dead code. linux-user on
x86_64 is able to run binaries. HPPA code has still to be ported to TCG.
Yes it still uses dyngen.

>>> Does it really hurt to leave TARGET_HPPA around?
>>
>> It means writing code for this target, in the current case for
>> floatXX_maybe_silence_NaN(). I don't see the point of writing and
>> maintaining unused code if we don't get the insurance the target is
>> going to be merged later. Unless someone volunteer to maintain this
>> code.
>
> For new code,
>
> #elif defined(TARGET_HPPA)
> #error Target not supported yet.
> ...
>
> shouldn't be too much work if you already handle architecture specifics 

That makes work, code less readable, without any benefit. The code should
be added when we have a real fork to reintegrate back.

Should we already start adding code provision about TARGET_DPTR (the
marvelous CPU I have dreamed last night)? And about TARGET_IPU (the one
I have dreamed the night before)?

> and is different from ripping out existing code, as you seemed to suggest 
> for linux-user.

Strange interpretation for "I personnally don't have a lot of interest
in linux-user, so I will let the linux-user maintainer (Cc) to decide."
Andreas Färber - Jan. 5, 2011, 8:15 a.m.
Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:

> On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
>> Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
>>
>>> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>>>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>>>
>>>>> We don't have any HPPA target, so let's remove HPPA specific code.
>>>>> It
>>>>> can be re-added when someone adds an HPPA target.
>>>>>
>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>>
>>>> There actually is such a project on SourceForge [1, 2].
>>>
>>> The project hasn't seen any commit for 1.5 year. It looks like dead.
>>
>> As we have begun to collect in the forks thread, there's many multi-
>> month-old repos around with features not in upstream. Even "dead"
>> doesn't mean useless. Considering that linux-user is incomplete  
>> even on
>> amd64, I don't see why we shouldn't have target-hppa in master.  
>> Then it
>> would at least allow for compile-testing and would benefit from  
>> general
>> refactoring rather than bitrotting. All it takes is someone to step  
>> up
>> for upstreaming patches, and I do not feel qualified to volunteer for
>> that architecture.
>
> You are comparing apples and oranges, forks and dead code. linux- 
> user on
> x86_64 is able to run binaries. HPPA code has still to be ported to  
> TCG.
> Yes it still uses dyngen.

Oh. I was pretty sure I saw TCG host support patches...

>>>> Does it really hurt to leave TARGET_HPPA around?
>>>
>>> It means writing code for this target, in the current case for
>>> floatXX_maybe_silence_NaN(). I don't see the point of writing and
>>> maintaining unused code if we don't get the insurance the target is
>>> going to be merged later. Unless someone volunteer to maintain this
>>> code.
>>
>> For new code,
>>
>> #elif defined(TARGET_HPPA)
>> #error Target not supported yet.
>> ...
>>
>> shouldn't be too much work if you already handle architecture  
>> specifics
>
> That makes work, code less readable, without any benefit. The code  
> should
> be added when we have a real fork to reintegrate back.
>
> Should we already start adding code provision about TARGET_DPTR (the
> marvelous CPU I have dreamed last night)? And about TARGET_IPU (the  
> one
> I have dreamed the night before)?
>
>> and is different from ripping out existing code, as you seemed to  
>> suggest
>> for linux-user.
>
> Strange interpretation for "I personnally don't have a lot of interest
> in linux-user, so I will let the linux-user maintainer (Cc) to  
> decide."

Really? I think you're completely missing my point.

Quote: "softfloat: remove HPPA specific code

We don't have any HPPA target, so let's remove HPPA specific code.  
[...]"

Quote: "> Do we want to get rid of the one remaining TARGET_HPPA which  
is in linux-user/syscall_defs.h?
[...] I will let the linux-user maintainer (Cc) [...] decide."

I'm saying that the justification of your patch and action towards  
Riku is wrong. Patch 1/6 should be dropped, don't remove code just  
because "we don't have any HPPA target". Because obviously if anyone  
rebases hppa.git to such a master HEAD, the code will be gone just as  
well.

What you've been interpreting into this is that you should go through  
hoops and newly add some TARGET_FOOBAR crap, which is not what I said.  
If you add some silence_magic_nan() and don't *know* how the code for  
HPPA should look like based on the pre-existent code, then I suggested  
to simply stub it out for such platforms (whether based on #elif  
defined(TARGET_foo) or a mere #else is pretty irrelevant to me). So in  
the end we will end up with patches that don't support hppa out of the  
box but which don't needlessly remove SNAN_BIT_IS_ONE and  
float{32,64}_default_nan(). I fail to see how these three macros hurt  
without digging deeper into your patches. If they do, then your patch  
1/6 is lacking a proper description!

Point is, don't remove valuable code just because it's not being used  
in upstream *yet*.

In that context, there would've been no need to bring the linux-user  
maintainer into this who could've complained earlier if HPPA caused  
him any trouble. I never stumbled across any prominent mention of hppa  
in his patch series.

Hope that explains,

Andreas
Aurelien Jarno - Jan. 5, 2011, 10:21 a.m.
On Wed, Jan 05, 2011 at 09:15:43AM +0100, Andreas Färber wrote:
> Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:
> 
> >On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
> >>Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
> >>
> >>>On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
> >>>>Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
> >>>>
> >>>>>We don't have any HPPA target, so let's remove HPPA specific code.
> >>>>>It
> >>>>>can be re-added when someone adds an HPPA target.
> >>>>>
> >>>>>Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >>>>
> >>>>There actually is such a project on SourceForge [1, 2].
> >>>
> >>>The project hasn't seen any commit for 1.5 year. It looks like dead.
> >>
> >>As we have begun to collect in the forks thread, there's many multi-
> >>month-old repos around with features not in upstream. Even "dead"
> >>doesn't mean useless. Considering that linux-user is incomplete
> >>even on
> >>amd64, I don't see why we shouldn't have target-hppa in master.
> >>Then it
> >>would at least allow for compile-testing and would benefit from
> >>general
> >>refactoring rather than bitrotting. All it takes is someone to
> >>step up
> >>for upstreaming patches, and I do not feel qualified to volunteer for
> >>that architecture.
> >
> >You are comparing apples and oranges, forks and dead code. linux-
> >user on
> >x86_64 is able to run binaries. HPPA code has still to be ported
> >to TCG.
> >Yes it still uses dyngen.
> 
> Oh. I was pretty sure I saw TCG host support patches...

Yes, there is TCG *host* code in upstream, and this code has been updated
recently. 

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*.

> >>>>Does it really hurt to leave TARGET_HPPA around?
> >>>
> >>>It means writing code for this target, in the current case for
> >>>floatXX_maybe_silence_NaN(). I don't see the point of writing and
> >>>maintaining unused code if we don't get the insurance the target is
> >>>going to be merged later. Unless someone volunteer to maintain this
> >>>code.
> >>
> >>For new code,
> >>
> >>#elif defined(TARGET_HPPA)
> >>#error Target not supported yet.
> >>...
> >>
> >>shouldn't be too much work if you already handle architecture
> >>specifics
> >
> >That makes work, code less readable, without any benefit. The code
> >should
> >be added when we have a real fork to reintegrate back.
> >
> >Should we already start adding code provision about TARGET_DPTR (the
> >marvelous CPU I have dreamed last night)? And about TARGET_IPU
> >(the one
> >I have dreamed the night before)?
> >
> >>and is different from ripping out existing code, as you seemed
> >>to suggest
> >>for linux-user.
> >
> >Strange interpretation for "I personnally don't have a lot of interest
> >in linux-user, so I will let the linux-user maintainer (Cc) to
> >decide."
> 
> Really? I think you're completely missing my point.
> 
> Quote: "softfloat: remove HPPA specific code
> 
> We don't have any HPPA target, so let's remove HPPA specific code.
> [...]"
> 
> Quote: "> Do we want to get rid of the one remaining TARGET_HPPA
> which is in linux-user/syscall_defs.h?
> [...] I will let the linux-user maintainer (Cc) [...] decide."
> 
> I'm saying that the justification of your patch and action towards
> Riku is wrong. Patch 1/6 should be dropped, don't remove code just
> because "we don't have any HPPA target". Because obviously if anyone
> rebases hppa.git to such a master HEAD, the code will be gone just
> as well.

We have forks still using dyngen. Rebasing them against HEAD will break
them (including this HPPA target). Still you didn't complain about 
dyngen removal. And this is only an example. QEMU evolves quite fast
recently, if you don't rebase regularly, your code becomes incompatible,
and useless quite soon.

> What you've been interpreting into this is that you should go
> through hoops and newly add some TARGET_FOOBAR crap, which is not
> what I said. If you add some silence_magic_nan() and don't *know*
> how the code for HPPA should look like based on the pre-existent
> code, then I suggested to simply stub it out for such platforms
> (whether based on #elif defined(TARGET_foo) or a mere #else is
> pretty irrelevant to me). So in the end we will end up with patches
> that don't support hppa out of the box but which don't needlessly
> remove SNAN_BIT_IS_ONE and float{32,64}_default_nan(). I fail to see
> how these three macros hurt without digging deeper into your
> patches. If they do, then your patch 1/6 is lacking a proper
> description!
> 
> Point is, don't remove valuable code just because it's not being
> used in upstream *yet*.

Please define "yet". I am fine dropping this patch if we know for sure
that some progress will be done on the HPPA target. But I am not fine
keeping useless code eternally.

I propose to drop this patch for now, I'll commit it in 6 months if
nothing has changed with regards to HPPA. That will make 2 years without
activity.
Stuart Brady - Jan. 5, 2011, 11:13 p.m.
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,
Peter Maydell - Jan. 6, 2011, 8:58 a.m.
On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> 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...

>   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.

-- PMM
Andreas Färber - Jan. 6, 2011, 1:10 p.m.
Am 05.01.2011 um 11:21 schrieb Aurelien Jarno:

> On Wed, Jan 05, 2011 at 09:15:43AM +0100, Andreas Färber wrote:
>> Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:
>>
>>> HPPA code has still to be ported
>>> to TCG.
>>> Yes it still uses dyngen.
>>
>> Oh. I was pretty sure I saw TCG host support patches...
>
> Yes, there is TCG *host* code in upstream, and this code has been  
> updated
> recently.
>
> But that's not the subject we are talking about HPPA guest support.

[Thanks, I do know the difference. Obviously, by inference I assumed  
that the target would then use TCG as well. Repo doesn't make it  
obvious what the changes to qemu.git are, since it was merged, not  
rebased.]

> We have forks still using dyngen. Rebasing them against HEAD will  
> break
> them (including this HPPA target). Still you didn't complain about
> dyngen removal.

For the record, I did. Replacing dyngen with TCG was a good step, but  
it broke Darwin/ppc host for MONTHS.

I also did for kqemu. Its removal effectively means NO acceleration on  
(Open)Solaris and Haiku.

And I'm curious how Windows support will work out. We have seen some  
patches lately (one yet to be reviewed comes to mind) but no official  
maintainer yet, which was demanded by Anthony to not drop Win32. On  
the other hand, AdaCore and CodeSourcery have spoken up as commercial  
Win32 users.

Anyway, for target-ppc, me and others helped in a concerted effort to  
get things converted to TCG and reviewed. Might work for other  
projects, too.

> And this is only an example. QEMU evolves quite fast
> recently, if you don't rebase regularly, your code becomes  
> incompatible,
> and useless quite soon.

Which is why I've volunteered to step up as PReP maintainer. Can't do  
that for all projects, but we should encourage people to push their  
favorite features into upstream before things diverge more making it  
even more complicated.

This was sort-of evoked by the "1st QEMU Users Forum" thread, which  
expressed a sentiment that the qemu-devel community had shifted  
towards KVM/virtualization and were neglecting the emulation part of  
QEMU.

> I'll commit it in 6 months if
> nothing has changed with regards to HPPA.

Just pointing out that there were 5 months without activity from  
yourself last year. It happens, Real Life can be gripping.

> That will make 2 years without
> activity.

My haiku-user project is still in its infancy for 1 year now, with the  
ELF runtime_loader not yet fully ported, since I dedicated more time  
to getting ppc system emulation working. While I try to emulate ppc,  
the Haiku/ppc kernel gets neglected, etc. Two years go by really  
quickly when you're busy!

Cheers,

Andreas
Aurelien Jarno - Jan. 6, 2011, 2:35 p.m.
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 - Jan. 6, 2011, 3:08 p.m.
On Thu, Jan 06, 2011 at 02:10:36PM +0100, Andreas Färber wrote:
> Am 05.01.2011 um 11:21 schrieb Aurelien Jarno:
>> I'll commit it in 6 months if
>> nothing has changed with regards to HPPA.
>
> Just pointing out that there were 5 months without activity from  
> yourself last year. It happens, Real Life can be gripping.

Fully agreed that it can happen. During this 5 months, I have seen
a number of patches I was not very happy with. However I considered 
that not doing any QEMU work, I could not really complain about those
changes.
Peter Maydell - Jan. 6, 2011, 3:34 p.m.
On 6 January 2011 14:35, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
>>    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.

MIPS looks OK. However PPC has this in helper_compute_fprf():

    if (unlikely(float64_is_quiet_nan(farg.d))) {
        if (float64_is_signaling_nan(farg.d)) {
            /* Signaling NaN: flags are undefined */
            ret = 0x00;
        } else {
            /* Quiet NaN */
            ret = 0x11;
        }

which is definitely wrong because the first case can't be reached.
The outer if should be is_any_nan(), I think.

In helper_fnmadd() and helper_fnmsub():
        if (likely(!float64_is_quiet_nan(farg1.d)))
            farg1.d = float64_chs(farg1.d);

is I think OK but somebody else might like to check.

Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
and helper_fcmpo, efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
efsctuf and all the helper_efd* functions. I haven't actually
checked them all, but for instance efdctsi in the Power ISA
2.03 spec says "NaNs are converted as though they were zero",
but qemu's code says:
    /* NaN are not treated the same way IEEE 754 does */
    if (unlikely(float64_is_quiet_nan(u.d)))
        return 0;

which is not going to do the right thing for signaling NaNs.

-- PMM
Stuart Brady - Jan. 6, 2011, 6:13 p.m.
On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> 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,
Peter Maydell - Jan. 6, 2011, 6:43 p.m.
On 6 January 2011 18:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
>> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> 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.  */

The point of that FIXME is that it is saying "softfloat doesn't implement
the feature of flushing denormal inputs to zero". The patch implements
that feature in softfloat. Therefore the FIXME should be removed,
because it has been fixed :-)

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

As I said in the patch series cover letter, I suspect you could implement
a MIPS FCCR bit using the softfloat feature the patch adds. But that's
totally irrelevant to whether that FIXME comment should be deleted,
because that FIXME isn't about any architecture but about a softfloat
feature (or lack of it). Existing architectures are no more or less broken
than they were before, because they are unaffected by the patch series.

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

That would be all of them, I rather suspect. I don't imagine anybody
has run any of the qemu emulated models through a rigorous
validation process for any architecture, so we can't say "we know
this is correct". I'm going through fixing things for ARM as I encounter
them and I have a huge list of corner cases where we don't do the
right thing.

>> >   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've now submitted a patchset to do that.

-- PMM
Aurelien Jarno - Jan. 6, 2011, 6:48 p.m.
On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> On 6 January 2011 14:35, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
> >>    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.
> 
> MIPS looks OK. However PPC has this in helper_compute_fprf():
> 
>     if (unlikely(float64_is_quiet_nan(farg.d))) {
>         if (float64_is_signaling_nan(farg.d)) {
>             /* Signaling NaN: flags are undefined */
>             ret = 0x00;
>         } else {
>             /* Quiet NaN */
>             ret = 0x11;
>         }
> 
> which is definitely wrong because the first case can't be reached.
> The outer if should be is_any_nan(), I think.

Correct.

> In helper_fnmadd() and helper_fnmsub():
>         if (likely(!float64_is_quiet_nan(farg1.d)))
>             farg1.d = float64_chs(farg1.d);
> 
> is I think OK but somebody else might like to check.

After reading the manual again, it seems float64_is_nan() should be used
here. The idea is that fnmadd returns the negated value of fmadd, and
that NaN should be propagated as fnmadd was a single instruction. In
QEMU chs changes the sign bit even if the value is NaN. Quoting the
manual:

| This instruction produces the same result as would be obtained by 
| using the Floating Multiply-Add instruction and then negating the 
| result, with the following exceptions. 
| * QNaNs propagate with no effect on their "sign" bit.
| * QNaNs that are generated as the result of a disabled Invalid 
|   Operation Exception have a "sign" bit of 0.
| * SNaNs that are converted to QNaNs as the result of a disabled 
| Invalid Operation Exception retain the "sign”"bit of the SNaN.

> Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
> and helper_fcmpo, 

I confirm the issue for this three helpers, tested on real hardware.

> efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
> efsctuf and all the helper_efd* functions. I haven't actually
> checked them all, but for instance efdctsi in the Power ISA
> 2.03 spec says "NaNs are converted as though they were zero",
> but qemu's code says:
>     /* NaN are not treated the same way IEEE 754 does */
>     if (unlikely(float64_is_quiet_nan(u.d)))
>         return 0;
> 
> which is not going to do the right thing for signaling NaNs.
> 

Also correct.

Looks like I have read the code a bit quickly... Thanks for looking at
it, I will send a patch later.
Stuart Brady - Jan. 6, 2011, 7:25 p.m.
On Thu, Jan 06, 2011 at 06:43:28PM +0000, Peter Maydell wrote:
> On 6 January 2011 18:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> > On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
> >> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> 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.
[...]
> >   /* FIXME: Flush-To-Zero only effects results.  Denormal inputs should
> >      also be flushed to zero.  */
> 
> The point of that FIXME is that it is saying "softfloat doesn't implement
> the feature of flushing denormal inputs to zero". The patch implements
> that feature in softfloat. Therefore the FIXME should be removed,
> because it has been fixed :-)

Agreed, although note that I never insisted that the FIXME be kept in
this instance -- I was just concerned that this might be forgotten
for other targets.

If we accept that those targets are likely to be buggy, and that this
is an issue for the maintainers of those targets, that sounds fine,
but I just thought I'd ask.

Cheers,
Nathan Froyd - Jan. 6, 2011, 7:26 p.m.
On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
> and helper_fcmpo, efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
> efsctuf and all the helper_efd* functions. I haven't actually
> checked them all, but for instance efdctsi in the Power ISA
> 2.03 spec says "NaNs are converted as though they were zero",
> but qemu's code says:
>     /* NaN are not treated the same way IEEE 754 does */
>     if (unlikely(float64_is_quiet_nan(u.d)))
>         return 0;
> 
> which is not going to do the right thing for signaling NaNs.

I think you are correct about fsel, fcmpu, and fcmpo.

The E500 FP instructions are broken for various corner cases (and there
are a lot of them, because E500 is screwy).  I've been meaning to go
through and fix them up, but haven't taken the time to do so.

-Nathan
Peter Maydell - Jan. 6, 2011, 9:19 p.m.
On 6 January 2011 18:48, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
>> In helper_fnmadd() and helper_fnmsub():
>>         if (likely(!float64_is_quiet_nan(farg1.d)))
>>             farg1.d = float64_chs(farg1.d);
>>
>> is I think OK but somebody else might like to check.
>
> After reading the manual again, it seems float64_is_nan() should be used
> here. The idea is that fnmadd returns the negated value of fmadd, and
> that NaN should be propagated as fnmadd was a single instruction. In
> QEMU chs changes the sign bit even if the value is NaN. Quoting the
> manual:
>
> | This instruction produces the same result as would be obtained by
> | using the Floating Multiply-Add instruction and then negating the
> | result, with the following exceptions.
> | * QNaNs propagate with no effect on their "sign" bit.
> | * QNaNs that are generated as the result of a disabled Invalid
> |   Operation Exception have a "sign" bit of 0.
> | * SNaNs that are converted to QNaNs as the result of a disabled
> | Invalid Operation Exception retain the "sign”"bit of the SNaN.

Yes, I read that text, but I kind of convinced myself that at
that point in the code it wasn't possible to get a signalling NaN
(since they've been handled earlier). Still, the _any_nan() check
is probably clearer code.

-- PMM
Aurelien Jarno - Jan. 6, 2011, 9:31 p.m.
On Thu, Jan 06, 2011 at 09:19:13PM +0000, Peter Maydell wrote:
> On 6 January 2011 18:48, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> >> In helper_fnmadd() and helper_fnmsub():
> >>         if (likely(!float64_is_quiet_nan(farg1.d)))
> >>             farg1.d = float64_chs(farg1.d);
> >>
> >> is I think OK but somebody else might like to check.
> >
> > After reading the manual again, it seems float64_is_nan() should be used
> > here. The idea is that fnmadd returns the negated value of fmadd, and
> > that NaN should be propagated as fnmadd was a single instruction. In
> > QEMU chs changes the sign bit even if the value is NaN. Quoting the
> > manual:
> >
> > | This instruction produces the same result as would be obtained by
> > | using the Floating Multiply-Add instruction and then negating the
> > | result, with the following exceptions.
> > | * QNaNs propagate with no effect on their "sign" bit.
> > | * QNaNs that are generated as the result of a disabled Invalid
> > |   Operation Exception have a "sign" bit of 0.
> > | * SNaNs that are converted to QNaNs as the result of a disabled
> > | Invalid Operation Exception retain the "sign”"bit of the SNaN.
> 
> Yes, I read that text, but I kind of convinced myself that at
> that point in the code it wasn't possible to get a signalling NaN
> (since they've been handled earlier). Still, the _any_nan() check
> is probably clearer code.
> 

For the long term I plan to add softfloat to have more granularity in
the invalid exception flags so that we can implement most PowerPC FP
without checking the operand first, and just looking at the softfloat 
exception flags. This will use this code path, so that's better to fix
it as it is spotted.

Patch

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index f8f36f3..f23bd6a 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -30,7 +30,7 @@  these four paragraphs for those parts of this code that are retained.
 
 =============================================================================*/
 
-#if defined(TARGET_MIPS) || defined(TARGET_HPPA)
+#if defined(TARGET_MIPS)
 #define SNAN_BIT_IS_ONE		1
 #else
 #define SNAN_BIT_IS_ONE		0
@@ -63,8 +63,6 @@  typedef struct {
 #define float32_default_nan make_float32(0x7FFFFFFF)
 #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float32_default_nan make_float32(0x7FC00000)
-#elif defined(TARGET_HPPA)
-#define float32_default_nan make_float32(0x7FA00000)
 #elif SNAN_BIT_IS_ONE
 #define float32_default_nan make_float32(0x7FBFFFFF)
 #else
@@ -275,8 +273,6 @@  static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
 #define float64_default_nan make_float64(LIT64( 0x7FFFFFFFFFFFFFFF ))
 #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float64_default_nan make_float64(LIT64( 0x7FF8000000000000 ))
-#elif defined(TARGET_HPPA)
-#define float64_default_nan make_float64(LIT64( 0x7FF4000000000000 ))
 #elif SNAN_BIT_IS_ONE
 #define float64_default_nan make_float64(LIT64( 0x7FF7FFFFFFFFFFFF ))
 #else