[Qemu-devel] Re: [PATCH 02/17] lm32: translation routines

[Michael Walle <michael@walle.cc>]

Hi Edgar.

first of all let me thank you for the review.

On Mon, Feb 07 2011, 19:41:25, Edgar E. Iglesias wrote:
> > +#define JMP_NOJMP    0
> > +#define JMP_DIRECT   1
> > +#define JMP_INDIRECT 2
> 
> These don't seem to be used, can we remove them?
Good catch :) I'll remove them.

> > +
> > +/* This is the state at translation time.  */
> > +typedef struct DisasContext {
> > +    CPUState *env;
> > +    target_ulong pc;
> > +
> > +    /* Decoder.  */
> > +    int format;
> > +    uint32_t ir;
> > +    uint8_t opcode;
> > +    uint8_t r0, r1, r2, csr;
> > +    uint16_t imm5;
> > +    uint16_t imm16;
> > +    uint32_t imm26;
> > +
> > +    unsigned int delayed_branch;
> > +    unsigned int tb_flags, synced_flags; /* tb dependent flags.  */
> > +    int is_jmp;
> > +
> > +    unsigned int jmp;
> > +    uint32_t jmp_pc;
> 
> These too.
Ditto.

> > +
> > +    int nr_nops;
> 
> This should probably go aswell..
See below.

> > +    /* try guessing 'empty' instruction memory, although it may be a
> > valid +     * instruction sequence (eg. srui r0, r0, 0) */
> > +    if (dc->ir) {
> > +        dc->nr_nops = 0;
> > +    } else {
> > +        LOG_DIS("nr_nops=%d\t", dc->nr_nops);
> > +        dc->nr_nops++;
> > +        if (dc->nr_nops > 4) {
> > +            cpu_abort(dc->env, "fetching nop sequence\n");
> > +        }
> > +    }
> 
> This nop sequence detection should probably go away. A reminder for
> me too to remove it from the microblaze port (from where I guess you
> inherited it)..
I've always found this 'feature' handy, esp when you are porting new software. 
Is there any particular reason why this should be removed? Besides the fact 
that it won't match the real hardware if those instructions are executed. In 
the case of the LM32, these are really nonsense instructions (shift r0 right 
by 0, r0 is by definition 0).

-- 
Michael