Re: [Qemu-devel] [PATCH v1 08/21] RISC-V TCG Code Generation

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

On 01/02/2018 04:44 PM, Michael Clark wrote:
> +typedef struct DisasContext {
> +    struct TranslationBlock *tb;
> +    target_ulong pc;
> +    target_ulong next_pc;
> +    uint32_t opcode;
> +    int singlestep_enabled;
> +    int mem_idx;
> +    int bstate;
> +} DisasContext;
> +
> +static inline void kill_unknown(DisasContext *ctx, int excp);
> +
> +enum {
> +    BS_NONE     = 0, /* When seen outside of translation while loop, indicates
> +                     need to exit tb due to end of page. */
> +    BS_STOP     = 1, /* Need to exit tb for syscall, sret, etc. */
> +    BS_BRANCH   = 2, /* Need to exit tb for branch, jal, etc. */
> +};

I would prefer this to be updated to use exec/translator.h, TranslatorOps.
However, please use DisasJumpType and DisasContextBase right away.


> +static inline void generate_exception(DisasContext *ctx, int excp)
> +{
> +    tcg_gen_movi_tl(cpu_pc, ctx->pc);
> +    TCGv_i32 helper_tmp = tcg_const_i32(excp);
> +    gen_helper_raise_exception(cpu_env, helper_tmp);
> +    tcg_temp_free_i32(helper_tmp);
> +}

Drop inline unless required.  Let the compiler choose.

