Re: [Qemu-devel] [PATCH] ppc: Three floating point fixes

[Richard Henderson]

On 8/16/19 8:27 PM, Paul A. Clarke wrote:
> From: "Paul A. Clarke" <address@hidden>
> 
> - target/ppc/fpu_helper.c:
>   - helper_todouble() was not properly converting INFINITY from 32 bit
>   float to 64 bit double.
>   - helper_todouble() was not properly converting any denormalized
>   32 bit float to 64 bit double.

These two would seem to be my fault:
Fixes: 86c0cab11aa (target/ppc: Use non-arithmetic conversions for fp 
load/store)

> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 5611cf0..82b5425 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -62,13 +62,14 @@ uint64_t helper_todouble(uint32_t arg)
>          ret  = (uint64_t)extract32(arg, 30, 2) << 62;
>          ret |= ((extract32(arg, 30, 1) ^ 1) * (uint64_t)7) << 59;
>          ret |= (uint64_t)extract32(arg, 0, 30) << 29;
> +        ret |= (0x7ffULL * (extract32(arg, 23, 8) == 0xff)) << 52;

Since the overwrites bits set two lines above,
I think this would be better as

    if (likely(abs_arg >= 0x00800000)) {
        /* Normalized operand, or Inf or NaN.  */
        if (unlikely(extract32(arg, 23, 8) == 0xff)) {
            /* Inf or NaN.  */
            ...
        } else {
            /* Normalized operand.  */


>      } else {
>          /* Zero or Denormalized operand.  */
>          ret = (uint64_t)extract32(arg, 31, 1) << 63;
>          if (unlikely(abs_arg != 0)) {
>              /* Denormalized operand.  */
>              int shift = clz32(abs_arg) - 9;
> -            int exp = -126 - shift + 1023;
> +            int exp = -127 - shift + 1023;
>              ret |= (uint64_t)exp << 52;
>              ret |= abs_arg << (shift + 29);

Hmm.  The manual says -126, but it also shifts the fraction by a different
amount, such that the implicit bit is 1.

What I also don't see here is where the most significant bit is removed from
the fraction, a-la "FRT[5:63] = frac[1:52]" in the manual.

The soft-float code from which this was probably copied did this by shifting
the fraction such that the msb overlaps the exponent, biasing the exponent by
-1, and then using an add instead of an or to simultaneously remove the bias,
swallow the msb, and include the lower bits unchanged.

So it looks like this should be

    /*
     * Shift fraction so that the msb is in the implicit bit position.
     * Thus shift is in the range [1:23].
     */
    int shift = clz32(abs_arg) - 8;
    /*
     * The first 3 terms compute the float64 exponent.  We then bias
     * this result by -1 so that we can swallow the implicit bit below.
     */
    int exp = -126 - shift + 1023 - 1;
    ret |= (uint64_t)exp << 52;
    ret += (uint64_t)abs_arg << (52 - 23 + shift);

Hmm.  I see another bug in the existing code whereby abs_arg is not cast before
shifting.  This truncation is probably how you're seeing correct results with
your patch, for denormals containing only one bit set, e.g. FLT_TRUE_MIN.

It would be good to test other denormals, like 0x00055555.


> @@ -2871,10 +2872,14 @@ void helper_xscvqpdp(CPUPPCState *env, uint32_t 
> opcode,
>  
>  uint64_t helper_xscvdpspn(CPUPPCState *env, uint64_t xb)
>  {
> +    uint64_t result;
> +
>      float_status tstat = env->fp_status;
>      set_float_exception_flags(0, &tstat);
>  
> -    return (uint64_t)float64_to_float32(xb, &tstat) << 32;
> +    result = (uint64_t)float64_to_float32(xb, &tstat);
> +    /* hardware replicates result to both words of the doubleword result.  */
> +    return (result << 32) | result;
>  }

This definitely should be a separate patch.  The comment should include some
language about this behaviour being required by a future ISA revision.


r~