From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrSmz-0002z0-LB for qemu-devel@nongnu.org; Thu, 20 Nov 2014 09:33:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrSmq-0005ZR-Io for qemu-devel@nongnu.org; Thu, 20 Nov 2014 09:32:53 -0500 Message-ID: <546DFB87.80609@gmail.com> Date: Thu, 20 Nov 2014 08:32:39 -0600 From: Tom Musta 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> In-Reply-To: <546DF731.2060303@suse.de> 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: Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org 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.