fix fdiv instruction
PATCH v2 1/1 fix fdiv instruction
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.
---
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(-)
target/ppc/fpu_helper.c +20 -1
@@ -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;
target/ppc/helper.h +1 -1
@@ -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)
target/ppc/translate/fp-impl.inc.c +28 -1
@@ -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 );
Signature · 1 line
2.14.3 (Apple Git-98)
Quoted 42 lines · 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 <[email protected]>---
v2 changes:
- Added comment for computing sign bit.
- Changed return value ofhelper_fdiv()to return the original value in the destination register when thefpscr_zeif 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(-)target/ppc/fpu_helper.c +0
-0
target/ppc/fpu_helper.c uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2) · line 648
Quoted 6 lines
target/ppc/fpu_helper.c +2
-1@@ -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,
Quoted 5 lines
target/ppc/fpu_helper.c +5
-0+ }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 */
target/ppc/fpu_helper.c uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2) · line 676
Quoted 10 lines
target/ppc/fpu_helper.c +10
-0+ 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.
target/ppc/fpu_helper.c uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2) · line 686
Quoted 3 lines
farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);target/ppc/fpu_helper.c +2
-0+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~
Quoted 87 lines · Richard Henderson
Quoted 42 lines · 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 <[email protected]>---
v2 changes:
- Added comment for computing sign bit.
- Changed return value ofhelper_fdiv()to return the original value in the destination register when thefpscr_zeif 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(-)target/ppc/fpu_helper.c +0
-0target/ppc/fpu_helper.c uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2) · line 648
Quoted 6 lines
target/ppc/fpu_helper.c +2
-1@@ -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,
Quoted 5 lines
target/ppc/fpu_helper.c +5
-0+ }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 */ target/ppc/fpu_helper.c uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
Quoted 10 lines
target/ppc/fpu_helper.c +10
-0+ 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.
target/ppc/fpu_helper.c
Quoted 3 lines
farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);target/ppc/fpu_helper.c +2
-0+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_statushere, 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_excpout ofdo_float_check_statusto here. Like
Quoted 7 lines
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(-)
target/ppc/fpu_helper.c +10 -3
@@ -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;
target/ppc/translate/fp-impl.inc.c +26 -1
@@ -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 );
Signature · 1 line
2.7.2
This series seems to have some coding style problems. See output below for more information:
Hi,
This series seems to have some coding style problems. See output below for more information:
Type: series
Message-id: [email protected]
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 [email protected]