Skip to contentSkip to conversation

target-ppc: fix fcmp{o,u} instructions

PATCH Applied Nathan Froyd 2 participants 5 messages Dec 14, 2008

lore
Share & tools
lore

Nathan Froyd

author
#0 lore raw

PATCH 1/1 target-ppc: fix fcmp{o,u} instructions

The instructions are specified to update the condition register even if an error is to be signaled because of NaN input.

Signed-off-by Nathan Froyd <[email protected]>
---
 target-ppc/helper.h    |    4 +-
 target-ppc/op_helper.c |   56 ++++++++++++++++++++++++++---------------------
 target-ppc/translate.c |    8 +++---
 3 files changed, 37 insertions(+), 31 deletions(-)
target-ppc/helper.h +2 -2
index 26d1627..07d8f8a 100644
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
 DEF_HELPER_1(float64_to_float32, i32, i64)
 DEF_HELPER_1(float32_to_float64, i64, i32)
 
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
 
 DEF_HELPER_1(fctiw, i64, i64)
 DEF_HELPER_1(fctiwz, i64, i64)
target-ppc/op_helper.c +31 -25

87 lines not on this page · raw message

target-ppc/translate.c +4 -4
index afe0265..cabebc0 100644
@@ -2274,8 +2274,8 @@ GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+                     crfD(ctx->opcode));
     gen_helper_float_check_status();
 }
 

@@ -2287,8 +2287,8 @@ GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+                     crfD(ctx->opcode));
     gen_helper_float_check_status();
 }
Signature · 1 line
1.6.0.5
#1 lore raw
Quoted 4 lines · Nathan Froyd

The instructions are specified to update the condition register even if an error is to be signaled because of NaN input.

Signed-off-by Nathan Froyd <[email protected]>

Please find my comments inline.

Quoted 54 lines
---
 target-ppc/helper.h    |    4 +-
 target-ppc/op_helper.c |   56 ++++++++++++++++++++++++++---------------------
 target-ppc/translate.c |    8 +++---
 3 files changed, 37 insertions(+), 31 deletions(-)
target-ppc/helper.h +2 -2
index 26d1627..07d8f8a 100644
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
 DEF_HELPER_1(float64_to_float32, i32, i64)
 DEF_HELPER_1(float32_to_float64, i64, i32)
 
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
 
 DEF_HELPER_1(fctiw, i64, i64)
 DEF_HELPER_1(fctiwz, i64, i64)
target-ppc/op_helper.c +8 -10
index cd88bb6..c002e60 100644
@@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         return arg3;
 }
 
-uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
+void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
 {
     CPU_DoubleU farg1, farg2;
     uint32_t ret = 0;
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN comparison */
-        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+    if (unlikely(float64_is_nan(farg1.d) ||
+                 float64_is_nan(farg2.d))) {
+        ret = 0x01UL;
+    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x08UL;
+    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x04UL;
     } else {
-        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x08UL;
-        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x04UL;
-        } else {

target-ppc/op_helper.c uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3) · line 1626

Quoted 10 lines
target-ppc/op_helper.c +4 -3
-            ret = 0x02UL;
-        }
+        ret = 0x02UL;
     }
+
     env->fpscr &= ~(0x0F << FPSCR_FPRF);
     env->fpscr |= ret << FPSCR_FPRF;
-    return ret;
+    env->crf[crfD] = ret;
+    if (unlikely(ret == 0x01UL

Why do you need to test if the result is 0x01UL?

target-ppc/op_helper.c uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3) · line 1628

Quoted 2 lines
target-ppc/op_helper.c +2 -0
+                 && (float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d)))) {

If at least one of the arguments is a sNaN, the result should already by 0x01.

Quoted 20 lines
target-ppc/op_helper.c +10 -1
+        /* sNaN comparison */
+        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+    }
 }
 
-uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
+void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32 crfD)
 {
     CPU_DoubleU farg1, farg2;
     uint32_t ret = 0;

@@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
 
     if (unlikely(float64_is_nan(farg1.d) ||
                  float64_is_nan(farg2.d))) {
+        ret = 0x01UL;
+    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x08UL;
+    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x04UL;
+    } else {

target-ppc/op_helper.c uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2) · line 1658

Quoted 10 lines
target-ppc/op_helper.c +7 -0
+        ret = 0x02UL;
+    }
+
+    env->fpscr &= ~(0x0F << FPSCR_FPRF);
+    env->fpscr |= ret << FPSCR_FPRF;
+    env->crf[crfD] = ret;
+    if (unlikely (ret == 0x01UL)) {
         if (float64_is_signaling_nan(farg1.d) ||
             float64_is_signaling_nan(farg2.d)) {
             /* sNaN comparison */

Same here.

Quoted 51 lines
target-ppc/op_helper.c +0 -11
@@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
             /* qNaN comparison */
             fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
         }
-    } else {
-        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x08UL;
-        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x04UL;
-        } else {
-            ret = 0x02UL;
-        }
     }
