| Message ID | 1294065273-30274-2-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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 > >
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.
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
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."
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
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.
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,
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
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
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.
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.
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
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,
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
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.
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,
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
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
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.
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
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(-)