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

[Richard Henderson <richard.henderson@linaro.org>]

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~