[Qemu-devel] [PATCH v2] fix fdiv instruction

[Qemu-devel] [PATCH v2] fix fdiv instruction

[John Arbuckle]

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 <programmingkidx@gmail.com>
---
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)
 {
     CPU_DoubleU farg1, farg2;
 
@@ -658,6 +659,22 @@ uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
     } else if (unlikely(float64_is_zero(farg1.d) && float64_is_zero(farg2.d))) {
         /* Division of zero by zero */
         farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXZDZ, 1);
+    } 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);
+        }
     } else {
         if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
                      float64_is_signaling_nan(farg2.d, &env->fp_status))) {
@@ -665,6 +682,8 @@ uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
             float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
         }
         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
+        helper_compute_fprf_float64(env, farg1.d);
+        helper_float_check_status(env);
     }
 
     return farg1.ll;
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index d751f0e219..ced3e99d71 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -88,7 +88,7 @@ DEF_HELPER_2(frim, i64, env, i64)
 DEF_HELPER_3(fadd, i64, env, i64, i64)
 DEF_HELPER_3(fsub, i64, env, i64, i64)
 DEF_HELPER_3(fmul, i64, env, i64, i64)
-DEF_HELPER_3(fdiv, i64, env, i64, i64)
+DEF_HELPER_4(fdiv, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmadd, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmsub, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmadd, i64, env, i64, i64, i64)
diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c
index 2fbd4d4f38..fa29616b87 100644
--- a/target/ppc/translate/fp-impl.inc.c
+++ b/target/ppc/translate/fp-impl.inc.c
@@ -84,6 +84,33 @@ 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)],                                \
+                     cpu_fpr[rD(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 +176,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);
 
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-devel] [PATCH v2] fix fdiv instruction

[Richard Henderson]

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 <programmingkidx@gmail.com>
> ---
> 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~

Re: [Qemu-devel] [PATCH v2] fix fdiv instruction

[Programmingkid]

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 <programmingkidx@gmail.com>
>> ---
>> 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(-)

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);
 
-- 
2.7.2

Re: [Qemu-devel] [PATCH v2] fix fdiv instruction

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180630030606.17288-1-programmingkidx@gmail.com
Subject: [Qemu-devel] [PATCH v2] fix fdiv instruction

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c839af6c62 fix fdiv instruction

=== OUTPUT BEGIN ===
Checking PATCH 1/1: fix fdiv instruction...
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#120: FILE: target/ppc/translate/fp-impl.inc.c:109:
+#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);

total: 1 errors, 0 warnings, 88 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com