Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension

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

On 1/14/22 7:20 AM, Philipp Tomsich wrote:
> new file mode 100644
> index 0000000000..b8a5d031b5
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_xventanacondops.inc

The filename suffix should be ".c.inc".

> +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> +{
> +    TCGv dest = dest_gpr(ctx, a->rd);
> +    TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> +    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +
> +    tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> +
> +    gen_set_gpr(ctx, a->rd, dest);
> +    return true;
> +}
> +
> +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> +{
> +    return gen_condmask(ctx, a, TCG_COND_NE);
> +}
> +
> +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> +{
> +    return gen_condmask(ctx, a, TCG_COND_EQ);
> +}

Implementation looks good.

> +#define MATERIALISE_EXT_PREDICATE(ext)  \
> +    static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> +                                         DisasContext *ctx  __attribute__((__unused__)))  \
> +    { \
> +        return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> +    }

Again, no inline.

Don't look back to RISCV_CPU here.  We shouldn't even have access to that here, as it 
leads to temptation to do invalid things at translation time (this isn't one of them, 
since it only accesses constant state).  What we have been doing is copying ext_foo into 
DisasContext in riscv_tr_init_disas_context.  Though it might be time to revisit that.

Perhaps give the cpu->cfg structure type a name, e.g. RISCVCPUConfig.  Add  "const 
RISCVCPUConfig *cfg" to DisasContext and copy the pointer across in init_disas_context.


r~