Re: [Qemu-devel] [PATCH] fix fdiv instruction

[David Gibson]

On Sat, Jun 23, 2018 at 04:17:08PM -0400, Programmingkid wrote:
> 
> > On Jun 23, 2018, at 12:17 PM, Richard Henderson <address@hidden> wrote:
> > 
> > On 06/22/2018 07:22 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 zero. 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 <address@hidden>
> >> ---
> >> target/ppc/fpu_helper.c            | 16 ++++++++++++++++
> >> target/ppc/translate/fp-impl.inc.c | 28 +++++++++++++++++++++++++++-
> >> 2 files changed, 43 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> >> index 7714bfe0f9..de694604fb 100644
> >> --- a/target/ppc/fpu_helper.c
> >> +++ b/target/ppc/fpu_helper.c
> >> @@ -658,6 +658,20 @@ uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, 
> >> uint64_t arg2)
> >>     } else if (unlikely(float64_is_zero(farg1.d) && 
> >> float64_is_zero(farg2.d))) {
> >>         /* Division of zero by zero */
> >>         farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXZDZ, 1);
> >> +    } else if (arg2 == 0) {
> >> +        /* Division by zero */
> >> +        float_zero_divide_excp(env, GETPC());
> >> +        if (fpscr_ze) { /* if zero divide exception is enabled */
> >> +            farg1.ll = 0;
> >> +        } else {
> >> +            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);
> > 
> > I don't believe you.
> > (1) This is against IEEE spec,
> 
> I'm trying to implement IBM PowerPC specs. 

Which should have an IEEE compliant FPU.

> > (2) There is nothing about this zero result in the Power manual,
> 
> This is for PowerPC. Power and PowerPC are cousins to each other
> rather than having a child-parent relationship. Yes there are a lot
> of similar instructions between them, this does not mean they are
> compatible with each other.

The're definitely supposed to be compatible, at least for userspace
(PR=1) instructions.  The distinction between "POWER" and "PowerPC" is
kind of confusing, but basically every PowerPC or POWER chip from
POWER2 onwards should be backwards compatible for non-privileged
instructions.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature