Re: [Qemu-devel] [PATCH 04/18] target-riscv: Add framework for instruction decode

[Richard Henderson]

On 09/26/2016 03:56 AM, Sagar Karandikar wrote:
> +/* THIS BUILDS 13 bit imm (implicit zero is tacked on here), also note that bit
> +   #12 is obtained in a special way to get sign extension */
> +#define GET_B_IMM(inst)              ((int16_t)((((inst >> 25) & 0x3F) << 5)\
> +                                     | ((((int32_t)inst) >> 31) << 12)      \
> +                                     | (((inst >> 8) & 0xF) << 1)           \
> +                                     | (((inst >> 7) & 0x1) << 11)))
> +
> +#define GET_STORE_IMM(inst)          ((int16_t)(((((int32_t)inst) >> 25) << 5)\
> +                                     | ((inst >> 7) & 0x1F)))
> +#define GET_JAL_IMM(inst)            ((int32_t)((inst & 0xFF000) \
> +                                     | (((inst >> 20) & 0x1) << 11)\
> +                                     | (((inst >> 21) & 0x3FF) << 1)\
> +                                     | ((((int32_t)inst) >> 31) << 20)))
> +#define GET_RM(inst)                 ((inst >> 12) & 0x7)
> +#define GET_RS3(inst)                ((inst >> 27) & 0x1f)
> +#define GET_RS1(inst)                ((inst >> 15) & 0x1f)
> +#define GET_RS2(inst)                ((inst >> 20) & 0x1f)
> +#define GET_RD(inst)                 ((inst >> 7) & 0x1f)
> +#define GET_IMM(inst)                ((int16_t)(((int32_t)inst) >> 20))

I would prefer that these use extract32 and sextract32.  Then you don't need 
the comment there at the top because it's self-evident.

> +    switch (op) {
> +    case OPC_RISC_LUI:
> +        if (rd == 0) {
> +            break; /* NOP */
> +        }
> +        tcg_gen_movi_tl(cpu_gpr[rd], (ctx->opcode & 0xFFFFF000));
> +        tcg_gen_ext32s_tl(cpu_gpr[rd], cpu_gpr[rd]);

This should be done in one movi with the sign-extension in C.

> +        break;
> +    case OPC_RISC_AUIPC:
> +        if (rd == 0) {
> +            break; /* NOP */
> +        }
> +        tcg_gen_movi_tl(cpu_gpr[rd], (ctx->opcode & 0xFFFFF000));
> +        tcg_gen_ext32s_tl(cpu_gpr[rd], cpu_gpr[rd]);
> +        tcg_gen_addi_tl(cpu_gpr[rd], cpu_gpr[rd], ctx->pc);

Likewise.

> +        break;
> +    case OPC_RISC_JAL: {
> +            TCGv nextpc = tcg_temp_local_new();
> +            TCGv testpc = tcg_temp_local_new();
> +            TCGLabel *misaligned = gen_new_label();
> +            TCGLabel *done = gen_new_label();
> +            ubimm = (target_long) (GET_JAL_IMM(ctx->opcode));
> +            tcg_gen_movi_tl(nextpc, ctx->pc + ubimm);
> +            /* check misaligned: */
> +            tcg_gen_andi_tl(testpc, nextpc, 0x3);
> +            tcg_gen_brcondi_tl(TCG_COND_NE, testpc, 0x0, misaligned);
> +            if (rd != 0) {
> +                tcg_gen_movi_tl(cpu_gpr[rd], ctx->pc + 4);
> +            }
> +
> +#ifdef DISABLE_CHAINING_JAL
> +            tcg_gen_mov_tl(cpu_PC, nextpc);
> +            tcg_gen_exit_tb(0);
> +#else
> +            gen_goto_tb(ctx, 0, ctx->pc + ubimm); /* must use this for safety 
> */
> +#endif
> +            tcg_gen_br(done);
> +            gen_set_label(misaligned);
> +            /* throw exception for misaligned case */
> +            generate_exception_mbadaddr(ctx, RISCV_EXCP_INST_ADDR_MIS);
> +            gen_set_label(done);
> +            ctx->bstate = BS_BRANCH;
> +            tcg_temp_free(nextpc);
> +            tcg_temp_free(testpc);

Remember that nextpc is a translate-time constant.  Therefore this entire 
misalignment check could be done during translation, and thus there should be 
no tcg labels nor tcg temporaries allocated.

Why would you ever want to disable chaining JAL (and therefore J)?  Do note 
that there is a -d nochain command-line switch that will disable all chaining 
for the purposes of debugging...


r~