| Message ID | 20180630030606.17288-1-programmingkidx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fix fdiv instruction | expand |
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~
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);
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
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);
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(-)