-    env->fpscr &= ~(0x0F << FPSCR_FPRF);
-    env->fpscr |= ret << FPSCR_FPRF;
-    return ret;
 }
 
 #if !defined (CONFIG_USER_ONLY)
target-ppc/translate.c +4 -4
index afe0265..cabebc0 100644
@@ -2274,8 +2274,8 @@ GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+                     crfD(ctx->opcode));
     gen_helper_float_check_status();
 }
 

@@ -2287,8 +2287,8 @@ GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+                     crfD(ctx->opcode));
     gen_helper_float_check_status();
 }
Signature · 1 line
1.6.0.5
Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net
author
#2 lore raw
Quoted 38 lines · Aurelien Jarno
Quoted 25 lines
target-ppc/op_helper.c +8 -10
-uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
+void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
 {
     CPU_DoubleU farg1, farg2;
     uint32_t ret = 0;
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN comparison */
-        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+    if (unlikely(float64_is_nan(farg1.d) ||
+                 float64_is_nan(farg2.d))) {
+        ret = 0x01UL;
+    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x08UL;
+    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x04UL;
     } else {
-        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x08UL;
-        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x04UL;
-        } else {

target-ppc/op_helper.c uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3) · line 1626

Quoted 10 lines
target-ppc/op_helper.c +4 -3
-            ret = 0x02UL;
-        }
+        ret = 0x02UL;
     }
+
     env->fpscr &= ~(0x0F << FPSCR_FPRF);
     env->fpscr |= ret << FPSCR_FPRF;
