Re: [Qemu-devel] [PATCH 2/5] softmmu templates: optionally pass CPUState to memory access functions

[Aurelien Jarno <aurelien@aurel32.net>]

On Sun, Mar 11, 2012 at 10:24:03PM +0000, Blue Swirl wrote:
> Optionally, make memory access helpers take a parameter for CPUState
> instead of relying on global env.
> 
> On most targets, perform simple moves to reorder registers. On i386,
> switch from regparm(3) calling convention to standard stack-based
> version.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  cpu-all.h              |    9 +++++
>  exec-all.h             |    2 +
>  exec.c                 |    4 ++
>  softmmu_defs.h         |   28 ++++++++++++++++
>  softmmu_header.h       |   60 ++++++++++++++++++++++++++--------
>  softmmu_template.h     |   84 ++++++++++++++++++++++++++++++++---------------
>  tcg/arm/tcg-target.c   |   53 ++++++++++++++++++++++++++++++
>  tcg/hppa/tcg-target.c  |   44 +++++++++++++++++++++++++
>  tcg/i386/tcg-target.c  |   57 ++++++++++++++++++++++++++++++++
>  tcg/ia64/tcg-target.c  |   46 ++++++++++++++++++++++++++
>  tcg/mips/tcg-target.c  |   44 +++++++++++++++++++++++++
>  tcg/ppc/tcg-target.c   |   45 +++++++++++++++++++++++++
>  tcg/ppc64/tcg-target.c |   44 +++++++++++++++++++++++++
>  tcg/s390/tcg-target.c  |   44 +++++++++++++++++++++++++
>  tcg/sparc/tcg-target.c |   50 +++++++++++++++++++++++++++--
>  tcg/tci/tcg-target.c   |    6 +++
>  16 files changed, 576 insertions(+), 44 deletions(-)
> 

This commit completely broke arm and mips host support, not only for 64
bit targets as written in the comments, but even for 32 bit targets as
shifting arguments one by one doesn't work for qemu_st64 which needs 5
values, while only 4 can be passed in registers.

Moreover even on x86_64, this introduces some performance regressions by
emitting 4 additional moves in the slow path and adding some more
constraints on the registers that can be used for passing arguments to
ld/st ops.

While more and more targets needs AREG0 to be passed, I have started to
work on fixing that. I came to the conclusion that passing AREG0 as the
first argument, even if it is look the nice way to do it in C is
probably not the best option:
- On 32 bit hosts, which usually need register alignments for 64-bit
  values (at least on arm and mips), given AREG0 is a 32-bit value this
  makes the register usage very inefficient when the address or the value
  are 64 bits, in addition to making the code to handle quite complex. It
  would be better to place it close to mem_idx which is also 32 bits.
- On at least ppc, ppc64, sparc64 and x86_64, this adds some more 
  constraints to the ld/st ops arguments.
- On x86_64 This also means the address loading of the first argument done
  in the  TLB function can't be reused easily (it's not a problem right now
  due to registers shifting, but this problem appears when trying to clean
  the code).
- Finally on all hosts, this make the AREG0 / nonAREG0 load/store
  different, and thus the load/store code much more complex (this is
  something that should disappear when all targets are using the AREG0
  case).

That's why I would propose to move the env argument to the last
argument. It's better to place it after mem_idx, as it is usually easier
to store a register on the stack than an immediate value. It also means
we don't need any register shifting, the code change for most hosts 
would be only a few lines to either copy a value from one register to 
another, or to store a register on the stack, that is without additional
constraints (there is a call after that so the argument registers are 
already clobbered).

What do you think of that? If that seems the way to go, I can start
writing patches to do the changes and fix most hosts support.

Aurelien

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net