[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 08/21] RISC-V TCG Code Generation
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH v1 08/21] RISC-V TCG Code Generation |
Date: |
Wed, 3 Jan 2018 13:35:49 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 |
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~
- [Qemu-devel] [PATCH v1 04/21] RISC-V Disassembler, (continued)
- [Qemu-devel] [PATCH v1 04/21] RISC-V Disassembler, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 09/21] RISC-V Physical Memory Protection, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 12/21] RISC-V HART Array, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 08/21] RISC-V TCG Code Generation, Michael Clark, 2018/01/02
- Re: [Qemu-devel] [PATCH v1 08/21] RISC-V TCG Code Generation,
Richard Henderson <=
- [Qemu-devel] [PATCH v1 10/21] RISC-V Linux User Emulation, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 13/21] SiFive RISC-V CLINT Block, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 14/21] SiFive RISC-V PLIC Block, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 11/21] RISC-V HTIF Console, Michael Clark, 2018/01/02
- [Qemu-devel] [PATCH v1 15/21] RISC-V Spike Machines, Michael Clark, 2018/01/02