Re: [Qemu-devel] [PATCH v3 05/16] target-m68k: use floatx80 internally

[Richard Henderson]

On 02/07/2017 11:59 AM, Laurent Vivier wrote:
> +    uint32_t fp0h;
> +    uint64_t fp0l;
> +    uint32_t fp1h;
> +    uint64_t fp1l;

I'm not especially keen on these temporaries.

Wouldn't it be better to pass pointers to FPReg to the helpers, so that e.g.

  fadd.x  fp0, fp1

puts the result in fp1 directly, without having to copy from this FP0 location?
I can see that you would need a proper FPReg temporary to deal with e.g.

  fadd.d  a0@, fp1

such that the memory source gets converted before being used as input to the 
addition.

> +static float32 FP0_to_float32(CPUM68KState *env)
>  {
> +    return *(float32 *)&env->fp0h;
>  }
...
> +static void float32_to_FP0(CPUM68KState *env, float32 val)
>  {
> +    env->fp0h = *(uint32_t *)&val;
>  }

I don't like this type-punning.  I also don't see what good it does to store 
these truncated values in portions of FP0, when you could simply pass or return 
them by value from the relevant helpers, e.g.

> +void HELPER(reds32_FP0)(CPUM68KState *env)
>  {
> +    int32_t res;
> +
> +    res = floatx80_to_int32(FP0_to_floatx80(env), &env->fp_status);
> +
> +    int32_to_FP0(env, res);
>  }

could easily be

int32_t HELPER(reds32)(CPUM68KState *env, FPReg *val)
{
  return floatx80_to_int32(*val, &env->fp_status);
}


r~