Re: [try 2nd 2/8] Rename copy_* functions to arm_copy_*

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Yao Qi wrote:
> On 04/07/2011 04:51 AM, Ulrich Weigand wrote:
> > I'd prefer if this were handled in a regular manner: the copy_
> > routines parse the insn text, extract all required information
> > and pass it all as arguments to the install_ routine.  The
> > install_ routine then stores all information that is needed
> > by the cleanup_ routine into the displaced_step_closure struct.
> 
> From the consistency's point of view, I am fine with your approach.  In
> this new patch, the following things are changed,
> 1.  Merge 6/8 patch,
> 2.  Pass all values as parameters to install_* routines,
> 3.  Fix install_* routines' arguments order,
> 4.  Change all install_* routines to void method.
> 
> This patches makes patch 4/8 and 5/8 obsolete.  I'll re-send an updated
> version once this patch is approved.

Sorry for the delay; I've been out of the office for a couple of weeks.

Thanks for making those changes, this is looking very good now.  There's
just a couple of minor issues:


> +  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.


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).


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.)


Otherwise, this looks OK now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com