> +static inline void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> +{
> +    if (use_goto_tb(ctx, dest)) {
> +        /* chaining is only allowed when the jump is to the same page */
> +        tcg_gen_goto_tb(n);
> +        tcg_gen_movi_tl(cpu_pc, dest);
> +        tcg_gen_exit_tb((uintptr_t)ctx->tb + n);
> +    } else {
> +        tcg_gen_movi_tl(cpu_pc, dest);
> +        if (ctx->singlestep_enabled) {
> +            gen_helper_raise_exception_debug(cpu_env);
> +        }

else

> +        tcg_gen_exit_tb(0);

tcg_gen_lookup_and_goto_ptr

and set ctx->base.is_jmp = DISAS_NORETURN.

> +static void gen_fsgnj(DisasContext *ctx, uint32_t rd, uint32_t rs1,
> +    uint32_t rs2, int rm, uint64_t min)
> +{
> +    TCGv t0 = tcg_temp_new();
> +#if !defined(CONFIG_USER_ONLY)
> +    TCGLabel *fp_ok = gen_new_label();
> +    TCGLabel *done = gen_new_label();
> +
> +    /* check MSTATUS.FS */
> +    tcg_gen_ld_tl(t0, cpu_env, offsetof(CPURISCVState, mstatus));
> +    tcg_gen_andi_tl(t0, t0, MSTATUS_FS);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, t0, 0x0, fp_ok);
> +    /* MSTATUS_FS field was zero */
> +    kill_unknown(ctx, RISCV_EXCP_ILLEGAL_INST);
> +    tcg_gen_br(done);

As I suggested for the FP helper patch, this could become

  if (!(ctx->base.tb->flags & TB_FLAG_MSTATUS_FS)) {
    kill_unknown(...);
    return;
  }

> +#if defined(TARGET_RISCV64)
> +    case OPC_RISC_SRAW:
> +        /* first, trick to get it to act like working on 32 bits (get rid of
> +        upper 32, sign extend to fill space) */
> +        tcg_gen_ext32s_tl(source1, source1);
> +        tcg_gen_andi_tl(source2, source2, 0x1F);
> +        tcg_gen_sar_tl(source1, source1, source2);
> +        break;
> +        /* fall through to SRA */

fallthru after break?

> +        /* Handle by altering args to tcg_gen_div to produce req'd results:
> +         * For overflow: want source1 in source1 and 1 in source2
> +         * For div by zero: want -1 in source1 and 1 in source2 -> -1 result */
> +        cond1 = tcg_temp_new();
> +        cond2 = tcg_temp_new();
> +        zeroreg = tcg_const_tl(0);
> +        resultopt1 = tcg_temp_new();
> +
> +        tcg_gen_movi_tl(resultopt1, (target_ulong)-1);

Pointless cast.

> +        tcg_gen_setcondi_tl(TCG_COND_EQ, cond2, source2, (target_ulong)(~0L));

Pointless cast and pointless L.

> +        tcg_gen_movi_tl(resultopt1, (target_ulong)1);
> +        tcg_gen_movcond_tl(TCG_COND_EQ, source2, cond1, zeroreg, source2,
> +                resultopt1);

You can use cond1 instead of resultopt1 here, since cond1 is either 0 or 1.

> +        tcg_gen_setcondi_tl(TCG_COND_EQ, cond1, source2, 0);
> +        tcg_gen_movi_tl(resultopt1, (target_ulong)-1);
> +        tcg_gen_movcond_tl(TCG_COND_EQ, source1, cond1, zeroreg, source1,
> +                resultopt1);
> +        tcg_gen_movi_tl(resultopt1, (target_ulong)1);
> +        tcg_gen_movcond_tl(TCG_COND_EQ, source2, cond1, zeroreg, source2,
> +                resultopt1);

The setcond is useless.  Let the movconds use source2 directly.

Similarly in REM and REMU.

> +    case OPC_RISC_ADDI:
> +#if defined(TARGET_RISCV64)
> +    case OPC_RISC_ADDIW:
> +#endif

CASE_OP_32_64?

> +    gen_goto_tb(ctx, 0, ctx->pc + imm); /* must use this for safety */

What do you mean by this?

> +    /* no chaining with JALR */

We can now chain indirect branches.

> +    TCGLabel *misaligned = gen_new_label();
> +    TCGv t0;
> +    t0 = tcg_temp_new();

These may not be needed...

> +
> +    switch (opc) {
> +    case OPC_RISC_JALR:
> +        gen_get_gpr(cpu_pc, rs1);
> +        tcg_gen_addi_tl(cpu_pc, cpu_pc, imm);
> +        tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
> +
> +        if (!riscv_feature(env, RISCV_FEATURE_RVC)) {

... unless this condition is true.  In particular you can avoid emitting the
label unless it is used.

> +            tcg_gen_andi_tl(t0, cpu_pc, 0x2);
> +            tcg_gen_brcondi_tl(TCG_COND_NE, t0, 0x0, misaligned);
> +        }
> +
> +        if (rd != 0) {
> +            tcg_gen_movi_tl(cpu_gpr[rd], ctx->next_pc);
> +        }
> +        tcg_gen_exit_tb(0);

tcg_gen_lookup_and_goto_ptr

> +
> +        gen_set_label(misaligned);
> +        generate_exception_mbadaddr(ctx, RISCV_EXCP_INST_ADDR_MIS);
> +        tcg_gen_exit_tb(0);

exit_tb is unreached, since the exception exits the loop.

> +    switch (opc) {
> +    case OPC_RISC_BEQ:
> +        tcg_gen_brcond_tl(TCG_COND_EQ, source1, source2, l);
> +        break;
> +    case OPC_RISC_BNE:
> +        tcg_gen_brcond_tl(TCG_COND_NE, source1, source2, l);
> +        break;
> +    case OPC_RISC_BLT:
> +        tcg_gen_brcond_tl(TCG_COND_LT, source1, source2, l);
> +        break;
> +    case OPC_RISC_BGE:
> +        tcg_gen_brcond_tl(TCG_COND_GE, source1, source2, l);
> +        break;
> +    case OPC_RISC_BLTU:
> +        tcg_gen_brcond_tl(TCG_COND_LTU, source1, source2, l);
> +        break;
> +    case OPC_RISC_BGEU:
> +        tcg_gen_brcond_tl(TCG_COND_GEU, source1, source2, l);
> +        break;

You could just set the condition in the switch, or load it from a table.

> +    gen_goto_tb(ctx, 1, ctx->next_pc);
> +    gen_set_label(l); /* branch taken */
> +    if (!riscv_feature(env, RISCV_FEATURE_RVC) && ((ctx->pc + bimm) & 0x3)) {
> +        /* misaligned */
> +        generate_exception_mbadaddr(ctx, RISCV_EXCP_INST_ADDR_MIS);
> +        tcg_gen_exit_tb(0);

Unreached.

> +static void gen_atomic(DisasContext *ctx, uint32_t opc,
> +                      int rd, int rs1, int rs2)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    /* TODO: handle aq, rl bits? - for now just get rid of them: */
> +    opc = MASK_OP_ATOMIC_NO_AQ_RL(opc);
> +    TCGv source1, source2, dat;
> +    TCGLabel *j = gen_new_label();
> +    TCGLabel *done = gen_new_label();
> +    source1 = tcg_temp_local_new();
> +    source2 = tcg_temp_local_new();
> +    dat = tcg_temp_local_new();
> +    gen_get_gpr(source1, rs1);
> +    gen_get_gpr(source2, rs2);
> +
> +    switch (opc) {
> +        /* all currently implemented as non-atomics */
> +    case OPC_RISC_LR_W:
> +        /* put addr in load_res */
> +        tcg_gen_mov_tl(load_res, source1);
> +        tcg_gen_qemu_ld_tl(dat, source1, ctx->mem_idx, MO_TESL | MO_ALIGN);
> +        break;
> +    case OPC_RISC_SC_W:
> +        tcg_gen_brcond_tl(TCG_COND_NE, load_res, source1, j);
> +        tcg_gen_qemu_st_tl(source2, source1, ctx->mem_idx, MO_TEUL | MO_ALIGN);
> +        tcg_gen_movi_tl(dat, 0); /*success */
> +        tcg_gen_br(done);
> +        gen_set_label(j);
> +        tcg_gen_movi_tl(dat, 1); /*fail */
> +        gen_set_label(done);
> +        break;

We have atomic primitives now.  You'll want to use them.

> +#else
> +    tcg_gen_movi_i32(cpu_amoinsn, ctx->opcode);
> +    generate_exception(ctx, QEMU_USER_EXCP_ATOMIC);
> +#endif

Including for CONFIG_USER_ONLY.  No need for cpu_amoinsn or the exception.

> +#ifndef CONFIG_USER_ONLY
> +        case 0x002: /* URET */
> +            printf("URET unimplemented\n");
> +            exit(1);

No exit, ever.  qemu_log_mask w/ LOG_UNIMP and kill_unknown.

> +#ifndef CONFIG_USER_ONLY
> +        /* standard fence is nop, fence_i flushes TB (like an icache): */

Standard fence should be tcg_gen_mb with some combination of TCGBar bits.

QEMU will automatically detect self-modifying code.  FENCE_I should either be a
nop or another tcg_gen_mb.

> +    ctx.mem_idx = cpu_mmu_index(env, false);

tb->flags must contain enough to cover the bits you're testing here.  Otherwise
hashing may select a TB with different permissions.

Depending on how complicated the test is, another way to accomplish this is to
actually store cpu_mmu_index into tb->flags.  At which point you would retrieve
ctx.mem_idx directly from tb->flags.

> +void riscv_translate_init(void)
> +{
> +    int i;
> +
> +    /* WARNING: cpu_gpr[0] is not allocated ON PURPOSE. Do not use it. */
> +    /* Use the gen_set_gpr and gen_get_gpr helper functions when accessing */
> +    /* registers, unless you specifically block reads/writes to reg 0 */
> +    TCGV_UNUSED(cpu_gpr[0]);

A patch to remove TCGV_UNUSED is in queue.
This would simply be cpu_gpr[0] = NULL now.


r~