Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Tom Hanson <thomas.hanson@linaro.org>]

On 10/11/2016 10:12 AM, Peter Maydell wrote:
> On 11 October 2016 at 16:51, Thomas Hanson <thomas.hanson@linaro.org> wrote:
>> On 5 October 2016 at 16:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 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.
> 
>> 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.
> 
> The 'lower level' stuff here has a general pattern of taking either
> (1) a TCGv or (2) an integer immediate. We should follow that pattern.
> 
>> 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?
> 
> Anything where we found ourselves wanting to do some preliminary
> manipulation of the value before writing it to the PC.
> 
> thanks
> -- PMM
> 

I split gen_a64_set_pc_reg() into 2 funtions, upper that takes a register and lower
that takes a variable.  Patch v3 submitted.