qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [PATCH 2/6] target/riscv: add support for unique fpr read/write with support for zfinx
Date: Fri, 24 Dec 2021 14:00:40 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

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~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]