-    return ret;
+    env->crf[crfD] = ret;
+    if (unlikely(ret == 0x01UL

Why do you need to test if the result is 0x01UL?

Quoted 5 lines

target-ppc/op_helper.c uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3) · line 1628

Quoted 2 lines
target-ppc/op_helper.c +2 -0
+                 && (float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d)))) {

If at least one of the arguments is a sNaN, the result should already by 0x01.

My reasoning was that testing for NaNness would be relatively expensive and that NaN input should be infrequent. Testing for 0x01UL would therefore be a quick check to see if we need to do more expensive checks. I'm happy to redo the patch in the interest of clarity.

Quoted 6 lines
Quoted 4 lines
+    if (unlikely (ret == 0x01UL)) {
         if (float64_is_signaling_nan(farg1.d) ||
             float64_is_signaling_nan(farg2.d)) {
             /* sNaN comparison */

Same here.

Ditto.

-Nathan

author
#3 lore raw
In reply to Aurelien Jarno
Quoted 1 line · Aurelien Jarno

Please find my comments inline.

Aurelien also pointed out an error on IRC: we need to use real TCG temporaries for the crfD value, not the integers themselves. Fixed thusly (sorry, not exactly sure how to do this cleanly for git's sake).

-Nathan

The instructions are specified to update the condition register even if an error is to be signaled because of NaN input.

Signed-off-by Nathan Froyd <[email protected]>
---
 target-ppc/helper.h    |    4 +-
 target-ppc/op_helper.c |   56 ++++++++++++++++++++++++++---------------------
 target-ppc/translate.c |   12 ++++++---
 3 files changed, 41 insertions(+), 31 deletions(-)
target-ppc/helper.h +2 -2
index 26d1627..07d8f8a 100644
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
 DEF_HELPER_1(float64_to_float32, i32, i64)
 DEF_HELPER_1(float32_to_float64, i64, i32)
 
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
 
 DEF_HELPER_1(fctiw, i64, i64)
 DEF_HELPER_1(fctiwz, i64, i64)
target-ppc/op_helper.c +31 -25

87 lines not on this page · raw message

target-ppc/translate.c +8 -4
index 69801d0..17e5fd7 100644
@@ -2269,26 +2269,30 @@ GEN_FLOAT_B(rim, 0x08, 0x0F, 1, PPC_FLOAT_EXT);
 /* fcmpo */
 GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
 {
+    TCGv crf;
     if (unlikely(!ctx->fpu_enabled)) {
         gen_exception(ctx, POWERPC_EXCP_FPU);
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    crf = tcg_const_i32(crfD(ctx->opcode));
+    gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+    tcg_temp_free(crf);
     gen_helper_float_check_status();
 }
 
 /* fcmpu */
 GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
 {
+    TCGv crf;
     if (unlikely(!ctx->fpu_enabled)) {
         gen_exception(ctx, POWERPC_EXCP_FPU);
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    crf = tcg_const_i32(crfD(ctx->opcode));
+    gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+    tcg_temp_free(crf);
     gen_helper_float_check_status();
 }
Signature · 1 line
1.6.0.5
Applied
#4 lore raw
Quoted 7 lines · Nathan Froyd
Quoted 1 line · Aurelien Jarno

Please find my comments inline.

Aurelien also pointed out an error on IRC: we need to use real TCG temporaries for the crfD value, not the integers themselves. Fixed thusly (sorry, not exactly sure how to do this cleanly for git's sake).

Quoted 6 lines

-Nathan

The instructions are specified to update the condition register even if an error is to be signaled because of NaN input.

Signed-off-by Nathan Froyd <[email protected]>

Thanks, applied.

Quoted 160 lines
---
 target-ppc/helper.h    |    4 +-
 target-ppc/op_helper.c |   56 ++++++++++++++++++++++++++---------------------
 target-ppc/translate.c |   12 ++++++---
 3 files changed, 41 insertions(+), 31 deletions(-)
target-ppc/helper.h +2 -2
index 26d1627..07d8f8a 100644
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
 DEF_HELPER_1(float64_to_float32, i32, i64)
 DEF_HELPER_1(float32_to_float64, i64, i32)
 
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
 
 DEF_HELPER_1(fctiw, i64, i64)
 DEF_HELPER_1(fctiwz, i64, i64)
target-ppc/op_helper.c +31 -25
index cd88bb6..c0b4c70 100644
@@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         return arg3;
 }
 
-uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
+void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
 {
     CPU_DoubleU farg1, farg2;
     uint32_t ret = 0;
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN comparison */
-        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+    if (unlikely(float64_is_nan(farg1.d) ||
+                 float64_is_nan(farg2.d))) {
+        ret = 0x01UL;
+    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x08UL;
+    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x04UL;
     } else {
-        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x08UL;
-        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x04UL;
-        } else {
-            ret = 0x02UL;
-        }
+        ret = 0x02UL;
     }
+
     env->fpscr &= ~(0x0F << FPSCR_FPRF);
     env->fpscr |= ret << FPSCR_FPRF;
-    return ret;
+    env->crf[crfD] = ret;
+    if (unlikely(ret == 0x01UL
+                 && (float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d)))) {
+        /* sNaN comparison */
+        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+    }
 }
 
-uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
+void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32_t crfD)
 {
     CPU_DoubleU farg1, farg2;
     uint32_t ret = 0;

@@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
 
     if (unlikely(float64_is_nan(farg1.d) ||
                  float64_is_nan(farg2.d))) {
+        ret = 0x01UL;
+    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x08UL;
+    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+        ret = 0x04UL;
+    } else {
+        ret = 0x02UL;
+    }
+
+    env->fpscr &= ~(0x0F << FPSCR_FPRF);
+    env->fpscr |= ret << FPSCR_FPRF;
+    env->crf[crfD] = ret;
+    if (unlikely (ret == 0x01UL)) {
         if (float64_is_signaling_nan(farg1.d) ||
             float64_is_signaling_nan(farg2.d)) {
             /* sNaN comparison */

@@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
             /* qNaN comparison */
             fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
         }
-    } else {
-        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x08UL;
-        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
-            ret = 0x04UL;
-        } else {
-            ret = 0x02UL;
-        }
     }
-    env->fpscr &= ~(0x0F << FPSCR_FPRF);
-    env->fpscr |= ret << FPSCR_FPRF;
-    return ret;
 }
 
 #if !defined (CONFIG_USER_ONLY)
target-ppc/translate.c +8 -4
index 69801d0..17e5fd7 100644
@@ -2269,26 +2269,30 @@ GEN_FLOAT_B(rim, 0x08, 0x0F, 1, PPC_FLOAT_EXT);
 /* fcmpo */
 GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
 {
+    TCGv crf;
     if (unlikely(!ctx->fpu_enabled)) {
         gen_exception(ctx, POWERPC_EXCP_FPU);
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    crf = tcg_const_i32(crfD(ctx->opcode));
+    gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+    tcg_temp_free(crf);
     gen_helper_float_check_status();
 }
 
 /* fcmpu */
 GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
 {
+    TCGv crf;
     if (unlikely(!ctx->fpu_enabled)) {
         gen_exception(ctx, POWERPC_EXCP_FPU);
         return;
     }
     gen_reset_fpstatus();
-    gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
-                     cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+    crf = tcg_const_i32(crfD(ctx->opcode));
+    gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+    tcg_temp_free(crf);
     gen_helper_float_check_status();
 }
Signature · 1 line
1.6.0.5
Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net