qemu-riscv
[Top][All Lists]
Advanced

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

Re: [PATCH v3 6/9] target/riscv: add support for Zcmp extension


From: Richard Henderson
Subject: Re: [PATCH v3 6/9] target/riscv: add support for Zcmp extension
Date: Thu, 17 Nov 2022 01:44:03 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2

On 11/16/22 23:03, Weiwei Li wrote:
Add encode, trans* functions for Zcmp instructions

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
  target/riscv/insn16.decode                |  18 ++
  target/riscv/insn_trans/trans_rvzce.c.inc | 242 +++++++++++++++++++++-
  target/riscv/translate.c                  |   5 +
  3 files changed, 264 insertions(+), 1 deletion(-)

Better, though...

+static bool gen_zcmp_check(DisasContext *ctx, arg_zcmp *a)
+{
+    /* rlist 0 to 3 are reserved for future EABI variant */
+    if (a->zcmp_rlist < 4) {
+        return false;
+    }
+
+    /* rlist <= 6 when RV32E/RV64E */
+    if (ctx->cfg_ptr->ext_e && a->zcmp_rlist > 6) {
+        return false;
+    }
+
+    return true;
+}

This could be merged into...

+
+#define X_S0    8
+#define X_S1    9
+#define X_Sn    16
+
+static inline void update_push_pop_list(target_ulong rlist, bool *xreg_list)

... here.

For instance, one way is to return false when the list is invalid.
Better is to return a uint32_t bitmap of the registers in the list, with 0 
indicating invalid.

Nit 1: Remove the inline.
Nit 2: A better name might be decode_push_pop_list.

+static inline target_ulong caculate_stack_adj(int bytes, target_ulong rlist,
+                                              target_ulong spimm)
+{
+    target_ulong stack_adj_base = 0;
+    switch (rlist) {
+    case 15:
+        stack_adj_base = bytes == 4 ? 64 : 112;
+        break;
+    case 14:
+        stack_adj_base = bytes == 4 ? 48 : 96;
+        break;
+    case 13:
+    case 12:
+        stack_adj_base = bytes == 4 ? 48 : 80;
+        break;
+    case 11:
+    case 10:
+        stack_adj_base = bytes == 4 ? 32 : 64;
+        break;
+    case 9:
+    case 8:
+        stack_adj_base = bytes == 4 ? 32 : 48;
+        break;
+    case 7:
+    case 6:
+        stack_adj_base = bytes == 4 ? 16 : 32;
+        break;
+    case 5:
+    case 4:
+        stack_adj_base = 16;
+        break;
+    }

I really dislike this, as it replicates the decoding done just above.
I think this ought to be simply:

    ROUND_UP(ctpop32(reg_bitmap) * reg_size, 16) + spimm


+static bool gen_pop(DisasContext *ctx, arg_zcmp *a, bool ret, bool ret_val)
+{
+    REQUIRE_ZCMP(ctx);
+
+    if (!gen_zcmp_check(ctx, a)) {
+        return false;
+    }
+
+    bool xreg_list[32] = {false};
+    int bytes = get_ol(ctx) == MXL_RV32 ? 4 : 8;

Better with

    MemOp memop = get_ol(ctx) == MXL_RV32 ? MO_TEUL : MO_TEUQ;
    int reg_size = memop_size(memop);

+            switch (bytes) {
+            case 4:
+                tcg_gen_qemu_ld_tl(dest, addr, ctx->mem_idx, MO_32);
+                break;
+            case 8:
+                tcg_gen_qemu_ld_tl(dest, addr, ctx->mem_idx, MO_64);
+                break;
+            default:
+                break;
+            }

These are incorrect in that they do not indicate the target endianness.
Better to merge the two using the common memop computed above:

    tcg_gen_qemu_ld_tl(dest, addr, ctx->mem_idx, memop);

+static bool trans_cm_mvsa01(DisasContext *ctx, arg_cm_mvsa01 *a)
+{
+    REQUIRE_ZCMP(ctx);
+
+    TCGv a0 = get_gpr(ctx, xA0, EXT_NONE);
+    TCGv a1 = get_gpr(ctx, xA1, EXT_NONE);
+
+    gen_set_gpr(ctx, a->rs1, a0);
+    gen_set_gpr(ctx, a->rs2, a1);

rs1 must not equal rs2.


r~



reply via email to

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