Patchwork [17/19] target-ppc: fix SPE comparison functions

login
register
mail settings
Submitter Aurelien Jarno
Date April 12, 2011, 9:59 p.m.
Message ID <1302645571-20500-18-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/90930/
State New
Headers show

Comments

Aurelien Jarno - April 12, 2011, 9:59 p.m.
efstst*() functions are fast SPE funtions which do not take into account
special values (infinites, NaN, etc.), while efscmp*() functions are
IEEE754 compliant.

Given that float32_*() functions are IEEE754 compliant, the efscmp*()
functions are correctly implemented, while efstst*() are not. This
patch reverse the implementation of this two groups of functions and
fix the comments. It also use float32_eq() instead of float32_eq_quiet()
as qNaNs should not be ignored.

Cc: Alexander Graf <agraf@suse.de>
Cc: Nathan Froyd <froydnj@codesourcery.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
(cherry picked from commit 9f4055a7d1426c6a1f6b11725e3458ada4cbc5eb)
---
 target-ppc/op_helper.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
Nathan Froyd - April 13, 2011, 2:40 a.m.
On Tue, Apr 12, 2011 at 11:59:29PM +0200, Aurelien Jarno wrote:
> Given that float32_*() functions are IEEE754 compliant, the efscmp*()
> functions are correctly implemented, while efstst*() are not. This
> patch reverse the implementation of this two groups of functions and
> fix the comments. It also use float32_eq() instead of float32_eq_quiet()
> as qNaNs should not be ignored.

Thanks for addressing this; the E500 emulation in QEMU is more like how
we wish the hardware acted, rather than how it actually acts. :)

It's late here, but I think this change:

> -static inline uint32_t efscmplt(uint32_t op1, uint32_t op2)
> +static inline uint32_t efststlt(uint32_t op1, uint32_t op2)
>  {
> -    /* XXX: TODO: test special values (NaN, infinites, ...) */
> +    /* XXX: TODO: ignore special values (NaN, infinites, ...) */
>      return efststlt(op1, op2);
>  }

is not correct, as you've just turned this into an infinite (inlined!)
loop.  You'd want to change the efststlt call into an efscmplt call.
Similarly for efstst{gt,eq}.

-Nathan
Aurelien Jarno - April 13, 2011, 6:11 p.m.
On Tue, Apr 12, 2011 at 07:40:33PM -0700, Nathan Froyd wrote:
> On Tue, Apr 12, 2011 at 11:59:29PM +0200, Aurelien Jarno wrote:
> > Given that float32_*() functions are IEEE754 compliant, the efscmp*()
> > functions are correctly implemented, while efstst*() are not. This
> > patch reverse the implementation of this two groups of functions and
> > fix the comments. It also use float32_eq() instead of float32_eq_quiet()
> > as qNaNs should not be ignored.
> 
> Thanks for addressing this; the E500 emulation in QEMU is more like how
> we wish the hardware acted, rather than how it actually acts. :)
> 
> It's late here, but I think this change:
> 
> > -static inline uint32_t efscmplt(uint32_t op1, uint32_t op2)
> > +static inline uint32_t efststlt(uint32_t op1, uint32_t op2)
> >  {
> > -    /* XXX: TODO: test special values (NaN, infinites, ...) */
> > +    /* XXX: TODO: ignore special values (NaN, infinites, ...) */
> >      return efststlt(op1, op2);
> >  }
> 
> is not correct, as you've just turned this into an infinite (inlined!)
> loop.  You'd want to change the efststlt call into an efscmplt call.
> Similarly for efstst{gt,eq}.
> 

Thanks for catching that, I'll fix that in the next version.

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index f645f57..696958a 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -3331,7 +3331,7 @@  HELPER_SPE_VECTOR_ARITH(fsmul);
 HELPER_SPE_VECTOR_ARITH(fsdiv);
 
 /* Single-precision floating-point comparisons */
-static inline uint32_t efststlt(uint32_t op1, uint32_t op2)
+static inline uint32_t efscmplt(uint32_t op1, uint32_t op2)
 {
     CPU_FloatU u1, u2;
     u1.l = op1;
@@ -3339,7 +3339,7 @@  static inline uint32_t efststlt(uint32_t op1, uint32_t op2)
     return float32_lt(u1.f, u2.f, &env->vec_status) ? 4 : 0;
 }
 
-static inline uint32_t efststgt(uint32_t op1, uint32_t op2)
+static inline uint32_t efscmpgt(uint32_t op1, uint32_t op2)
 {
     CPU_FloatU u1, u2;
     u1.l = op1;
@@ -3347,29 +3347,29 @@  static inline uint32_t efststgt(uint32_t op1, uint32_t op2)
     return float32_le(u1.f, u2.f, &env->vec_status) ? 0 : 4;
 }
 
-static inline uint32_t efststeq(uint32_t op1, uint32_t op2)
+static inline uint32_t efscmpeq(uint32_t op1, uint32_t op2)
 {
     CPU_FloatU u1, u2;
     u1.l = op1;
     u2.l = op2;
-    return float32_eq_quiet(u1.f, u2.f, &env->vec_status) ? 4 : 0;
+    return float32_eq(u1.f, u2.f, &env->vec_status) ? 4 : 0;
 }
 
-static inline uint32_t efscmplt(uint32_t op1, uint32_t op2)
+static inline uint32_t efststlt(uint32_t op1, uint32_t op2)
 {
-    /* XXX: TODO: test special values (NaN, infinites, ...) */
+    /* XXX: TODO: ignore special values (NaN, infinites, ...) */
     return efststlt(op1, op2);
 }
 
-static inline uint32_t efscmpgt(uint32_t op1, uint32_t op2)
+static inline uint32_t efststgt(uint32_t op1, uint32_t op2)
 {
-    /* XXX: TODO: test special values (NaN, infinites, ...) */
+    /* XXX: TODO: ignore special values (NaN, infinites, ...) */
     return efststgt(op1, op2);
 }
 
-static inline uint32_t efscmpeq(uint32_t op1, uint32_t op2)
+static inline uint32_t efststeq(uint32_t op1, uint32_t op2)
 {
-    /* XXX: TODO: test special values (NaN, infinites, ...) */
+    /* XXX: TODO: ignore special values (NaN, infinites, ...) */
     return efststeq(op1, op2);
 }