| Message ID | 20211224034915.17204-3-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support subsets of Float-Point in Integer Registers extensions | expand |
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. > + 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. > + 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. > +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. > + } 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. > + } 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. r~
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~
On 12/24/21 7:13 PM, liweiwei wrote: > 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. There is no nanboxing in zfinx at all -- values are sign-extended. r~
Sorry. In the old spec(version 0.41), nanboxing is not totally disabled, but "NaN-boxing is limited to |XLEN| bits, not |FLEN| bits". Taking misa.mxl into acount, if misa.mxl is RV32, and maximum is RV64, this should be sign-extended. Is there any other new update for nanboxing to the spec? 在 2021/12/26 上午6:00, Richard Henderson 写道: > On 12/24/21 7:13 PM, liweiwei wrote: >> 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. > > There is no nanboxing in zfinx at all -- values are sign-extended. > > > r~
OK. It have been changed to sign-extended in the "processing of Narrower Values" section of the new spec(version 1.0.0.rc) . I'll fix this and update other nan-boxing processing in current implementation. 在 2021/12/26 上午9:42, liweiwei 写道: > > Sorry. In the old spec(version 0.41), nanboxing is not totally > disabled, but "NaN-boxing is limited to |XLEN| bits, not |FLEN| bits". > Taking misa.mxl into acount, if misa.mxl is RV32, and maximum is RV64, > this should be sign-extended. Is there any other new update for > nanboxing to the spec? > > 在 2021/12/26 上午6:00, Richard Henderson 写道: >> On 12/24/21 7:13 PM, liweiwei wrote: >>> 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. >> >> There is no nanboxing in zfinx at all -- values are sign-extended. >> >> >> r~
On 12/25/21 5:42 PM, liweiwei wrote: > Sorry. In the old spec(version 0.41), nanboxing is not totally disabled, but "NaN-boxing > is limited to |XLEN| bits, not |FLEN| bits". Taking misa.mxl into acount, if misa.mxl is > RV32, and maximum is RV64, this should be sign-extended. Is there any other new update for > nanboxing to the spec? Yes, in 1.0.0-rc, it's all gone. r~
diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 8b1cdacf50..bac42e60bd 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -104,10 +104,13 @@ typedef struct DisasContext { target_ulong vstart; bool vl_eq_vlmax; uint8_t ntemp; + uint8_t nftemp; CPUState *cs; TCGv zero; /* Space for 3 operands plus 1 extra for address computation. */ TCGv temp[4]; + /* Space for 4 float point operands */ + TCGv_i64 ftemp[4]; /* PointerMasking extension */ bool pm_enabled; TCGv pm_mask; @@ -295,6 +298,165 @@ static void gen_set_gpr(DisasContext *ctx, int reg_num, TCGv t) } } +static TCGv_i64 ftemp_new(DisasContext *ctx) +{ + assert(ctx->nftemp < ARRAY_SIZE(ctx->ftemp)); + return ctx->ftemp[ctx->nftemp++] = tcg_temp_new_i64(); +} + +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 + { + 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], ctx->zero); + } + return t; + } +#else + /* fall through */ + 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]; + } +} + +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; + } 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]; + } +} + +static TCGv_i64 dest_fpr(DisasContext *ctx, int reg_num) +{ + if (ctx->ext_zfinx) { + switch (get_ol(ctx)) { + case MXL_RV32: + return ftemp_new(ctx); +#ifdef TARGET_RISCV64 + case MXL_RV64: + if (reg_num == 0) { + return ftemp_new(ctx); + } else { + return cpu_gpr[reg_num]; + } +#endif + default: + g_assert_not_reached(); + } + } else { + return cpu_fpr[reg_num]; + } +} + +static void gen_set_fpr_hs(DisasContext *ctx, int reg_num, TCGv_i64 t) +{ + if (ctx->ext_zfinx) { + if (reg_num != 0) { + switch (get_ol(ctx)) { + case MXL_RV32: +#ifdef TARGET_RISCV32 + tcg_gen_extrl_i64_i32(cpu_gpr[reg_num], t); + break; +#else + tcg_gen_ext32s_i64(cpu_gpr[reg_num], t); + break; + case MXL_RV64: + tcg_gen_mov_i64(cpu_gpr[reg_num], t); + break; +#endif + default: + g_assert_not_reached(); + } + } + } else { + tcg_gen_mov_i64(cpu_fpr[reg_num], t); + } +} + +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); + } 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); + } + break; +#else + tcg_gen_ext32s_i64(cpu_gpr[reg_num], t); + tcg_gen_sari_i64(cpu_gpr[reg_num + 1], t, 32); + } + break; + case MXL_RV64: + tcg_gen_mov_i64(cpu_gpr[reg_num], t); + break; +#endif + default: + g_assert_not_reached(); + } + } + } else { + tcg_gen_mov_i64(cpu_fpr[reg_num], t); + } +} + static void gen_jal(DisasContext *ctx, int rd, target_ulong imm) { target_ulong next_pc; @@ -727,6 +889,8 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->cs = cs; ctx->ntemp = 0; memset(ctx->temp, 0, sizeof(ctx->temp)); + ctx->nftemp = 0; + memset(ctx->ftemp, 0, sizeof(ctx->ftemp)); ctx->pm_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_ENABLED); int priv = tb_flags & TB_FLAGS_PRIV_MMU_MASK; ctx->pm_mask = pm_mask[priv]; @@ -761,6 +925,11 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) ctx->temp[i] = NULL; } ctx->ntemp = 0; + for (int i = ctx->nftemp - 1; i >= 0; --i) { + tcg_temp_free_i64(ctx->ftemp[i]); + ctx->ftemp[i] = NULL; + } + ctx->nftemp = 0; if (ctx->base.is_jmp == DISAS_NEXT) { target_ulong page_start;