* [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
@ 2008-12-14 2:56 Nathan Froyd
2008-12-14 10:35 ` Aurelien Jarno
0 siblings, 1 reply; 5+ messages in thread
From: Nathan Froyd @ 2008-12-14 2:56 UTC (permalink / raw)
To: qemu-devel
The instructions are specified to update the condition register even if
an error is to be signaled because of NaN input.
Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
---
target-ppc/helper.h | 4 +-
target-ppc/op_helper.c | 56 ++++++++++++++++++++++++++---------------------
target-ppc/translate.c | 8 +++---
3 files changed, 37 insertions(+), 31 deletions(-)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 26d1627..07d8f8a 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
DEF_HELPER_1(float64_to_float32, i32, i64)
DEF_HELPER_1(float32_to_float64, i64, i32)
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
DEF_HELPER_1(fctiw, i64, i64)
DEF_HELPER_1(fctiwz, i64, i64)
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index cd88bb6..c002e60 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
return arg3;
}
-uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
+void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
{
CPU_DoubleU farg1, farg2;
uint32_t ret = 0;
farg1.ll = arg1;
farg2.ll = arg2;
- if (unlikely(float64_is_signaling_nan(farg1.d) ||
- float64_is_signaling_nan(farg2.d))) {
- /* sNaN comparison */
- fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+ if (unlikely(float64_is_nan(farg1.d) ||
+ float64_is_nan(farg2.d))) {
+ ret = 0x01UL;
+ } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x08UL;
+ } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x04UL;
} else {
- if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x08UL;
- } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x04UL;
- } else {
- ret = 0x02UL;
- }
+ ret = 0x02UL;
}
+
env->fpscr &= ~(0x0F << FPSCR_FPRF);
env->fpscr |= ret << FPSCR_FPRF;
- return ret;
+ env->crf[crfD] = ret;
+ if (unlikely(ret == 0x01UL
+ && (float64_is_signaling_nan(farg1.d) ||
+ float64_is_signaling_nan(farg2.d)))) {
+ /* sNaN comparison */
+ fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+ }
}
-uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
+void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32 crfD)
{
CPU_DoubleU farg1, farg2;
uint32_t ret = 0;
@@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
if (unlikely(float64_is_nan(farg1.d) ||
float64_is_nan(farg2.d))) {
+ ret = 0x01UL;
+ } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x08UL;
+ } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x04UL;
+ } else {
+ ret = 0x02UL;
+ }
+
+ env->fpscr &= ~(0x0F << FPSCR_FPRF);
+ env->fpscr |= ret << FPSCR_FPRF;
+ env->crf[crfD] = ret;
+ if (unlikely (ret == 0x01UL)) {
if (float64_is_signaling_nan(farg1.d) ||
float64_is_signaling_nan(farg2.d)) {
/* sNaN comparison */
@@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
/* qNaN comparison */
fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
}
- } else {
- if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x08UL;
- } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x04UL;
- } else {
- ret = 0x02UL;
- }
}
- env->fpscr &= ~(0x0F << FPSCR_FPRF);
- env->fpscr |= ret << FPSCR_FPRF;
- return ret;
}
#if !defined (CONFIG_USER_ONLY)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index afe0265..cabebc0 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2274,8 +2274,8 @@ GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
return;
}
gen_reset_fpstatus();
- gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
- cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+ gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+ crfD(ctx->opcode));
gen_helper_float_check_status();
}
@@ -2287,8 +2287,8 @@ GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
return;
}
gen_reset_fpstatus();
- gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
- cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+ gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
+ crfD(ctx->opcode));
gen_helper_float_check_status();
}
--
1.6.0.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
2008-12-14 2:56 [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions Nathan Froyd
@ 2008-12-14 10:35 ` Aurelien Jarno
2008-12-14 12:51 ` Nathan Froyd
2008-12-14 19:10 ` Nathan Froyd
0 siblings, 2 replies; 5+ messages in thread
From: Aurelien Jarno @ 2008-12-14 10:35 UTC (permalink / raw)
To: Nathan Froyd; +Cc: qemu-devel
On Sat, Dec 13, 2008 at 06:56:28PM -0800, Nathan Froyd wrote:
> The instructions are specified to update the condition register even if
> an error is to be signaled because of NaN input.
>
> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
Please find my comments inline.
> ---
> target-ppc/helper.h | 4 +-
> target-ppc/op_helper.c | 56 ++++++++++++++++++++++++++---------------------
> target-ppc/translate.c | 8 +++---
> 3 files changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 26d1627..07d8f8a 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
> DEF_HELPER_1(float64_to_float32, i32, i64)
> DEF_HELPER_1(float32_to_float64, i64, i32)
>
> -DEF_HELPER_2(fcmpo, i32, i64, i64)
> -DEF_HELPER_2(fcmpu, i32, i64, i64)
> +DEF_HELPER_3(fcmpo, void, i64, i64, i32)
> +DEF_HELPER_3(fcmpu, void, i64, i64, i32)
>
> DEF_HELPER_1(fctiw, i64, i64)
> DEF_HELPER_1(fctiwz, i64, i64)
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index cd88bb6..c002e60 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
> return arg3;
> }
>
> -uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
> +void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> {
> CPU_DoubleU farg1, farg2;
> uint32_t ret = 0;
> farg1.ll = arg1;
> farg2.ll = arg2;
>
> - if (unlikely(float64_is_signaling_nan(farg1.d) ||
> - float64_is_signaling_nan(farg2.d))) {
> - /* sNaN comparison */
> - fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> + if (unlikely(float64_is_nan(farg1.d) ||
> + float64_is_nan(farg2.d))) {
> + ret = 0x01UL;
> + } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x08UL;
> + } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x04UL;
> } else {
> - if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x08UL;
> - } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x04UL;
> - } else {
> - ret = 0x02UL;
> - }
> + ret = 0x02UL;
> }
> +
> env->fpscr &= ~(0x0F << FPSCR_FPRF);
> env->fpscr |= ret << FPSCR_FPRF;
> - return ret;
> + env->crf[crfD] = ret;
> + if (unlikely(ret == 0x01UL
Why do you need to test if the result is 0x01UL?
> + && (float64_is_signaling_nan(farg1.d) ||
> + float64_is_signaling_nan(farg2.d)))) {
If at least one of the arguments is a sNaN, the result should already by
0x01.
> + /* sNaN comparison */
> + fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> + }
> }
>
> -uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
> +void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32 crfD)
> {
> CPU_DoubleU farg1, farg2;
> uint32_t ret = 0;
> @@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
>
> if (unlikely(float64_is_nan(farg1.d) ||
> float64_is_nan(farg2.d))) {
> + ret = 0x01UL;
> + } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x08UL;
> + } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x04UL;
> + } else {
> + ret = 0x02UL;
> + }
> +
> + env->fpscr &= ~(0x0F << FPSCR_FPRF);
> + env->fpscr |= ret << FPSCR_FPRF;
> + env->crf[crfD] = ret;
> + if (unlikely (ret == 0x01UL)) {
> if (float64_is_signaling_nan(farg1.d) ||
> float64_is_signaling_nan(farg2.d)) {
> /* sNaN comparison */
Same here.
> @@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
> /* qNaN comparison */
> fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
> }
> - } else {
> - if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x08UL;
> - } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x04UL;
> - } else {
> - ret = 0x02UL;
> - }
> }
> - env->fpscr &= ~(0x0F << FPSCR_FPRF);
> - env->fpscr |= ret << FPSCR_FPRF;
> - return ret;
> }
>
> #if !defined (CONFIG_USER_ONLY)
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index afe0265..cabebc0 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -2274,8 +2274,8 @@ GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
> return;
> }
> gen_reset_fpstatus();
> - gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
> - cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
> + gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
> + crfD(ctx->opcode));
> gen_helper_float_check_status();
> }
>
> @@ -2287,8 +2287,8 @@ GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
> return;
> }
> gen_reset_fpstatus();
> - gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
> - cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
> + gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
> + crfD(ctx->opcode));
> gen_helper_float_check_status();
> }
>
> --
> 1.6.0.5
>
>
>
>
--
.''`. Aurelien Jarno | GPG: 1024D/F1BCDB73
: :' : Debian developer | Electrical Engineer
`. `' aurel32@debian.org | aurelien@aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
2008-12-14 10:35 ` Aurelien Jarno
@ 2008-12-14 12:51 ` Nathan Froyd
2008-12-14 19:10 ` Nathan Froyd
1 sibling, 0 replies; 5+ messages in thread
From: Nathan Froyd @ 2008-12-14 12:51 UTC (permalink / raw)
To: qemu-devel
On Sun, Dec 14, 2008 at 11:35:54AM +0100, Aurelien Jarno wrote:
> > -uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
> > +void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> > {
> > CPU_DoubleU farg1, farg2;
> > uint32_t ret = 0;
> > farg1.ll = arg1;
> > farg2.ll = arg2;
> >
> > - if (unlikely(float64_is_signaling_nan(farg1.d) ||
> > - float64_is_signaling_nan(farg2.d))) {
> > - /* sNaN comparison */
> > - fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > + if (unlikely(float64_is_nan(farg1.d) ||
> > + float64_is_nan(farg2.d))) {
> > + ret = 0x01UL;
> > + } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > + ret = 0x08UL;
> > + } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > + ret = 0x04UL;
> > } else {
> > - if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > - ret = 0x08UL;
> > - } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > - ret = 0x04UL;
> > - } else {
> > - ret = 0x02UL;
> > - }
> > + ret = 0x02UL;
> > }
> > +
> > env->fpscr &= ~(0x0F << FPSCR_FPRF);
> > env->fpscr |= ret << FPSCR_FPRF;
> > - return ret;
> > + env->crf[crfD] = ret;
> > + if (unlikely(ret == 0x01UL
>
> Why do you need to test if the result is 0x01UL?
>
> > + && (float64_is_signaling_nan(farg1.d) ||
> > + float64_is_signaling_nan(farg2.d)))) {
>
> If at least one of the arguments is a sNaN, the result should already by
> 0x01.
My reasoning was that testing for NaNness would be relatively expensive
and that NaN input should be infrequent. Testing for 0x01UL would
therefore be a quick check to see if we need to do more expensive
checks. I'm happy to redo the patch in the interest of clarity.
> > + if (unlikely (ret == 0x01UL)) {
> > if (float64_is_signaling_nan(farg1.d) ||
> > float64_is_signaling_nan(farg2.d)) {
> > /* sNaN comparison */
>
> Same here.
Ditto.
-Nathan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
2008-12-14 10:35 ` Aurelien Jarno
2008-12-14 12:51 ` Nathan Froyd
@ 2008-12-14 19:10 ` Nathan Froyd
2008-12-14 19:34 ` Aurelien Jarno
1 sibling, 1 reply; 5+ messages in thread
From: Nathan Froyd @ 2008-12-14 19:10 UTC (permalink / raw)
To: qemu-devel
On Sun, Dec 14, 2008 at 11:35:54AM +0100, Aurelien Jarno wrote:
> Please find my comments inline.
Aurelien also pointed out an error on IRC: we need to use real TCG
temporaries for the crfD value, not the integers themselves. Fixed
thusly (sorry, not exactly sure how to do this cleanly for git's sake).
-Nathan
The instructions are specified to update the condition register even if
an error is to be signaled because of NaN input.
Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
---
target-ppc/helper.h | 4 +-
target-ppc/op_helper.c | 56 ++++++++++++++++++++++++++---------------------
target-ppc/translate.c | 12 ++++++---
3 files changed, 41 insertions(+), 31 deletions(-)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 26d1627..07d8f8a 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
DEF_HELPER_1(float64_to_float32, i32, i64)
DEF_HELPER_1(float32_to_float64, i64, i32)
-DEF_HELPER_2(fcmpo, i32, i64, i64)
-DEF_HELPER_2(fcmpu, i32, i64, i64)
+DEF_HELPER_3(fcmpo, void, i64, i64, i32)
+DEF_HELPER_3(fcmpu, void, i64, i64, i32)
DEF_HELPER_1(fctiw, i64, i64)
DEF_HELPER_1(fctiwz, i64, i64)
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index cd88bb6..c0b4c70 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
return arg3;
}
-uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
+void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
{
CPU_DoubleU farg1, farg2;
uint32_t ret = 0;
farg1.ll = arg1;
farg2.ll = arg2;
- if (unlikely(float64_is_signaling_nan(farg1.d) ||
- float64_is_signaling_nan(farg2.d))) {
- /* sNaN comparison */
- fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+ if (unlikely(float64_is_nan(farg1.d) ||
+ float64_is_nan(farg2.d))) {
+ ret = 0x01UL;
+ } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x08UL;
+ } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x04UL;
} else {
- if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x08UL;
- } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x04UL;
- } else {
- ret = 0x02UL;
- }
+ ret = 0x02UL;
}
+
env->fpscr &= ~(0x0F << FPSCR_FPRF);
env->fpscr |= ret << FPSCR_FPRF;
- return ret;
+ env->crf[crfD] = ret;
+ if (unlikely(ret == 0x01UL
+ && (float64_is_signaling_nan(farg1.d) ||
+ float64_is_signaling_nan(farg2.d)))) {
+ /* sNaN comparison */
+ fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+ }
}
-uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
+void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32_t crfD)
{
CPU_DoubleU farg1, farg2;
uint32_t ret = 0;
@@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
if (unlikely(float64_is_nan(farg1.d) ||
float64_is_nan(farg2.d))) {
+ ret = 0x01UL;
+ } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x08UL;
+ } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
+ ret = 0x04UL;
+ } else {
+ ret = 0x02UL;
+ }
+
+ env->fpscr &= ~(0x0F << FPSCR_FPRF);
+ env->fpscr |= ret << FPSCR_FPRF;
+ env->crf[crfD] = ret;
+ if (unlikely (ret == 0x01UL)) {
if (float64_is_signaling_nan(farg1.d) ||
float64_is_signaling_nan(farg2.d)) {
/* sNaN comparison */
@@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
/* qNaN comparison */
fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
}
- } else {
- if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x08UL;
- } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
- ret = 0x04UL;
- } else {
- ret = 0x02UL;
- }
}
- env->fpscr &= ~(0x0F << FPSCR_FPRF);
- env->fpscr |= ret << FPSCR_FPRF;
- return ret;
}
#if !defined (CONFIG_USER_ONLY)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 69801d0..17e5fd7 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2269,26 +2269,30 @@ GEN_FLOAT_B(rim, 0x08, 0x0F, 1, PPC_FLOAT_EXT);
/* fcmpo */
GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
{
+ TCGv crf;
if (unlikely(!ctx->fpu_enabled)) {
gen_exception(ctx, POWERPC_EXCP_FPU);
return;
}
gen_reset_fpstatus();
- gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
- cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+ crf = tcg_const_i32(crfD(ctx->opcode));
+ gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+ tcg_temp_free(crf);
gen_helper_float_check_status();
}
/* fcmpu */
GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
{
+ TCGv crf;
if (unlikely(!ctx->fpu_enabled)) {
gen_exception(ctx, POWERPC_EXCP_FPU);
return;
}
gen_reset_fpstatus();
- gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
- cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
+ crf = tcg_const_i32(crfD(ctx->opcode));
+ gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
+ tcg_temp_free(crf);
gen_helper_float_check_status();
}
--
1.6.0.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
2008-12-14 19:10 ` Nathan Froyd
@ 2008-12-14 19:34 ` Aurelien Jarno
0 siblings, 0 replies; 5+ messages in thread
From: Aurelien Jarno @ 2008-12-14 19:34 UTC (permalink / raw)
To: Nathan Froyd; +Cc: qemu-devel
On Sun, Dec 14, 2008 at 11:10:46AM -0800, Nathan Froyd wrote:
> On Sun, Dec 14, 2008 at 11:35:54AM +0100, Aurelien Jarno wrote:
> > Please find my comments inline.
>
> Aurelien also pointed out an error on IRC: we need to use real TCG
> temporaries for the crfD value, not the integers themselves. Fixed
> thusly (sorry, not exactly sure how to do this cleanly for git's sake).
>
> -Nathan
>
> The instructions are specified to update the condition register even if
> an error is to be signaled because of NaN input.
>
> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
Thanks, applied.
> ---
> target-ppc/helper.h | 4 +-
> target-ppc/op_helper.c | 56 ++++++++++++++++++++++++++---------------------
> target-ppc/translate.c | 12 ++++++---
> 3 files changed, 41 insertions(+), 31 deletions(-)
>
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 26d1627..07d8f8a 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -62,8 +62,8 @@ DEF_HELPER_1(fpscr_setbit, void, i32)
> DEF_HELPER_1(float64_to_float32, i32, i64)
> DEF_HELPER_1(float32_to_float64, i64, i32)
>
> -DEF_HELPER_2(fcmpo, i32, i64, i64)
> -DEF_HELPER_2(fcmpu, i32, i64, i64)
> +DEF_HELPER_3(fcmpo, void, i64, i64, i32)
> +DEF_HELPER_3(fcmpu, void, i64, i64, i32)
>
> DEF_HELPER_1(fctiw, i64, i64)
> DEF_HELPER_1(fctiwz, i64, i64)
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index cd88bb6..c0b4c70 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -1602,32 +1602,36 @@ uint64_t helper_fsel (uint64_t arg1, uint64_t arg2, uint64_t arg3)
> return arg3;
> }
>
> -uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
> +void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> {
> CPU_DoubleU farg1, farg2;
> uint32_t ret = 0;
> farg1.ll = arg1;
> farg2.ll = arg2;
>
> - if (unlikely(float64_is_signaling_nan(farg1.d) ||
> - float64_is_signaling_nan(farg2.d))) {
> - /* sNaN comparison */
> - fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> + if (unlikely(float64_is_nan(farg1.d) ||
> + float64_is_nan(farg2.d))) {
> + ret = 0x01UL;
> + } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x08UL;
> + } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x04UL;
> } else {
> - if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x08UL;
> - } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x04UL;
> - } else {
> - ret = 0x02UL;
> - }
> + ret = 0x02UL;
> }
> +
> env->fpscr &= ~(0x0F << FPSCR_FPRF);
> env->fpscr |= ret << FPSCR_FPRF;
> - return ret;
> + env->crf[crfD] = ret;
> + if (unlikely(ret == 0x01UL
> + && (float64_is_signaling_nan(farg1.d) ||
> + float64_is_signaling_nan(farg2.d)))) {
> + /* sNaN comparison */
> + fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> + }
> }
>
> -uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
> +void helper_fcmpo (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> {
> CPU_DoubleU farg1, farg2;
> uint32_t ret = 0;
> @@ -1636,6 +1640,19 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
>
> if (unlikely(float64_is_nan(farg1.d) ||
> float64_is_nan(farg2.d))) {
> + ret = 0x01UL;
> + } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x08UL;
> + } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> + ret = 0x04UL;
> + } else {
> + ret = 0x02UL;
> + }
> +
> + env->fpscr &= ~(0x0F << FPSCR_FPRF);
> + env->fpscr |= ret << FPSCR_FPRF;
> + env->crf[crfD] = ret;
> + if (unlikely (ret == 0x01UL)) {
> if (float64_is_signaling_nan(farg1.d) ||
> float64_is_signaling_nan(farg2.d)) {
> /* sNaN comparison */
> @@ -1645,18 +1662,7 @@ uint32_t helper_fcmpo (uint64_t arg1, uint64_t arg2)
> /* qNaN comparison */
> fload_invalid_op_excp(POWERPC_EXCP_FP_VXVC);
> }
> - } else {
> - if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x08UL;
> - } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> - ret = 0x04UL;
> - } else {
> - ret = 0x02UL;
> - }
> }
> - env->fpscr &= ~(0x0F << FPSCR_FPRF);
> - env->fpscr |= ret << FPSCR_FPRF;
> - return ret;
> }
>
> #if !defined (CONFIG_USER_ONLY)
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 69801d0..17e5fd7 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -2269,26 +2269,30 @@ GEN_FLOAT_B(rim, 0x08, 0x0F, 1, PPC_FLOAT_EXT);
> /* fcmpo */
> GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT)
> {
> + TCGv crf;
> if (unlikely(!ctx->fpu_enabled)) {
> gen_exception(ctx, POWERPC_EXCP_FPU);
> return;
> }
> gen_reset_fpstatus();
> - gen_helper_fcmpo(cpu_crf[crfD(ctx->opcode)],
> - cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
> + crf = tcg_const_i32(crfD(ctx->opcode));
> + gen_helper_fcmpo(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
> + tcg_temp_free(crf);
> gen_helper_float_check_status();
> }
>
> /* fcmpu */
> GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT)
> {
> + TCGv crf;
> if (unlikely(!ctx->fpu_enabled)) {
> gen_exception(ctx, POWERPC_EXCP_FPU);
> return;
> }
> gen_reset_fpstatus();
> - gen_helper_fcmpu(cpu_crf[crfD(ctx->opcode)],
> - cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
> + crf = tcg_const_i32(crfD(ctx->opcode));
> + gen_helper_fcmpu(cpu_fpr[rA(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], crf);
> + tcg_temp_free(crf);
> gen_helper_float_check_status();
> }
>
> --
> 1.6.0.5
>
>
>
>
--
.''`. Aurelien Jarno | GPG: 1024D/F1BCDB73
: :' : Debian developer | Electrical Engineer
`. `' aurel32@debian.org | aurelien@aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-12-14 19:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-14 2:56 [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions Nathan Froyd
2008-12-14 10:35 ` Aurelien Jarno
2008-12-14 12:51 ` Nathan Froyd
2008-12-14 19:10 ` Nathan Froyd
2008-12-14 19:34 ` Aurelien Jarno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).