Re: [PATCH 3/6] target/riscv: add support for zfinx

[liweiwei <liweiwei@iscas.ac.cn>]

在 2021/12/25 上午6:26, Richard Henderson 写道:
> On 12/23/21 7:49 PM, liweiwei wrote:
>>   static bool trans_fsgnj_s(DisasContext *ctx, arg_fsgnj_s *a)
>>   {
>>       REQUIRE_FPU;
>> -    REQUIRE_EXT(ctx, RVF);
>> +    REQUIRE_ZFINX_OR_F(ctx);
>>   +    TCGv_i64 dest = dest_fpr(ctx, a->rd);
>>       if (a->rs1 == a->rs2) { /* FMOV */
>> -        gen_check_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rs1]);
>> +        TCGv_i64 src1 = get_fpr_hs(ctx, a->rs1);
>> +        if (ctx->ext_zfinx) {
>> +            gen_nanbox_s(dest, src1);
>
> Sign-extend, not nanbox.  Or, since you handle sign-extension in 
> gen_set_gpr_hs, nothing at all -- just tcg_gen_mov_i64.

Yeah. I did gen_nanbox_s here is to make the dest nan-boxing just as 
other instructions. It's truely unnecessary here. I'll try maximize the 
reuse of code here.

>
>>       } else { /* FSGNJ */
>> -        TCGv_i64 rs1 = tcg_temp_new_i64();
>> -        TCGv_i64 rs2 = tcg_temp_new_i64();
>> -
>> -        gen_check_nanbox_s(rs1, cpu_fpr[a->rs1]);
>> -        gen_check_nanbox_s(rs2, cpu_fpr[a->rs2]);
>> -
>> -        /* This formulation retains the nanboxing of rs2. */
>> -        tcg_gen_deposit_i64(cpu_fpr[a->rd], rs2, rs1, 0, 31);
>> -        tcg_temp_free_i64(rs1);
>> -        tcg_temp_free_i64(rs2);
>> +        TCGv_i64 rs1, rs2;
>> +        if (!ctx->ext_zfinx) {
>> +            TCGv_i64 src1 = get_fpr_hs(ctx, a->rs1);
>> +            TCGv_i64 src2 = get_fpr_hs(ctx, a->rs2);
>> +            rs1 = tcg_temp_new_i64();
>> +            rs2 = tcg_temp_new_i64();
>> +            gen_check_nanbox_s(rs1, src1);
>> +            gen_check_nanbox_s(rs2, src2);
>> +        } else {
>> +            rs1 = get_fpr_hs(ctx, a->rs1);
>> +            rs2 = get_fpr_hs(ctx, a->rs2);
>> +        }
>> +
>> +        /* This formulation retains the nanboxing of rs2 in normal 
>> 'F'. */
>> +        tcg_gen_deposit_i64(dest, rs2, rs1, 0, 31);
>> +        if (!ctx->ext_zfinx) {
>> +            tcg_temp_free_i64(rs1);
>> +            tcg_temp_free_i64(rs2);
>> +        } else {
>> +            gen_nanbox_s(dest, dest);
>> +        }
>
> This is tangled enough that I think you should check zfinx at one 
> higher indent level, and not do conditional allocate followed by 
> conditional free.
OK, I'll divide them. By the way I did in this way to maximize the reuse 
of code.
>
>> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
>> index 065e8162a2..9f3f3319f2 100644
>> --- a/target/riscv/internals.h
>> +++ b/target/riscv/internals.h
>> @@ -51,8 +51,12 @@ static inline uint64_t nanbox_s(float32 f)
>>       return f | MAKE_64BIT_MASK(32, 32);
>>   }
>>   -static inline float32 check_nanbox_s(uint64_t f)
>> +static inline float32 check_nanbox_s(CPURISCVState *env, uint64_t f)
>>   {
>> +    /* Disable nanbox check when enable zfinx */
>> +    if (RISCV_CPU(env_cpu(env))->cfg.ext_zfinx)
>> +        return (uint32_t)f;
>> +
>
> Braces.
>
>
> r~