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

[Laurent Vivier]

Le 19/06/2017 à 22:53, Richard Henderson a écrit :
> 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)?

yes, it's totally wrong.

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

OK, I will.

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

Thanks,
Laurent