Re: [Qemu-devel] [PATCH 15/28] target/riscv: Convert quadrant 0 of RVXC insns to decodetree

[Richard Henderson]

On 10/12/18 10:30 AM, Bastian Koppelmann wrote:
> +# Argument sets:
> +&cl               rs1 rd
> +&cl_dw     uimm   rs1 rd
> +&ciw       nzuimm     rd
> +&cs               rs1 rs2
> +&cs_dw     uimm   rs1 rs2

I guess this is good enough for now.
What I'd like to see is something like

&i      imm rs1 rd      !extern

which tells decodetree that the arg_i structure has been declared elsewhere.
Then you can write

> address@hidden      ... ... ... .. ... .. &i  imm=%uimm_cl_w  rs1=%rs1_3  
> rd=%rs2_3

lw      010 ... ... .. ... 00   @cl_w

and then the same trans_lw can be used.

> +c_flw_ld          011  --- ... -- ... 00 @cl    #Note: Must parse uimm 
> manually

Why?  It looks identical to @cl_w to me.

> +static bool trans_c_addi4spn(DisasContext *ctx, arg_c_addi4spn *a,
> +        uint16_t insn)
> +{
> +    if (a->nzuimm == 0) {
> +        /* Reserved in ISA */
> +        gen_exception_illegal(ctx);
> +        return true;

This should definitely be return false, as that kind of instruction overlap is
exactly what the return value is for.  (There is no current support for
overlapping insn patterns, but it is planned.)

> +static bool trans_c_flw_ld(DisasContext *ctx, arg_c_flw_ld *a, uint16_t insn)
> +{
> +#ifdef TARGET_RISCV32
> +    /* C.FLW ( RV32FC-only ) */
> +    arg_c_lw *tmp = g_new0(arg_c_lw, 1);
> +    extract_cl_w(tmp, insn);
> +    arg_flw arg = { .rd = tmp->rd, .rs1 = tmp->rs1, .imm = tmp->uimm };
> +    return trans_flw(ctx, &arg, insn);
> +#else
> +    /* C.LD ( RV64C/RV128C-only ) */
> +    arg_c_fld *tmp = g_new0(arg_c_fld, 1);
> +    extract_cl_d(tmp, insn);
> +    arg_ld arg = { .rd = tmp->rd, .rs1 = tmp->rs1, .imm = tmp->uimm };
> +    return trans_ld(ctx, &arg, insn);
> +#endif

Oh I see what you're after wrt manual parsing above.
But why g_new0 and missing free.  Why not stack allocation?

> +static int64_t ex_rvc_register(int reg)

return type should be int.


r~