From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBwjw-0004NG-Ko for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:34:56 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBwjv-0004Mt-Pb for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:34:56 -0500 Received: from [199.232.76.173] (port=50022 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBwjv-0004Mm-I2 for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:34:55 -0500 Received: from hall.aurel32.net ([88.191.82.174]:60436) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LBwju-0001IG-W4 for qemu-devel@nongnu.org; Sun, 14 Dec 2008 14:34:55 -0500 Date: Sun, 14 Dec 2008 20:34:51 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions Message-ID: <20081214193451.GA8523@volta.aurel32.net> References: <20081214025628.GE23471@codesourcery.com> <20081214103554.GG17729@volta.aurel32.net> <20081214191046.GI23471@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20081214191046.GI23471@codesourcery.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nathan Froyd Cc: qemu-devel@nongnu.org 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 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