On Fri, Aug 24, 2012 at 3:05 PM, Aurelien Jarno <aurel...@aurel32.net> wrote: > 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 <blauwir...@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.
IIRC this was an earlier version, at least regparm() stuff was separated. > > 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. For 1.2, fixing host support would be most important. It's a good idea to change the order, but I'd postpone it to 1.3. > > Aurelien > > -- > Aurelien Jarno GPG: 1024D/F1BCDB73 > aurel...@aurel32.net http://www.aurel32.net
