On 10/11/2016 10:12 AM, Peter Maydell wrote: > On 11 October 2016 at 16:51, Thomas Hanson <thomas.han...@linaro.org> wrote: >> On 5 October 2016 at 16:01, Peter Maydell <peter.mayd...@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.
