* [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction @ 2011-11-08 13:00 Jason Wessel 2011-11-08 13:48 ` Laurent Desnogues 0 siblings, 1 reply; 6+ messages in thread From: Jason Wessel @ 2011-11-08 13:00 UTC (permalink / raw) To: qemu-devel The maxsd instruction needs to take into account the sign of the numbers 64 bit numbers. This is a regression that was introduced in 347ac8e356 (target-i386: switch to softfloat). The case that fails is: maxsd %xmm1,%xmm0 When xmm1 = 24 and xmm0 = -100 This was found running the glib2 binding tests where it prints the message: /binding/transform: GLib-GObject-WARNING **: value "24.000000" of type `gdouble' is invalid or out of range for property `value' of type `gdouble' aborting... Using a signed comparison fixes the problem. Signed-off-by: Jason Wessel <jason.wessel@windriver.com> --- target-i386/ops_sse.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h index aa41d25..bcc0ed9 100644 --- a/target-i386/ops_sse.h +++ b/target-i386/ops_sse.h @@ -584,8 +584,8 @@ void helper_ ## name ## sd (Reg *d, Reg *s)\ #define FPU_SUB(size, a, b) float ## size ## _sub(a, b, &env->sse_status) #define FPU_MUL(size, a, b) float ## size ## _mul(a, b, &env->sse_status) #define FPU_DIV(size, a, b) float ## size ## _div(a, b, &env->sse_status) -#define FPU_MIN(size, a, b) (a) < (b) ? (a) : (b) -#define FPU_MAX(size, a, b) (a) > (b) ? (a) : (b) +#define FPU_MIN(size, a, b) (int ## size ## _t)(a) < (int ## size ## _t)(b) ? (a) : (b) +#define FPU_MAX(size, a, b) (int ## size ## _t)(a) > (int ## size ## _t)(b) ? (a) : (b) #define FPU_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status) SSE_HELPER_S(add, FPU_ADD) -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction 2011-11-08 13:00 [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction Jason Wessel @ 2011-11-08 13:48 ` Laurent Desnogues 2011-11-08 14:22 ` Jason Wessel 0 siblings, 1 reply; 6+ messages in thread From: Laurent Desnogues @ 2011-11-08 13:48 UTC (permalink / raw) To: Jason Wessel; +Cc: qemu-devel On Tue, Nov 8, 2011 at 2:00 PM, Jason Wessel <jason.wessel@windriver.com> wrote: > The maxsd instruction needs to take into account the sign of the > numbers 64 bit numbers. This is a regression that was introduced in > 347ac8e356 (target-i386: switch to softfloat). > > The case that fails is: > > maxsd %xmm1,%xmm0 > > When xmm1 = 24 and xmm0 = -100 > > This was found running the glib2 binding tests where it prints the message: > /binding/transform: > GLib-GObject-WARNING **: value "24.000000" of type `gdouble' is invalid or out of range for property `value' of type `gdouble' > aborting... > > Using a signed comparison fixes the problem. > > Signed-off-by: Jason Wessel <jason.wessel@windriver.com> > --- > target-i386/ops_sse.h | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h > index aa41d25..bcc0ed9 100644 > --- a/target-i386/ops_sse.h > +++ b/target-i386/ops_sse.h > @@ -584,8 +584,8 @@ void helper_ ## name ## sd (Reg *d, Reg *s)\ > #define FPU_SUB(size, a, b) float ## size ## _sub(a, b, &env->sse_status) > #define FPU_MUL(size, a, b) float ## size ## _mul(a, b, &env->sse_status) > #define FPU_DIV(size, a, b) float ## size ## _div(a, b, &env->sse_status) > -#define FPU_MIN(size, a, b) (a) < (b) ? (a) : (b) > -#define FPU_MAX(size, a, b) (a) > (b) ? (a) : (b) Isn't maxsd a floating-point instruction? If so, shouldn't FPU_{MIN,MAX} use softfloat operations? Laurent > +#define FPU_MIN(size, a, b) (int ## size ## _t)(a) < (int ## size ## _t)(b) ? (a) : (b) > +#define FPU_MAX(size, a, b) (int ## size ## _t)(a) > (int ## size ## _t)(b) ? (a) : (b) > #define FPU_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status) > > SSE_HELPER_S(add, FPU_ADD) > -- > 1.7.1 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction 2011-11-08 13:48 ` Laurent Desnogues @ 2011-11-08 14:22 ` Jason Wessel 2011-11-08 14:40 ` Peter Maydell 0 siblings, 1 reply; 6+ messages in thread From: Jason Wessel @ 2011-11-08 14:22 UTC (permalink / raw) To: Laurent Desnogues; +Cc: qemu-devel On 11/08/2011 07:48 AM, Laurent Desnogues wrote: > On Tue, Nov 8, 2011 at 2:00 PM, Jason Wessel <jason.wessel@windriver.com> wrote: >> The maxsd instruction needs to take into account the sign of the >> numbers 64 bit numbers. This is a regression that was introduced in >> 347ac8e356 (target-i386: switch to softfloat). >> >> The case that fails is: >> >> maxsd %xmm1,%xmm0 >> >> When xmm1 = 24 and xmm0 = -100 >> >> This was found running the glib2 binding tests where it prints the message: >> /binding/transform: >> GLib-GObject-WARNING **: value "24.000000" of type `gdouble' is invalid or out of range for property `value' of type `gdouble' >> aborting... >> >> Using a signed comparison fixes the problem. >> >> Signed-off-by: Jason Wessel <jason.wessel@windriver.com> >> --- >> target-i386/ops_sse.h | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h >> index aa41d25..bcc0ed9 100644 >> --- a/target-i386/ops_sse.h >> +++ b/target-i386/ops_sse.h >> @@ -584,8 +584,8 @@ void helper_ ## name ## sd (Reg *d, Reg *s)\ >> #define FPU_SUB(size, a, b) float ## size ## _sub(a, b, &env->sse_status) >> #define FPU_MUL(size, a, b) float ## size ## _mul(a, b, &env->sse_status) >> #define FPU_DIV(size, a, b) float ## size ## _div(a, b, &env->sse_status) >> -#define FPU_MIN(size, a, b) (a) < (b) ? (a) : (b) >> -#define FPU_MAX(size, a, b) (a) > (b) ? (a) : (b) > Isn't maxsd a floating-point instruction? If so, shouldn't > FPU_{MIN,MAX} use softfloat operations? You are correct. It should be: +#define FPU_MIN(size, a, b) float ## size ## _lt(a, b, &env->sse_status) ? (a) : (b) +#define FPU_MAX(size, a, b) float ## size ## _lt(b, a, &env->sse_status) ? (a) : (b) Jason. > > > Laurent > >> +#define FPU_MIN(size, a, b) (int ## size ## _t)(a) < (int ## size ## _t)(b) ? (a) : (b) >> +#define FPU_MAX(size, a, b) (int ## size ## _t)(a) > (int ## size ## _t)(b) ? (a) : (b) >> #define FPU_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status) >> >> SSE_HELPER_S(add, FPU_ADD) >> -- >> 1.7.1 >> >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction 2011-11-08 14:22 ` Jason Wessel @ 2011-11-08 14:40 ` Peter Maydell 2011-11-08 14:45 ` Jason Wessel 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2011-11-08 14:40 UTC (permalink / raw) To: Jason Wessel; +Cc: Laurent Desnogues, qemu-devel On 8 November 2011 14:22, Jason Wessel <jason.wessel@windriver.com> wrote: > +#define FPU_MIN(size, a, b) float ## size ## _lt(a, b, &env->sse_status) ? (a) : (b) > +#define FPU_MAX(size, a, b) float ## size ## _lt(b, a, &env->sse_status) ? (a) : (b) This will give the wrong answers for special cases involving +0, -0 and NaNs. Check the intel architecture manual which says how these should work. (You can't use float*_min() and float*_max() either, as those have sane semantics and you need to implement the Intel ones here.) -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction 2011-11-08 14:40 ` Peter Maydell @ 2011-11-08 14:45 ` Jason Wessel 2011-11-08 15:19 ` Peter Maydell 0 siblings, 1 reply; 6+ messages in thread From: Jason Wessel @ 2011-11-08 14:45 UTC (permalink / raw) To: Peter Maydell; +Cc: Laurent Desnogues, qemu-devel On 11/08/2011 08:40 AM, Peter Maydell wrote: > On 8 November 2011 14:22, Jason Wessel <jason.wessel@windriver.com> wrote: >> +#define FPU_MIN(size, a, b) float ## size ## _lt(a, b, &env->sse_status) ? (a) : (b) >> +#define FPU_MAX(size, a, b) float ## size ## _lt(b, a, &env->sse_status) ? (a) : (b) > This will give the wrong answers for special cases involving +0, -0 > and NaNs. Check the intel architecture manual which says how these > should work. > > (You can't use float*_min() and float*_max() either, as those have > sane semantics and you need to implement the Intel ones here.) Anyone out there know how to fix this the right way? I do not have a point of reference as to how to properly implement this, I just stumbled on the fact it was completely broken since the switch from hardfloat to softfloat between qemu 0.14 and 0.15. It certainly appears there is more to this problem than meets the eye. :-) Jason. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction 2011-11-08 14:45 ` Jason Wessel @ 2011-11-08 15:19 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2011-11-08 15:19 UTC (permalink / raw) To: Jason Wessel; +Cc: Laurent Desnogues, qemu-devel On 8 November 2011 14:45, Jason Wessel <jason.wessel@windriver.com> wrote: > On 11/08/2011 08:40 AM, Peter Maydell wrote: >> On 8 November 2011 14:22, Jason Wessel <jason.wessel@windriver.com> wrote: >>> +#define FPU_MIN(size, a, b) float ## size ## _lt(a, b, &env->sse_status) ? (a) : (b) >>> +#define FPU_MAX(size, a, b) float ## size ## _lt(b, a, &env->sse_status) ? (a) : (b) >> This will give the wrong answers for special cases involving +0, -0 >> and NaNs. Check the intel architecture manual which says how these >> should work. >> >> (You can't use float*_min() and float*_max() either, as those have >> sane semantics and you need to implement the Intel ones here.) > > Anyone out there know how to fix this the right way? Having mused about it a bit, I think that actually the macros there do return the right answers for the special cases :-) If (a,b) are +0,-0 in some order, then the _lt comparison will treat them as equal and return 0, so we return b, as required. If either of (a,b) are NaNs then the _lt comparison will raise InvalidOp and return 0, so we return b. That's a bit subtle, so I think it probably deserves a comment: /* Note that the choice of comparison op here is important to get the * special cases right: for min and max Intel specifies that (-0,0), * (NaN, anything) and (anything, NaN) return the second argument. */ Sorry for the false alarm. -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-08 15:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-08 13:00 [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction Jason Wessel 2011-11-08 13:48 ` Laurent Desnogues 2011-11-08 14:22 ` Jason Wessel 2011-11-08 14:40 ` Peter Maydell 2011-11-08 14:45 ` Jason Wessel 2011-11-08 15:19 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).