On 5 July 2011 10:19, <kha...@kics.edu.pk> wrote:
> ---
> host-utils.c | 1 +
> target-mips/cpu.h | 7 +
> target-mips/helper.h | 5 +
> target-mips/op_helper.c | 67 +++++++
> target-mips/translate.c | 443
> ++++++++++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 514 insertions(+), 9 deletions(-)
Don't you also need to add support for the new instructions
to the disassembler in mips-dis.c ?
> diff --git a/host-utils.c b/host-utils.c
> index dc96123..1128698 100644
> --- a/host-utils.c
> +++ b/host-utils.c
> @@ -102,4 +102,5 @@ void muls64 (uint64_t *plow, uint64_t *phigh, int64_t a,
> int64_t b)
> a, b, *phigh, *plow);
> #endif
> }
> +
> #endif /* !defined(__x86_64__) */
Stray random whitespace change to an unrelated file:
please drop this from the patch.
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index b0ac4da..8e75e9b 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -171,6 +171,13 @@ struct TCState {
> target_ulong CP0_TCSchedule;
> target_ulong CP0_TCScheFBack;
> int32_t CP0_Debug_tcstatus;
> + /* Multiplier registers for Octeon */
> + target_ulong MPL0;
> + target_ulong MPL1;
> + target_ulong MPL2;
> + target_ulong P0;
> + target_ulong P1;
> + target_ulong P2;
> };
If you add new fields to the CPU struct then you must also
add code to save/restore them in target-mips/machine.c.
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 6b966b1..a1893d1 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -266,7 +266,74 @@ void helper_dmultu (target_ulong arg1, target_ulong arg2)
> {
> mulu64(&(env->active_tc.LO[0]), &(env->active_tc.HI[0]), arg1, arg2);
> }
> +static void addc(uint64_t res[], uint64_t a, int i)
Can you leave blank lines between function definitions, please?
(here and also in a few places below)
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index eb108bc..b480665 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -69,6 +69,11 @@ enum {
> OPC_JAL = (0x03 << 26),
> OPC_JALS = OPC_JAL | 0x5,
> OPC_BEQ = (0x04 << 26), /* Unconditional if rs = rt = 0 (B) */
> + /* Cavium Specific */
> + OPC_BBIT1 = (0x3a << 26), /* jump on bit set, cavium specific */
> + OPC_BBIT132 = (0x3e << 26), /* jump on bit set(for upper 32 bits) */
Space before the '(' in the comment, please.
> + OPC_BBIT0 = (0x32 << 26), /* jump on bit clear, cavium specific */
> + OPC_BBIT032 = (0x36 << 26), /* jump on bit clear(for upper 32 bits) */
Ditto.
> @@ -482,7 +512,7 @@ enum {
> static TCGv_ptr cpu_env;
> static TCGv cpu_gpr[32], cpu_PC;
> static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC],
> cpu_ACX[MIPS_DSP_ACC];
> -static TCGv cpu_dspctrl, btarget, bcond;
> +static TCGv cpu_dspctrl, btarget, bcond, mpl0, mpl1, mpl2, p0, p1, p2;
p0/p1/p2 are awful names for global variables -- far too short.
mpl0/mpl1/mpl2 aren't a great deal better.
Also it looks like at least some of these aren't really used very
often -- you should consider just doing loads/stores to env+offset
the way we do for most other cpu fields.
> +/* set on equal/not equal immidiate */
> +static void gen_set_imm(CPUState *env, uint32_t opc,
> + int rt, int rs, int16_t imm)
> +{
> + target_ulong uimm = (target_long)imm;
> + TCGv t0;
> + const char *opn = "imm set";
> + if (rt == 0) {
> + /* If no destination, treat it as a NOP. */
> + MIPS_DEBUG("NOP");
> + return;
> + }
> + t0 = tcg_temp_new();
> + gen_load_gpr(t0, rs);
> + switch (opc) {
> + case OPC_SEQI:
> + tcg_gen_xori_tl(t0, t0, uimm);
> + tcg_gen_setcondi_tl(TCG_COND_LT, cpu_gpr[rt], t0, 1);
> + opn = "seqi";
> + break;
> + case OPC_SNEI:
> + tcg_gen_xori_tl(t0, t0, uimm);
> + tcg_gen_setcondi_tl(TCG_COND_GT, cpu_gpr[rt], t0, 0);
> + opn = "snei";
> + break;
> + }
> + tcg_temp_free(t0);
> +}
The tcg_gen_xori_tl() is the same in both cases of the switch, you
could pull it out of the switch.
> @@ -1636,6 +1881,30 @@ static void gen_arith (CPUState *env, DisasContext
> *ctx, uint32_t opc,
> }
> opn = "addu";
> break;
> + case OPC_BADDU:
> + {
> + TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> + gen_load_gpr(t0, rs);
> + gen_load_gpr(t1, rt);
> + tcg_gen_add_tl(t0, t1, t0);
> + tcg_gen_ext8u_tl(t0, t0);
> + gen_store_gpr(t0, rd);
> + tcg_temp_free(t0);
> + tcg_temp_free(t1);
> + }
> + opn = "baddu";
> + break;
These should go inside the braces, please [ditto for other cases below]
> + case OPC_DMUL:
> + {
> + TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> + gen_load_gpr(t0, rs);
> + gen_load_gpr(t1, rt);
> + tcg_gen_mul_i64(cpu_gpr[rd], t0, t1);
> + }
> + opn = "dmul";
> + break;
Missing tcg_temp_free()s ?
> case OPC_SUB:
> {
> TCGv t0 = tcg_temp_local_new();
> @@ -2704,6 +2973,7 @@ static void gen_compute_branch (DisasContext *ctx,
> uint32_t opc,
> target_ulong btgt = -1;
> int blink = 0;
> int bcond_compute = 0;
> + target_ulong maskb; /* Used in BBIT0 and BBIT1 */
Add braces to the relevant cases and use variables local to that
restricted scope -- there's no need for this to be visible for the
whole function.
> + case OPC_BBIT1:
> + case OPC_BBIT132:
> + tcg_gen_setcondi_tl(TCG_COND_NE, bcond, t0, 0);
> + goto not_likely;
> + case OPC_BBIT0:
> + case OPC_BBIT032:
> + tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, t0, 0);
> + goto not_likely;
Indentation on the OPC_BBIT0, OPC_BBIT032 cases is wrong.
> @@ -11637,6 +11946,9 @@ static void decode_opc (CPUState *env, DisasContext
> *ctx, int *is_branch)
> rd = (ctx->opcode >> 11) & 0x1f;
> sa = (ctx->opcode >> 6) & 0x1f;
> imm = (int16_t)ctx->opcode;
> + /* 10 bit Immediate value For SEQI,SNEI */
> + imm10 = (ctx->opcode >> 6) & 0x3ff;
> +
Only used inside a single case, so just make it a variable scoped
only to that case and decode it from opcode at that point.
> @@ -11862,6 +12174,58 @@ static void decode_opc (CPUState *env, DisasContext
> *ctx, int *is_branch)
> case OPC_MUL:
> gen_arith(env, ctx, op1, rd, rs, rt);
> break;
> +#if defined(TARGET_MIPS64)
> +
> + case OPC_DMUL:
This blank line is unnecessary.
> @@ -11881,13 +12245,24 @@ static void decode_opc (CPUState *env, DisasContext
> *ctx, int *is_branch)
> break;
> case OPC_DIV_G_2F:
> case OPC_DIVU_G_2F:
> - case OPC_MULT_G_2F:
> case OPC_MULTU_G_2F:
> case OPC_MOD_G_2F:
> case OPC_MODU_G_2F:
> check_insn(env, ctx, INSN_LOONGSON2F);
> gen_loongson_integer(ctx, op1, rd, rs, rt);
> break;
> + case OPC_MULT_G_2F:
> + if (!(env->insn_flags & CPU_OCTEON)) {
> + check_insn(env, ctx, INSN_LOONGSON2F);
> + gen_loongson_integer(ctx, op1, rd, rs, rt);
> + } else {
> +#if defined(TARGET_MIPS64)
> + /* Cavium Specific vmm0 */
> + check_mips_64(ctx);
> + gen_LMI(env, ctx, op1, rs, rt, rd);
> +#endif
> + }
> + break;
You could rearrange this to:
case OPC_MULT_G_2F:
#if defined(TARGET_MIPS64)
if (OCTEON) {
check_mips_64(ctx);
gen_LMI(...);
}
#endif
/* Otherwise fall through, this is also a Loongson insn */
case OPC_DIV_G_2F:
case OPC_DIVU_G_2F:
[etc]
check_insn(env, ctx, INSN_LOONGSON2F);
gen_loongson_integer(ctx, op1, rd, rs, rt);
...which lets you avoid duplicating the loongson code.
Ditto in the DMULT/DDIV case.
-- PMM
