Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx

[liweiwei]

Thanks for your comments.

在 2021/12/25 上午6:00, Richard Henderson 写道:
> On 12/23/21 7:49 PM, liweiwei wrote:
> > +static TCGv_i64 get_fpr_hs(DisasContext *ctx, int reg_num)
> > +{
> > +    if (ctx->ext_zfinx) {
> > +        switch (get_ol(ctx)) {
> > +        case MXL_RV32:
> > +#ifdef TARGET_RISCV32
> > +            if (reg_num == 0) {
> > +                tcg_gen_concat_i32_i64(t, ctx->zero, ctx->zero);
>
> return tcg_constant_i64(0);
> You could hoist this above the switch.
OK.
> > +                tcg_gen_concat_i32_i64(t, cpu_gpr[reg_num], ctx->zero);
> tcg_gen_extu_i32_i64 -- though I would think a signed extraction would 
> be more natural, as compared with the signed value you'll get from the 
> RV64 case.
In RV64 case, this should be nan-boxing value( upper bits are all 
ones).  However, zfinx will not check nan-boxing of source, the upper 32 
bits have no effect on the final result. So I think both zero-extended 
or sign-extended are OK.
> > +        case MXL_RV64:
> > +            if (reg_num == 0) {
> > +                return ctx->zero;
> > +            } else {
> > +                return cpu_gpr[reg_num];
> > +            }
> > +#endif
> > +        default:
> > +            g_assert_not_reached();
> > +        }
>
> > +    } else {
> > +        return cpu_fpr[reg_num];
> > +    }
>
> It is tempting to reverse the sense of the zfinx if, to eliminate this 
> case first, and keep the indentation level down.
OK I'll update this.
> > +static TCGv_i64 get_fpr_d(DisasContext *ctx, int reg_num)
> > +{
> > +    if (ctx->ext_zfinx) {
> > +        switch (get_ol(ctx)) {
> > +        case MXL_RV32:
> > +            if (reg_num & 1) {
> > +                gen_exception_illegal(ctx);
> > +                return NULL;
>
> Not keen on checking this here.  It should be done earlier.
OK. I'll put this check into the trans_* function
> > +            } else {
> > +#ifdef TARGET_RISCV32
> > +                TCGv_i64 t = ftemp_new(ctx);
> > +                if (reg_num == 0) {
> > +                    tcg_gen_concat_i32_i64(t, ctx->zero, ctx->zero);
> > +                } else {
> > +                    tcg_gen_concat_i32_i64(t, cpu_gpr[reg_num], 
> > cpu_gpr[reg_num + 1]);
> > +                }
> > +                return t;
> > +            }
> > +#else
> > +                if (reg_num == 0) {
> > +                    return ctx->zero;
> > +                } else {
> > +                    TCGv_i64 t = ftemp_new(ctx);
> > +                    tcg_gen_deposit_i64(t, cpu_gpr[reg_num], 
> > cpu_gpr[reg_num + 1], 32, 32);
> > +                    return t;
> > +                }
> > +            }
> > +        case MXL_RV64:
> > +            if (reg_num == 0) {
> > +                return ctx->zero;
> > +            } else {
> > +                return cpu_gpr[reg_num];
> > +            }
> > +#endif
> > +        default:
> > +            g_assert_not_reached();
> > +        }
> > +    } else {
> > +        return cpu_fpr[reg_num];
> > +    }
>
> Very similar comments to above.
>
> > +static void gen_set_fpr_d(DisasContext *ctx, int reg_num, TCGv_i64 t)
> > +{
> > +    if (ctx->ext_zfinx) {
> > +        if (reg_num != 0) {
> > +            switch (get_ol(ctx)) {
> > +            case MXL_RV32:
> > +                if (reg_num & 1) {
> > +                    gen_exception_illegal(ctx);
>
> This is *far* too late to diagnose illegal insn.  You've already 
> modified global state in the FPSCR, or have taken an fpu exception in 
> fpu_helper.c.
OK. I'll put this check into the trans_* function too.
> > +                } else {
> > +#ifdef TARGET_RISCV32
> > +                    tcg_gen_extrl_i64_i32(cpu_gpr[reg_num], t);
> > +                    tcg_gen_extrh_i64_i32(cpu_gpr[reg_num + 1], t);
> > +                }
>
> tcg_gen_extr_i64_i32 does both at once.
> Never split braces around ifdefs like this.
OK. I'll update this.
> r~