From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53402) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrT2r-0007v0-Fc for qemu-devel@nongnu.org; Thu, 20 Nov 2014 09:49:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrT2j-0002gC-Vf for qemu-devel@nongnu.org; Thu, 20 Nov 2014 09:49:17 -0500 Message-ID: <546DFF63.8090406@suse.de> Date: Thu, 20 Nov 2014 15:49:07 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1415828764-10582-1-git-send-email-tommusta@gmail.com> <1415828764-10582-3-git-send-email-tommusta@gmail.com> <546DF731.2060303@suse.de> <546DFB87.80609@gmail.com> In-Reply-To: <546DFB87.80609@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tom Musta , qemu-devel@nongnu.org, qemu-ppc@nongnu.org On 20.11.14 15:32, Tom Musta wrote: > On 11/20/2014 8:14 AM, Alexander Graf wrote: >> >> >> On 12.11.14 22:46, Tom Musta wrote: >>> The Floating Point Move instructions (fmr., fabs., fnabs., fneg., >>> and fcpsgn.) incorrectly copy FPSCR[FPCC] instead of [FX,FEX,VX,OX]. >>> Furthermore, the current code does this via a call to gen_compute_fprf, >>> which is awkward since these instructions do not actually set FPRF. >>> >>> Change the code to use the gen_set_cr1_from_fpscr utility. >>> >>> Signed-off-by: Tom Musta >>> --- >>> target-ppc/translate.c | 50 ++++++++++++++++++++++++++++------------------- >>> 1 files changed, 30 insertions(+), 20 deletions(-) >>> >>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >>> index 910ce56..2d79e39 100644 >>> --- a/target-ppc/translate.c >>> +++ b/target-ppc/translate.c >>> @@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx) >>> } >>> #endif >>> >>> +#if defined(TARGET_PPC64) >>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx) >>> +{ >>> + TCGv_i32 tmp = tcg_temp_new_i32(); >>> + tcg_gen_trunc_tl_i32(tmp, cpu_fpscr); >>> + tcg_gen_shri_i32(cpu_crf[1], tmp, 28); >>> + tcg_temp_free_i32(tmp); >>> +} >>> +#else >>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx) >>> +{ >>> + tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28); >>> +} >>> +#endif >>> + >>> /*** Floating-Point arithmetic ***/ >>> #define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type) \ >>> static void gen_f##name(DisasContext *ctx) \ >>> @@ -2370,7 +2385,9 @@ static void gen_fabs(DisasContext *ctx) >>> } >>> tcg_gen_andi_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)], >>> ~(1ULL << 63)); >>> - gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0); >>> + if (unlikely(Rc(ctx->opcode))) { >>> + gen_set_cr1_from_fpscr(ctx); >>> + } >> >> I don't quite understand this. We set cr1 based on fpscr, but we don't >> recalculate the respective fpscr bits? >> >> Wouldn't we get outdated comparison data? >> >> >> Alex >> > > Nope. > > The floating point move instructions don't actually even alter the FPSCR. From the ISA (see the last sentence): > > 4.6.5 Floating-Point Move Instructions > These instructions copy data from one floating-point > register to another, altering the sign bit (bit 0) as > described below for fneg, fabs, fnabs, and fcpsgn. > These instructions treat NaNs just like any other kind of > value (e.g., the sign bit of a NaN may be altered by > fneg, fabs, fnabs, and fcpsgn). These instructions do > not alter the FPSCR. Thanks, applied with the following squashed in: diff --git a/target-ppc/translate.c b/target-ppc/translate.c index 7aef089..35c3a16 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -2088,7 +2088,7 @@ static void gen_set_cr1_from_fpscr(DisasContext *ctx) #else static void gen_set_cr1_from_fpscr(DisasContext *ctx) { - tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28); + tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28); } #endif Alex