[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/35] target/mips: Add decode_nanomips_opc()
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH 04/35] target/mips: Add decode_nanomips_opc() |
Date: |
Thu, 21 Jun 2018 16:39:00 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 06/20/2018 05:05 AM, Yongbok Kim wrote:
> +static int decode_nanomips_opc(CPUMIPSState *env, DisasContext *ctx)
> +{
> + uint32_t op;
> + int rt = mmreg_nanomips(uMIPS_RD(ctx->opcode));
> + int rs = mmreg_nanomips(uMIPS_RS(ctx->opcode));
> + int rd = mmreg_nanomips(uMIPS_RS1(ctx->opcode));
Do you really want to be reusing micro-mips macros for nano-mips?
Even if they are the same?
> +
> + /* make sure instructions are on a halfword boundary */
> + if (ctx->base.pc_next & 0x1) {
> + env->CP0_BadVAddr = ctx->base.pc_next;
> + generate_exception_end(ctx, EXCP_AdEL);
> + return 2;
> + }
> +
> + op = (ctx->opcode >> 10) & 0x3f;
> + switch (op) {
> + case NM_P16_MV:
> + {
> + int rt = uMIPS_RD5(ctx->opcode);
The formatting here is off. It should be
switch (op) {
case NM_P16_MV:
{
int rt ...
but it might be even better to split each case off into a separate function.
For the most part that could become
return table[op](ctx, insn);
where the table is fully populated with function pointers. It would have the
same indirect branch predictability of the switch statement while producing
smaller functions that are easier to reason with.
> + if (rt != 0) {
> + /* MOVE */
> + int rs = uMIPS_RS5(ctx->opcode);
> + gen_arith(ctx, OPC_ADDU, rt, rs, 0);
> + } else {
> + /* P16.RI */
> + switch ((ctx->opcode >> 3) & 0x3) {
You have an eclectic mix of shift-and-mask and extract32 throughout.
I would prefer if you standardized on extract32.
> + case NM_MOVEP:
> + case NM_MOVEPREV:
> + {
> + static const int gpr2reg1[] = {4, 5, 6, 7};
> + static const int gpr2reg2[] = {5, 6, 7, 8};
> + int re;
> + int rd2 = extract32(ctx->opcode, 3, 1) << 1 |
> + extract32(ctx->opcode, 8, 1);
> + int r1 = gpr2reg1[rd2];
> + int r2 = gpr2reg2[rd2];
> + int r3 = extract32(ctx->opcode, 4, 1) << 3 |
> + extract32(ctx->opcode, 0, 3);
> + int r4 = extract32(ctx->opcode, 9, 1) << 3 |
> + extract32(ctx->opcode, 5, 3);
> + TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> + if (op == NM_MOVEP) {
> + rd = r1;
> + re = r2;
> + rs = mmreg4z_nanomips(r3);
> + rt = mmreg4z_nanomips(r4);
> + } else {
> + rd = mmreg4_nanomips(r3);
> + re = mmreg4_nanomips(r4);
> + rs = r1;
> + rt = r2;
> + }
> + gen_load_gpr(t0, rs);
> + gen_load_gpr(t1, rt);
> + tcg_gen_mov_tl(cpu_gpr[rd], t0);
> + tcg_gen_mov_tl(cpu_gpr[re], t1);
> + tcg_temp_free(t0);
> + tcg_temp_free(t1);
> + }
I would encourage you to qemu_log_mask(LOG_GUEST_ERROR, ...) whenever the
result of the instruction is UNKNOWN or UNPREDICTABLE, as here when
destinations overlap sources.
> } else if (ctx->insn_flags & ASE_MICROMIPS) {
> ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> - insn_bytes = decode_micromips_opc(env, ctx);
> + if (env->insn_flags & ISA_NANOMIPS32) {
> + insn_bytes = decode_nanomips_opc(env, ctx);
> + } else {
> + insn_bytes = decode_micromips_opc(env, ctx);
> + }
Clearer without useless sharing of one line:
} else if (env->insn_flags & ISA_NANOMIPS32) {
ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
insn_bytes = decode_nanomips_opc(env, ctx);
} else if (ctx->insn_flags & ASE_MICROMIPS) {
ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
insn_bytes = decode_micromips_opc(env, ctx);
}
r~
- [Qemu-devel] [PATCH 00/35] nanoMIPS, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 01/35] target/mips: Raise a RI when given fs is n/a from CTC1, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 02/35] target/mips: Fix microMIPS on reset, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 03/35] target/mips: Add nanoMIPS OPCODE table, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 04/35] target/mips: Add decode_nanomips_opc(), Yongbok Kim, 2018/06/20
- Re: [Qemu-devel] [PATCH 04/35] target/mips: Add decode_nanomips_opc(),
Richard Henderson <=
- [Qemu-devel] [PATCH 05/35] target/mips: Add nanoMIPS 16bit ld/st instructions, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 06/35] target/mips: Add nanoMIPS pool16c instructions, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 07/35] target/mips: Add nanoMIPS save and restore, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 08/35] target/mips: Add nanoMIPS 32bit instructions, Yongbok Kim, 2018/06/20
- [Qemu-devel] [PATCH 09/35] target/mips: Add nanoMIPS 48bit instructions, Yongbok Kim, 2018/06/20