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

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

On 10/12/18 10:30 AM, Bastian Koppelmann wrote:
> +static bool trans_c_addi(DisasContext *ctx, arg_c_addi *a, uint16_t insn)
> +{
> +    if (a->imm == 0) {
> +        return true;
> +    }

return false, I think.

> +static bool trans_c_jal_addiw(DisasContext *ctx, arg_c_jal_addiw *a,
> +        uint16_t insn)
> +{
> +#ifdef TARGET_RISCV32
> +    /* C.JAL */
> +    arg_c_j *tmp = g_new0(arg_c_j, 1);
> +    extract_cj(tmp, insn);

Again with the g_new0 without free, which should use stack.

> +    arg_jal arg = { .rd = 1, .imm = a->imm };
> +    return trans_jal(ctx, &arg, insn);
> +#else
> +    /* C.ADDIW */
> +    arg_addiw arg = { .rd = a->rd, .rs1 = a->rd, .imm = a->imm };
> +    return trans_addiw(ctx, &arg, insn);
> +#endif
> +}
> +
> +static bool trans_c_li(DisasContext *ctx, arg_c_li *a, uint16_t insn)
> +{
> +    if (a->rd == 0) {
> +        return true;
> +    }

return false.

> +static bool trans_c_addi16sp_lui(DisasContext *ctx, arg_c_addi16sp_lui *a,
> +        uint16_t insn)
> +{
> +    if (a->rd == 2) {
> +        /* C.ADDI16SP */
> +        arg_addi arg = { .rd = 2, .rs1 = 2, .imm = a->imm_addi16sp };
> +        return trans_addi(ctx, &arg, insn);
> +    } else if (a->imm_lui != 0) {
> +        if (a->rd == 0) {
> +            return true;
> +        }

I think it should be

  } else if (a->imm_lui != 0 && a->rd != 0) {

> +        /* C.LUI */
> +        arg_lui arg = { .rd = a->rd, .imm = a->imm_lui };
> +        return trans_lui(ctx, &arg, insn);
> +    }
> +    return false;
> +}
> +
> +static bool trans_c_srli(DisasContext *ctx, arg_c_srli *a, uint16_t insn)
> +{
> +    if (a->shamt == 0) {
> +        /* Reserved in ISA */
> +        gen_exception_illegal(ctx);
> +        return true;
> +    }

Choose return false or raise exception.  Except...
I wonder if we might write this as

    int shamt = a->shamt;
    if (shamt == 0) {
        shamt = 64;
    }

> +#ifdef TARGET_RISCV32
> +    /* Ensure, that shamt[5] is zero for RV32 */
> +    if (a->shamt >= 32) {
> +        gen_exception_illegal(ctx);
> +        return true;
> +    }
> +#endif

then this is unconditional as

    if (a->shamt >= TARGET_LONG_BITS)

which makes it clear that "reserved in isa" already has a meaning.

> +
> +    arg_srli arg = { .rd = a->rd, .rs1 = a->rd, .shamt = a->shamt };
> +    return trans_srli(ctx, &arg, insn);

Although I do wonder about moving that check into trans_srli et al, rather than
bend over backward parsing 6-bit shift amount rather just using @i format.


r~