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

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

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

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

[Laurent Desnogues]

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

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

[Jason Wessel]

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

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

[Peter Maydell]

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

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

[Jason Wessel]

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.

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

[Peter Maydell]

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