On 04/27/2011 01:09 AM, Ulrich Weigand wrote:
>> > +  dsc->u.ldst.writeback = writeback;
>> > +  dsc->u.ldst.rn = rn;
>> > +
>> >    dsc->tmp[0] = displaced_read_reg (regs, dsc, 0);
>> > -  rn_val = displaced_read_reg (regs, dsc, rn);
>> > +  rn_val = displaced_read_reg (regs, dsc, dsc->u.ldst.rn);
>> >    displaced_write_reg (regs, dsc, 0, rn_val, CANNOT_WRITE_PC);
>> >  
>> > -  dsc->u.ldst.writeback = bit (insn, 25);
>> > -  dsc->u.ldst.rn = rn;
> These changes are really unnecessary now, except for replacing
> "bit (insn, 25)" by "writeback" at the end.  Using the local
> arguments "rn" instead of struct accesses like "dsc->u.ldst.rn"
> keeps the code more readable (and the patch smaller), so it
> would be somewhat preferable.  Similar unnecessary changes
> are in a couple of other install_ routines.

These unnecessary changes in install_copy_copro_load_store,
install_bx_blx_reg, install_alu_reg, arm_copy_alu_reg,
install_alu_shifted_reg, and install_ldr_str_ldrb_strb are fixed.

> For some reason, just three of the copy_ routines are not renamed
> to arm_copy_ (even though they are of course ARM specific):
>  copy_extra_ld_st, copy_block_xfer, copy_unpred
> Please rename those as well (and any others I may have missed).

Renamed them to arm_copy_*.

> 
> 
> Finally, the patch renames two of the "decode_" routines to "arm_decode_",
> but not the others.  Shouldn't they all be renamed?  (Of course this
> is really independent of the rest of the patch, so maybe all those
> renamed ought to be done in a separate patch.)
> 

Renamed them to arm_decode_*.

-- 
Yao (齐尧)