[Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2

[Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2

[Jason Wessel]

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

Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] target-i386: Fix regression with maxsd SSE2 instruction v2

[Peter Maydell]

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