Re: [PATCH v2 13/22] target/loongarch: Add floating point arithmetic instruction translation

[Song Gao <gaosong@loongson.cn>]

Hi, Richard.

On 07/28/2021 12:12 AM, Richard Henderson wrote:
> On 7/26/21 9:17 PM, Song Gao wrote:
>>> I think this should be as simple as
>>>
>>>    gen_helper_fp_add_s(cpu_fpu[a->fd], cpu_env,
>>>                        cpu_fpu[a->fj], cpu_fpu[a->fk]);
>>>
>>> I also think that loongarch should learn from risc-v and change the architecture to "nan-box" single-precision results -- fill the high 32-bits with 1s.  This is an SNaN representation for double-precision and will immediately fail when incorrectly using a single-precision value as a double-precision input.
>>>
>>> Thankfully the current architecture is backward compatible with nan-boxing.
>>>
>>
>> by this method,  the trans_fadd_s is
>>
>> static bool trans_fadd_s(DisasContext *ctx, arg_fadd_s * a)
>> {
>>      TCGv_i64 fp0, fp1;
>>
>>      fp0 = tcg_temp_new_i64();
>>      fp1 = tcg_temp_new_i64();
>>
>>      check_fpu_enabled(ctx);
>>      gen_load_fpr64(fp0, a->fj);
>>      gen_load_fpr64(fp1, a->fk);
>>      gen_helper_fp_add_s(fp0, cpu_env, fp0, fp1);
>>
>>      gen_check_nanbox_s(fp0, fp0); /* from riscv */
>>
>>      gen_store_fpr64(fp0, a->fd);
>>
>>      tcg_temp_free_i64(fp0);
>>      tcg_temp_free_i64(fp1);
>>
>>      return true;
>> }
> 
> A few points here:
> 
> (1) You do not need gen_load_fpr64 and gen_store_fpr64 at all.
>     These were from mips to deal with the varying fpu sizes.
> 
> (2) If we need to call a helper, then the helper as much of
>     the work a possible.  Therefore the nanboxing should be
>     done there.  See riscv/fpu_helper.c, and the use of
>     nanbox_s within that file.
> 
> (3) Again, use a helper function:
> 
> static bool gen_binary_fp(DisasContext *ctx, arg_fmt_fdfjfk *a,
>                           void (*func)(TCGv_i64, TCGv_env,
>                                        TCGv_i64, TCGv_i64))
> {
>     if (check_fpu_enabled(ctx)) {
>         func(cpu_fpr[a->fd], cpu_env,
>              cpu_fpr[a->fj], cpu_fpr[a->fk]);
>     }
>     return true;
> }
> 
> TRANS(fadd_s, gen_binary_fp, gen_helper_fp_add_s)
> TRANS(fadd_d, gen_binary_fp, gen_helper_fp_add_d)
> 
>> uint64_t helper_fp_add_s(CPULoongArchState *env, uint64_t fp, uint64_t fp1)
>> {
>>      uint32_t fp2;
>>
>>      fp2 = float32_add((uint32_t)fp, (uint32_t)fp1, &env->active_fpu.fp_status);
>>      update_fcsr0(env, GETPC());
>>      return (uint64_t)fp2;
>> }
> 
> with return nanbox_s(fp2);
>
OK.

Again, thank you kindly help.

Thanks
Song Gao.
> 
> r~