From patchwork Sat Jun 30 20:54:37 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Programmingkid X-Patchwork-Id: 10498429 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 614ED603B4 for ; Sat, 30 Jun 2018 20:55:37 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4C98128E23 for ; Sat, 30 Jun 2018 20:55:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3CB1B28E25; Sat, 30 Jun 2018 20:55:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 7BD2728E23 for ; Sat, 30 Jun 2018 20:55:36 +0000 (UTC) Received: from localhost ([::1]:47983 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZMu7-0006wl-CD for patchwork-qemu-devel@patchwork.kernel.org; Sat, 30 Jun 2018 16:55:35 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58263) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZMtH-0006QQ-Tu for qemu-devel@nongnu.org; Sat, 30 Jun 2018 16:54:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fZMtE-0002lL-Nx for qemu-devel@nongnu.org; Sat, 30 Jun 2018 16:54:43 -0400 Received: from mail-it0-x242.google.com ([2607:f8b0:4001:c0b::242]:50772) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fZMtE-0002kt-HJ; Sat, 30 Jun 2018 16:54:40 -0400 Received: by mail-it0-x242.google.com with SMTP id u4-v6so7398151itg.0; Sat, 30 Jun 2018 13:54:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:mime-version:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=fv9jwliBTnKFGFXc+aw5Hux7/7nTP9UQH91oTkQcjb4=; b=ChtUdzFk5F3xkqs+NxvbhcdMllXumfCQJVk4xmR0ywQII66XhPyxpglzlTUJcNY6LY 5l4hhEF8xsyr/7NMREjb+qXX2VyATMu+wnFmFbl/c2lIuF1VSOXHLt3tHSBk0HDSwvPy 6TX06S2JeIBK1i4wYOEYsRvwd5bjBU5HnVOcrvKNbVk0dtg/uTglPlcdGgaD63buBndl BWIsuzsdBgqadFnWcCTBHHLHcx7+S5yNMRqRoZWa/xcjkql2lorWSAWimY+2Kqq2Z4yk YU5j4VDiLDOocvBzFc8BLFdTPKbBqfhxPA9EOhkZ8OCLbQHBI7T861zmxgxi+G11807h ghLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:mime-version:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=fv9jwliBTnKFGFXc+aw5Hux7/7nTP9UQH91oTkQcjb4=; b=JPvfr/AnrgWY1Crx9G8tUUeKv6PoGdjV6etPFkGu6v+e5KDMIjL+qTH3fZPTON0oOG es3cPFdVlGSCvhU5vlY2tZ/onuB1ra4Cuzy/EHkoW8fT1J4mFO76s16jkFyunT7TOWNw KYlgcJxgNub1pmeGEyooP6kSAUp1cBxUohmUBKXvaKv/iSkR1ZoNWx2YDj2iZVX/oi3z qLrFOXUyb7M0nU4LV0WDGRsrUnxmUmiPRe7edjWzirKMhyxJ6Le9jxtr4FF0IJQvUMCd s7dQsb/X81WC/C6FQvykg3g8v45sSkvfgszqGOxUIxK5wYqa+4GAAF0ynR6CeNekp5BB cb/g== X-Gm-Message-State: APt69E2srvMVoadfSFFdWD5qjNjztW5MwIDKxl/D+1dr9dqgXbA2Yo04 f4MIJPfHEgabQ33qSR5rSXk= X-Google-Smtp-Source: AAOMgpf99euzn1fjtQFW4/EZXk4zloI44n57YFvi2idwfv4vpQyhw+Wu0t0MzcxXL5b5XNCdhq4UJw== X-Received: by 2002:a24:7809:: with SMTP id p9-v6mr5511576itc.20.1530392079642; Sat, 30 Jun 2018 13:54:39 -0700 (PDT) Received: from [192.168.0.9] ([69.14.184.20]) by smtp.gmail.com with ESMTPSA id f184-v6sm5431260ioa.86.2018.06.30.13.54.38 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 30 Jun 2018 13:54:38 -0700 (PDT) Mime-Version: 1.0 (Apple Message framework v1084) From: Programmingkid In-Reply-To: Date: Sat, 30 Jun 2018 16:54:37 -0400 Message-Id: <5955C4AE-747E-4782-8AB8-1F96FEBF10CF@gmail.com> References: <20180630030606.17288-1-programmingkidx@gmail.com> To: Richard Henderson X-Mailer: Apple Mail (2.1084) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4001:c0b::242 Subject: Re: [Qemu-devel] [PATCH v2] fix fdiv instruction X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, david@gibson.dropbear.id.au Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Jun 30, 2018, at 11:43 AM, Richard Henderson wrote: > On 06/29/2018 08:06 PM, John Arbuckle wrote: >> 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 >> --- >> 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(-) >> >> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c >> index 7714bfe0f9..9ccba1ec3f 100644 >> --- a/target/ppc/fpu_helper.c >> +++ b/target/ppc/fpu_helper.c >> @@ -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, > >> + } 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); > > You don't need any of this. > >> farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status); >> + 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~ 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(-) diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index 7714bfe..3939983 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -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; diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c index 2fbd4d4..9b1ea68 100644 --- a/target/ppc/translate/fp-impl.inc.c +++ b/target/ppc/translate/fp-impl.inc.c @@ -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);