* [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2 @ 2011-11-08 14:20 Jason Wessel 2011-11-19 16:20 ` Peter Maydell 0 siblings, 1 reply; 3+ messages in thread From: Jason Wessel @ 2011-11-08 14:20 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..58f7bf5 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) 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) #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] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2 2011-11-08 14:20 [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2 Jason Wessel @ 2011-11-19 16:20 ` Peter Maydell 2011-12-14 14:55 ` Peter Maydell 0 siblings, 1 reply; 3+ messages in thread From: Peter Maydell @ 2011-11-19 16:20 UTC (permalink / raw) To: Jason Wessel; +Cc: qemu-devel On 8 November 2011 14:20, 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. This commit comment needs to be updated -- we're not using a signed comparison, we're using a floating point comparison. > diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h > index aa41d25..58f7bf5 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) 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) > #define FPU_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status) (repeating my comments from the other thread): 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), * (0,-0), (NaN, anything) and (anything, NaN) return the second argument. */ -- PMM ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2 2011-11-19 16:20 ` Peter Maydell @ 2011-12-14 14:55 ` Peter Maydell 0 siblings, 0 replies; 3+ messages in thread From: Peter Maydell @ 2011-12-14 14:55 UTC (permalink / raw) To: Jason Wessel; +Cc: qemu-devel Jason, ping? Are you going to do a v3 of this patch? Thanks -- PMM On 19 November 2011 16:20, Peter Maydell <peter.maydell@linaro.org> wrote: > On 8 November 2011 14:20, 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. > > This commit comment needs to be updated -- we're not using a > signed comparison, we're using a floating point comparison. > >> diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h >> index aa41d25..58f7bf5 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) 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) >> #define FPU_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status) > > (repeating my comments from the other thread): > > 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), > * (0,-0), (NaN, anything) and (anything, NaN) return the second argument. > */ > > -- PMM ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-12-14 14:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-08 14:20 [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2 Jason Wessel 2011-11-19 16:20 ` Peter Maydell 2011-12-14 14:55 ` 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).