On 24 March 2011 15:58, Alexander Graf <ag...@suse.de> wrote:
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> +typedef struct DisasContext DisasContext;
> +struct DisasContext {
> + uint64_t pc;
> + int is_jmp;
> + enum cc_op cc_op;
> + CPUS390XState *env;
> + struct TranslationBlock *tb;
> +};
I don't think anything actually uses dc->env, does it?
(I like the way that almost none of the translate.c code
gets a CPUState pointer, makes it hard to accidentally write
buggy code that relies on things not in the tb_flags.)
> +static char cpu_reg_names[10*3 + 6*4];
I can see code ins390x_translate_init() which sets this up, but
I can't see anything which uses it?
> +#if 0 /* reads four when it should read only 3 */
> + case 2:
Is there any point having #if'd out broken code?
Either fix it and enable it, or just have a comment
to the effect that we could have optimised versions
for cases 2, 4, 5, 6 but currently do not.
> + case 0x4: /* LMG R1,R3,D2(B2) [RSE] */
> + case 0x24: /* STMG R1,R3,D2(B2) [RSE] */
> + case 0x26: /* STMH R1,R3,D2(B2) [RSE] */
> + case 0x96: /* LMH R1,R3,D2(B2) [RSE] */
> + /* Apparently, unrolling lmg/stmg of any size gains performance -
> + even for very long ones... */
Doesn't this take you over MAX_OP_PER_INSTR for some cases?
> + tmp2 = tcg_const_i64((((uint64_t)i2) << 48) |
> 0x0000ffffffffffffULL);
This line is over 80 chars, as are a handful of others in this file.
> + case 0xa: /* SVC I [RR] */
> + insn = ld_code2(s->pc);
> + debug_insn(insn);
> + i = insn & 0xff;
> +#ifdef CONFIG_USER_ONLY
> + s->pc += 2;
> +#endif
> + update_psw_addr(s);
> + gen_op_calc_cc(s);
Why do we only need to update s->pc if CONFIG_USER_ONLY?
Not saying it's wrong, but it could use an explanatory comment...
-- PMM
