Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
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~
