[Qemu-devel] [PATCH 1/4 v2] target-i386: fix {min, max}{pd, ps, sd, ss} SSE2 instructions

[Aurelien Jarno <aurelien@aurel32.net>]

On Sat, Jan 07, 2012 at 08:22:53PM +0000, Peter Maydell wrote:
> On 7 January 2012 20:09, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > minpd, minps, minsd, minss and maxpd, maxps, maxsd, maxss SSE2
> > instructions have been broken when switching target-i386 to softfloat.
> > It's not possible to use comparison instructions on float types anymore
> > to softfloat, so use the floatXX_min anf floatXX_max functions instead.
> 
> Nope, this gets the x86 special cases wrong. This has been discussed
> here before:
> 
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg85557.html
> has the right implementation (from Jason Wessell) and a comment
> (from me) about why it's right.
> 

Good catch, the patch below should implement the correct behaviour.

target-i386: fix {min,max}{pd,ps,sd,ss} SSE2 instructions

minpd, minps, minsd, minss and maxpd, maxps, maxsd, maxss SSE2
instructions have been broken when switching target-i386 to softfloat.
It's not possible to use comparison instructions on float types anymore
to softfloat, so use the floatXX_lt function instead, as the
float_XX_min and float_XX_max functions can't be used due to the Intel
specific behaviour.

As it implements the correct NaNs behaviour, let's remove the
corresponding entry from the TODO.

It fixes GDM screen display on Debian Lenny.

Thanks to Peter Maydell and Jason Wessel for their analysis of the
problem.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-i386/TODO      |    1 -
 target-i386/ops_sse.h |    9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-i386/TODO b/target-i386/TODO
index c8ada07..a8d69cf 100644
--- a/target-i386/TODO
+++ b/target-i386/TODO
@@ -15,7 +15,6 @@ Correctness issues:
 - DRx register support
 - CR0.AC emulation
 - SSE alignment checks
-- fix SSE min/max with nans
 
 Optimizations/Features:
 
diff --git a/target-i386/ops_sse.h b/target-i386/ops_sse.h
index 47dde78..8ed231d 100644
--- a/target-i386/ops_sse.h
+++ b/target-i386/ops_sse.h
@@ -584,10 +584,15 @@ 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_SQRT(size, a, b) float ## size ## _sqrt(b, &env->sse_status)
 
+/* 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.
+ */
+#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)
+
 SSE_HELPER_S(add, FPU_ADD)
 SSE_HELPER_S(sub, FPU_SUB)
 SSE_HELPER_S(mul, FPU_MUL)
-- 
1.7.7.3


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net