Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode

[Blue Swirl]

On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell <address@hidden> wrote:
> On 3 September 2012 01:01, Peter Maydell <address@hidden> wrote:
>> On 2 September 2012 18:33, Blue Swirl <address@hidden> wrote:
>>> Add an explicit CPUState parameter instead of relying on AREG0
>>> and switch to AREG0 free mode.
>>>
>>> Signed-off-by: Blue Swirl <address@hidden>
>>> ---
>>>  configure                |    2 +-
>>>  target-arm/Makefile.objs |    2 -
>>>  target-arm/cpu.h         |   10 ++-
>>>  target-arm/helper.c      |    8 +-
>>>  target-arm/helper.h      |   60 +++++++++---------
>>>  target-arm/op_helper.c   |   92 +++++++++++++---------------
>>>  target-arm/translate.c   |  148 
>>> +++++++++++++++++++++++-----------------------
>>>  7 files changed, 158 insertions(+), 164 deletions(-)
>>
>> This is too big to easily review -- it's making a change to a lot
>> of helpers, and in each case that change affects three places
>> (callers, declaration, implementation). That'
>
> Sorry, finger slip meant I sent that half finished. To continue...
>
> That's quite hard to cross-reference when the patch is this big.
> I think it would be helpful if you could split it up into patches
> touching smaller groups of helpers at once rather than having a
> single patch that does them all at once.

For x86, Sparc and s390x I used the approach of splitting op_helper.c
to smaller files first. I didn't do it for ARM since
target-arm/op_helper.c is alread pretty small (<500 lines). It could
be split to saturating ops, condition code setting arithmetic ops and
misc ops, between 100 and 200 lines each. Would that be OK?

It looks like helper.c should be split too (maybe VFP, MMU, CPU init,
CPR), but that's starting to get beyond the scope of the series.

>
> thanks
> -- PMM