qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overw


From: Thomas Hanson
Subject: Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load
Date: Tue, 11 Oct 2016 09:51:05 -0600

On 5 October 2016 at 16:01, Peter Maydell <address@hidden> wrote:

> On 5 October 2016 at 14:53, Tom Hanson <address@hidden> wrote:
> > On 09/29/2016 07:24 PM, Peter Maydell wrote:
> >> On 16 September 2016 at 10:34, Thomas Hanson <address@hidden>
> wrote:
> >>> +void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
> >>
> >> I think it would be better to take a TCGv_i64 here rather than
> >> unsigned int rn (ie have the caller do the cpu_reg(s, rn)).
> >> (You probably don't need that prototype of cpu_reg() above if
> >> you do this, though that's not why it's better.)
> >>
> > Why would this be better?
> >
> > To me, the caller has a register number and wants that register used to
> load
> > the PC.  So, it passes in the register number.
> >
> > The fact that gen_a64_set_pc_reg() needs to convert that into a TCGv_i64
> is
> > an implementation detail that should be encapsulated/hidden from the
> caller.
> >
> > If the desire is to eliminate the multiple cpu_reg() calls inside of
> > gen_a64_set_pc_reg() then that mapping could be done at the top of the
> > function before the outer if().
>
> It matches the style of the rest of the code which generally
> prefers to convert register numbers into TCGv earlier rather
> than later (at the level which is doing decode of instruction
> bits, rather than inside utility functions), and gives you a
> more flexible utility function, which can do a "write value to PC"
> for any value, not just something that happens to be in a CPU
> register. And as you say it avoids calling cpu_reg() multiple times
> as a side benefit.
>
> thanks
> -- PMM
>

Sorry,  didn't see this before I submitted v2.  Even now I can't get
Thunderbird to display it.  Sigh.  (Apologies if this comes through with
bars on the side, I have to use the web mail interface in order to be able
to respond.)

This approach seems counter to both structured and OO design principles
which would push common code (like type conversion) down into the lower
level function in order to increase re-use and minimize code duplication.
Those principles suggest that if we need a gen_a64_set_pc_value() function
that can load the PC from something other than a register or an immediate,
then it should be a lower level function than, and be called by,
gen_a64_set_pc_reg().  This also has the benefit of reducing clutter in the
caller, making it more readable and more maintainable.

As a separate issue, we now have functions to load the PC from an immediate
value and from a register.  Where else could we legitimately load the PC
from?


reply via email to

[Prev in Thread] Current Thread [Next in Thread]