[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] fix fdiv instruction
|
From: |
Programmingkid |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] fix fdiv instruction |
|
Date: |
Sat, 30 Jun 2018 16:54:37 -0400 |
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 <address@hidden>
>> ---
>> 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