[v2] fix fdiv instruction

Commit Message

Programmingkid June 30, 2018, 8:54 p.m. UTC

On Jun 30, 2018, at 11:43 AM, Richard Henderson wrote:

> On 06/29/2018 08:06 PM, John Arbuckle wrote:
>> When the fdiv instruction divides a finite number by zero,
>> the result actually depends on the FPSCR[ZE] bit. If this
>> bit is set, the return value is the value originally in
>> the destination register. If it is not set
>> the result should be either positive or negative infinity.
>> The sign of this result would depend on the sign of the
>> two inputs. What currently happens is only infinity is
>> returned even if the FPSCR[ZE] bit is set. This patch
>> fixes this problem by actually checking the FPSCR[ZE] bit
>> when deciding what the answer should be.
>> 
>> fdiv is suppose to only set the FPSCR's FPRF bits during a
>> division by zero situation when the FPSCR[ZE] is not set.
>> What currently happens is these bits are always set. This
>> patch fixes this problem by checking the FPSCR[ZE] bit to
>> decide if the FPRF bits should be set. 
>> 
>> https://www.pdfdrive.net/powerpc-microprocessor-family-the-programming-environments-for-32-e3087633.html
>> This document has the information on the fdiv. Page 133 has
>> the information on what action is executed when a division
>> by zero situation takes place.
>> 
>> Signed-off-by: John Arbuckle <[e-mail]>
>> ---
>> v2 changes:
>> - Added comment for computing sign bit.
>> - Changed return value of helper_fdiv() to return the
>>  original value in the destination register when the
>>  fpscr_ze if condition is encountered.
>> - Patch comment adjusted to reflect returning
>>  destination register's value instead of zero.
>> 
>> target/ppc/fpu_helper.c            | 21 ++++++++++++++++++++-
>> target/ppc/helper.h                |  2 +-
>> target/ppc/translate/fp-impl.inc.c | 29 ++++++++++++++++++++++++++++-
>> 3 files changed, 49 insertions(+), 3 deletions(-)
>> 
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 7714bfe0f9..9ccba1ec3f 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -644,7 +644,8 @@ uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
>> }
>> 
>> /* fdiv - fdiv. */
>> -uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
>> +uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2, uint64_t
>> +                     old_value)
> 
> You don't need to pass in the old value,
> 
>> +    } else if (arg2 == 0) {
>> +        /* Division by zero */
>> +        float_zero_divide_excp(env, GETPC());
>> +        if (fpscr_ze) { /* if zero divide exception is enabled */
>> +            /* Keep the value in the destination register the same */
>> +            farg1.ll = old_value;
>> +        } else {
>> +            /* Compute sign bit */
>> +            uint64_t sign = (farg1.ll ^ farg2.ll) >> 63;
>> +            if (sign) { /* Negative sign bit */
>> +                farg1.ll = 0xfff0000000000000; /* Negative Infinity */
>> +            } else { /* Positive sign bit */
>> +                farg1.ll = 0x7ff0000000000000; /* Positive Infinity */
>> +            }
>> +            helper_compute_fprf_float64(env, farg1.d);
> 
> You don't need any of this.
> 
>>         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
>> +        helper_compute_fprf_float64(env, farg1.d);
>> +        helper_float_check_status(env);
> 
> You merely need to raise the exception here, which skips
> the code that assigns a new value to the register.
> 
> You do not want to do *all* of do_float_check_status here,
> because overflow and underflow and inexact exceptions *do*
> write a new value to the destination register (although a
> weird scaled value that we don't handle so far, but still
> an assignment, so the exception must be raised as a separate
> step after assignment is complete.)
> 
> So, you just need to move the call to float_zero_divide_excp
> out of do_float_check_status to here.  Like
> 
>    if (unlikely(get_float_exception_flags(&env->fp_status)
>                 & float_flag_divbyzero)) {
>        float_zero_divide_excp(env, GETPC());
>    }
> 
> 
> r~

I tried your suggestion but could not figure out how to prevent the destination register's value from being changed. Could you know the exact code that sets this value? 

I did manage to simply my patch more, but as-is it doesn't solve the problem of leaving the destination register's value alone when it needs to. 

Here is the new patch I am working on:

---
 target/ppc/fpu_helper.c            | 13 ++++++++++---
 target/ppc/translate/fp-impl.inc.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 4 deletions(-)

Patch

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 7714bfe..3939983 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -534,9 +534,7 @@  static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
     CPUState *cs = CPU(ppc_env_get_cpu(env));
     int status = get_float_exception_flags(&env->fp_status);
 
-    if (status & float_flag_divbyzero) {
-        float_zero_divide_excp(env, raddr);
-    } else if (status & float_flag_overflow) {
+    if (status & float_flag_overflow) {
         float_overflow_excp(env);
     } else if (status & float_flag_underflow) {
         float_underflow_excp(env);
@@ -664,7 +662,16 @@  uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
             /* sNaN division */
             float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
         }
+        if (arg2 == 0) {
+            float_zero_divide_excp(env, GETPC());
+        }
         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
+        if (!(fpscr_ze && arg2 == 0)) {
+            helper_compute_fprf_float64(env, farg1.d);
+        }
+        if (arg2 != 0) {
+            helper_float_check_status(env);
+        }
     }
 
     return farg1.ll;
diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c
index 2fbd4d4..9b1ea68 100644
--- a/target/ppc/translate/fp-impl.inc.c
+++ b/target/ppc/translate/fp-impl.inc.c
@@ -84,6 +84,31 @@  static void gen_f##name(DisasContext *ctx)                                    \
 _GEN_FLOAT_AB(name, name, 0x3F, op2, inval, 0, set_fprf, type);               \
 _GEN_FLOAT_AB(name##s, name, 0x3B, op2, inval, 1, set_fprf, type);
 
+
+#define _GEN_FLOAT_DIV(name, op, op1, op2, inval, isfloat, set_fprf, type)    \
+static void gen_f##name(DisasContext *ctx)                                    \
+{                                                                             \
+    if (unlikely(!ctx->fpu_enabled)) {                                        \
+        gen_exception(ctx, POWERPC_EXCP_FPU);                                 \
+        return;                                                               \
+    }                                                                         \
+    gen_reset_fpstatus();                                                     \
+    gen_helper_f##op(cpu_fpr[rD(ctx->opcode)], cpu_env,                       \
+                     cpu_fpr[rA(ctx->opcode)],                                \
+                     cpu_fpr[rB(ctx->opcode)]);                               \
+    if (isfloat) {                                                            \
+        gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
+                        cpu_fpr[rD(ctx->opcode)]);                            \
+    }                                                                         \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr(ctx);                                          \
+    }                                                                         \
+}
+
+#define GEN_FLOAT_DIV(name, op2, inval, set_fprf, type)                       \
+_GEN_FLOAT_DIV(name, name, 0x3F, op2, inval, 0, set_fprf, type);              \
+_GEN_FLOAT_DIV(name##s, name, 0x3B, op2, inval, 1, set_fprf, type);
+
 #define _GEN_FLOAT_AC(name, op, op1, op2, inval, isfloat, set_fprf, type)     \
 static void gen_f##name(DisasContext *ctx)                                    \
 {                                                                             \
@@ -149,7 +174,7 @@  static void gen_f##name(DisasContext *ctx)                                    \
 /* fadd - fadds */
 GEN_FLOAT_AB(add, 0x15, 0x000007C0, 1, PPC_FLOAT);
 /* fdiv - fdivs */
-GEN_FLOAT_AB(div, 0x12, 0x000007C0, 1, PPC_FLOAT);
+GEN_FLOAT_DIV(div, 0x12, 0x000007C0, 1, PPC_FLOAT);
 /* fmul - fmuls */
 GEN_FLOAT_AC(mul, 0x19, 0x0000F800, 1, PPC_FLOAT);