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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3178 bytes --]

On Fri, Aug 16, 2019 at 02:27:49PM -0500, Paul A. Clarke wrote:
> From: "Paul A. Clarke" <pc@us.ibm.com>
> 
> - 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.
> 
> - GCC, as of version 8 or so, takes advantage of the hardware's
>   implementation of the xscvdpspn instruction to optimize the following
>   sequence:
>     xscvdpspn vs0,vs1
>     mffprwz   r8,f0
>   ISA 3.0B has xscvdpspn leaving its result in word 1 of the target register,
>   and mffprwz expecting its input to come from word 0 of the source register.
>   This sequence fails with QEMU, as a shift is required between those two
>   instructions.  However, the hardware splats the result to both word 0 and
>   word 1 of its output register, so the shift is not necessary.
>   Expect a future revision of the ISA to specify this behavior.

As well as addressing Richard's comments, I'd prefer to see each of
the 3 fixes here in separate patches.  The code changes may be tiny,
but I find the fpu emulation code cryptic at the best of times, and
making it clear which change is addressing which bug would help.

> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
> ---
>  target/ppc/fpu_helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> 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;
>      } 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);
>          }
> @@ -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;
>  }
>  
>  uint64_t helper_xscvspdpn(CPUPPCState *env, uint64_t xb)

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]