diff mbox series

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

Message ID 20211224034915.17204-3-liweiwei@iscas.ac.cn
State New
Headers show
Series support subsets of Float-Point in Integer Registers extensions | expand

Commit Message

Weiwei Li Dec. 24, 2021, 3:49 a.m. UTC 
Co-authored-by: ardxwe <ardxwe@gmail.com>
Signed-off-by: liweiwei <liweiwei@iscas.ac.cn>
Signed-off-by: wangjunqiang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/translate.c | 169 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 169 insertions(+)

Comments

Richard Henderson Dec. 24, 2021, 10 p.m. UTC | #1 
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~
Weiwei Li Dec. 25, 2021, 3:13 a.m. UTC | #2 
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~
Richard Henderson Dec. 25, 2021, 10 p.m. UTC | #3 
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~
Weiwei Li Dec. 26, 2021, 1:42 a.m. UTC | #4 
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~
Weiwei Li Dec. 26, 2021, 1:54 a.m. UTC | #5 
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~
Richard Henderson Dec. 26, 2021, 3:48 a.m. UTC | #6 
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 mbox series

Patch

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;