Re: [Qemu-devel] [PATCH v4 5/7] target-m68k: use floatx80 internally

[Richard Henderson]

On 06/11/2017 04:16 PM, Laurent Vivier wrote:
> +static void gen_load_fp(DisasContext *s, int opsize, TCGv addr, TCGv_ptr fp)
> +{
> +    TCGv tmp;
> +    TCGv_i64 t64;
> +    int index = IS_USER(s);
> +
> +    t64 = tcg_temp_new_i64();
> +    tmp = tcg_temp_new();
> +    switch (opsize) {
> +    case OS_BYTE:
> +        tcg_gen_qemu_ld8s(tmp, addr, index);
> +        gen_helper_exts32(cpu_env, fp, tmp);
> +        break;
> +    case OS_WORD:
> +        tcg_gen_qemu_ld16s(tmp, addr, index);
> +        gen_helper_exts32(cpu_env, fp, tmp);
> +        break;
> +    case OS_LONG:
> +        tcg_gen_qemu_ld32u(tmp, addr, index);
> +        gen_helper_exts32(cpu_env, fp, tmp);
> +        break;
> +    case OS_SINGLE:
> +        tcg_gen_qemu_ld32u(tmp, addr, index);
> +        gen_helper_extf32(cpu_env, fp, tmp);
> +        break;
> +    case OS_DOUBLE:
> +        tcg_gen_qemu_ld64(t64, addr, index);
> +        gen_helper_extf64(cpu_env, fp, t64);
> +        tcg_temp_free_i64(t64);
> +        break;
> +    case OS_EXTENDED:
> +        tcg_gen_qemu_ld32u(tmp, addr, index);
> +        tcg_gen_shri_i32(tmp, tmp, 16);
> +        tcg_gen_st16_i32(tmp, fp, offsetof(FPReg, l.upper));
> +        tcg_gen_addi_i32(tmp, addr, 4);
> +        tcg_gen_qemu_ld64(t64, tmp, index);
> +        tcg_gen_st_i64(t64, fp, offsetof(FPReg, l.lower));
> +        break;
> +    case OS_PACKED:
> +        tcg_gen_qemu_ld32u(tmp, addr, index);
> +        tcg_gen_st16_i32(tmp, fp, offsetof(FPReg, l.upper));
> +        tcg_gen_addi_i32(tmp, addr, 4);
> +        tcg_gen_qemu_ld64(t64, tmp, index);
> +        tcg_gen_st_i64(t64, fp, offsetof(FPReg, l.lower));

I don't see how this can be correct.  Doesn't the packed-decimal format use all 
12 bytes (with two unaligned nibbles unused)?

It would also make me happier if we were to adjust the definition of fl0atx80 
to more closely match m68k and those missing zeros.  Shouldn't real hardware 
move instructions propagate those middle 2 bytes regardless of contents?

Perhaps something like

#ifdef TARGET_M68K
  typedef struct {
    uint64_t low;
    union {
      uin32_t high32;
      struct {
#ifdef HOST_WORDS_BIGENDIAN
        uint16_t high, zero;
#else
        uint16_t zero, high;
#endif
      };
    };
  } floatx80;
#else
  ...
#endif

(with a minor fix to make_floatx80 to use named initializers).

Then you can use full 32-bit store insns when copying data here.  Which also 
allows you to drop some of the shifts you're needing to add.

And, in future, when you actually implement the packed decimal, you'll be able 
to use the high32 field to Do the Right Thing.

All of the rest of the patch looks good.


r~