[Qemu-devel] TCG: AREG0 removal planning

[Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

Hi,

TCG uses a fixed global register (AREG0) to which points to currently
used CPUState, also known as 'env'. Using a fixed register has the
downsides that the register must be reserved by TCG for generated code
and by the compiler for compiling a few critical files (op_helper.c
etc.). The latter also means that any calls to C library may be unsafe
from those files.

Here are my sketches about transition to AREG0-less world.

TCG the generator backend
-AREG0 is used for qemu_ld/st ops for TLB access. It should be
possible for the translators to pass instead a pointer to either
CPUState or directly to the TLB. New qemu_ld/st ops are needed for all
TCG targets.
-TCG temps are stored in CPUState field temp_buf[], accessed via
AREG0. Maybe a regular stack frame should be allocated instead?
-A generator can support translators with and without AREG0 without
performance impact and minor #ifdeffery.

Translators/op helpers
-Op helpers should not use global 'env' but take a CPUState or more
specific pointer instead. The converted helpers should be moved from
op_helper.c to helper.c. As Paul suggested, a new TCG_CALL_TYPE could
be added.
-cpu_env: still needed. Maybe a new TCG temp type should be added?
Special magic temp set up by prologue?
-New qemu_ld/st ops must be used. This breaks the translator when used
with the TCG backends that aren't yet converted, except with heavy
#ifdeffery.
-When a translator is completely AREG0 free, the register can be freed
for normal allocations.
-Performance will degrade until AREG0 usage is eliminated, then
performance should be better than now.

Other TCG execution loop (cpu-exec.c)
-Convert global 'env' use.
-It may be a bit tricky (some #ifdeffery) to support simultaneously
targets that use AREG0 and those that don't.
-Performance may degrade for the QEMU targets that still use AREG0.

Staging
-Ideally generators should be converted first to avoid breakages.
-In practice, breakages and performance regressions will be hard to avoid.

Cleanups
-HELPER_CFLAGS will be eliminated.
-All of QEMU will be plain ordinary C code for easier portability.
-Buggy C libraries (Sparc glibc longjmp destroying %g registers...)
won't be able to mess with global registers, no restrictions about C
library calls from op helpers are needed.
-dyngen_exec.h can be finally eliminated.
-Since qemu_ld/st uses need fixing anyway, the ops can be refactored
as well, for example taking Aurelien's constant patches into account.
-Generic softfloat and other common ops could be added, called
directly without a helper.

History
-In dyngen times, there used to be three global registers, AREG0 to
AREG2 (even more were defined, but not used).
-AREG1 and AREG2 were known as T1 and T2. Dyngen ops used these directly.

Comments? There are a few blank spots too.

Re: [Qemu-devel] TCG: AREG0 removal planning

[Paul Brook]

> TCG uses a fixed global register (AREG0) to which points to currently
> used CPUState, also known as 'env'. Using a fixed register has the
> downsides that the register must be reserved by TCG for generated code
> and by the compiler for compiling a few critical files (op_helper.c
> etc.). The latter also means that any calls to C library may be unsafe
> from those files.
> 
> Here are my sketches about transition to AREG0-less world.
> ...
> Translators/op helpers
> ...
> Comments? There are a few blank spots too.

I think a useful, and incremental goal is elimination of global cpu_env state 
in C code (i.e eliminate HELPER_CFLAGS and dyngen-exec.h).
We already have much of the infrastructure for this - op_helper v.s. helper.c 
and code_gen_prologue for transition in/out of "generated code" state.

In practice generated code probably accesses CPUState often enough that a 
dedicated register isn't a bad idea.  My guess is that eliminating it from C 
code gets us almost all of the useful benefit.  Removing it from the code 
generator (i.e. TCG_AREG0) may be more pain that it's worth.  For changes to 
the TCG side we want to consider how we can provide useful aliasing 
information, rather than a naive replacement of TCG_AREG0 with a variable.

Paul

Re: [Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

On Wed, May 11, 2011 at 12:28 AM, Paul Brook <paul@codesourcery.com> wrote:
>> TCG uses a fixed global register (AREG0) to which points to currently
>> used CPUState, also known as 'env'. Using a fixed register has the
>> downsides that the register must be reserved by TCG for generated code
>> and by the compiler for compiling a few critical files (op_helper.c
>> etc.). The latter also means that any calls to C library may be unsafe
>> from those files.
>>
>> Here are my sketches about transition to AREG0-less world.
>> ...
>> Translators/op helpers
>> ...
>> Comments? There are a few blank spots too.
>
> I think a useful, and incremental goal is elimination of global cpu_env state
> in C code (i.e eliminate HELPER_CFLAGS and dyngen-exec.h).
> We already have much of the infrastructure for this - op_helper v.s. helper.c
> and code_gen_prologue for transition in/out of "generated code" state.
>
> In practice generated code probably accesses CPUState often enough that a
> dedicated register isn't a bad idea.  My guess is that eliminating it from C
> code gets us almost all of the useful benefit.  Removing it from the code
> generator (i.e. TCG_AREG0) may be more pain that it's worth.

I don't think moving the helpers from op_helper.c to helper.c will be
a performance win if AREG0 is not eliminated. The code gets to use one
register more, but AREG0 needs to be moved to a function argument
register in most cases and AREG0 has to be restored. I think the
benefit should come from generated code getting one more available
register.

TCG side doesn't look so difficult, just qemu_ld/st ops and using a
stack frame. Converting translators and helpers is a much bigger job.

> For changes to
> the TCG side we want to consider how we can provide useful aliasing
> information, rather than a naive replacement of TCG_AREG0 with a variable.

What aliasing information?

Re: [Qemu-devel] TCG: AREG0 removal planning

[Lluís]

Blue Swirl writes:

> On Wed, May 11, 2011 at 12:28 AM, Paul Brook <paul@codesourcery.com> wrote:
>> In practice generated code probably accesses CPUState often enough that a
>> dedicated register isn't a bad idea.  My guess is that eliminating it from C
>> code gets us almost all of the useful benefit.  Removing it from the code
>> generator (i.e. TCG_AREG0) may be more pain that it's worth.

> I don't think moving the helpers from op_helper.c to helper.c will be
> a performance win if AREG0 is not eliminated. The code gets to use one
> register more, but AREG0 needs to be moved to a function argument
> register in most cases and AREG0 has to be restored. I think the
> benefit should come from generated code getting one more available
> register.

So, it all boils down to the amount of register spilling that can be
avoided in TCG-generated code when eliminating the reserved register for
AREG0. Am I right?


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

On Wed, May 11, 2011 at 9:39 PM, Lluís <xscript@gmx.net> wrote:
> Blue Swirl writes:
>
>> On Wed, May 11, 2011 at 12:28 AM, Paul Brook <paul@codesourcery.com> wrote:
>>> In practice generated code probably accesses CPUState often enough that a
>>> dedicated register isn't a bad idea.  My guess is that eliminating it from C
>>> code gets us almost all of the useful benefit.  Removing it from the code
>>> generator (i.e. TCG_AREG0) may be more pain that it's worth.
>
>> I don't think moving the helpers from op_helper.c to helper.c will be
>> a performance win if AREG0 is not eliminated. The code gets to use one
>> register more, but AREG0 needs to be moved to a function argument
>> register in most cases and AREG0 has to be restored. I think the
>> benefit should come from generated code getting one more available
>> register.
>
> So, it all boils down to the amount of register spilling that can be
> avoided in TCG-generated code when eliminating the reserved register for
> AREG0. Am I right?

For the generated code, yes.

Re: [Qemu-devel] TCG: AREG0 removal planning

[Avi Kivity]

On 05/11/2011 08:25 PM, Blue Swirl wrote:
> >
> >  I think a useful, and incremental goal is elimination of global cpu_env state
> >  in C code (i.e eliminate HELPER_CFLAGS and dyngen-exec.h).
> >  We already have much of the infrastructure for this - op_helper v.s. helper.c
> >  and code_gen_prologue for transition in/out of "generated code" state.
> >
> >  In practice generated code probably accesses CPUState often enough that a
> >  dedicated register isn't a bad idea.  My guess is that eliminating it from C
> >  code gets us almost all of the useful benefit.  Removing it from the code
> >  generator (i.e. TCG_AREG0) may be more pain that it's worth.
>
> I don't think moving the helpers from op_helper.c to helper.c will be
> a performance win if AREG0 is not eliminated. The code gets to use one
> register more, but AREG0 needs to be moved to a function argument
> register in most cases and AREG0 has to be restored. I think the
> benefit should come from generated code getting one more available
> register.

If you use a callee-saved register then you don't need to restore it.  
Looks like that's already the case, at least on x86.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] TCG: AREG0 removal planning

[Paul Brook]

> > For changes to
> > the TCG side we want to consider how we can provide useful aliasing
> > information, rather than a naive replacement of TCG_AREG0 with a
> > variable.
> 
> What aliasing information?

Aliasing of cpu state accesses between tcg_global_mem_new_* variables, 
qemu_ld/st ops, and helper functions.

Paul

Re: [Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

On Mon, May 16, 2011 at 7:16 PM, Paul Brook <paul@codesourcery.com> wrote:
>> > For changes to
>> > the TCG side we want to consider how we can provide useful aliasing
>> > information, rather than a naive replacement of TCG_AREG0 with a
>> > variable.
>>
>> What aliasing information?
>
> Aliasing of cpu state accesses between tcg_global_mem_new_* variables,
> qemu_ld/st ops, and helper functions.

Structures describing such aliases must be somewhat complex. Those
descriptors should then be attached to these variables, ops and
functions. Checking the structures during translation may be simpler
but I'd expect some kind of list or bit map search to happen for each
access to these variables etc.

Maybe you have a better design in mind, but I'm not sure this way
would bring performance. The translator can't be too complex.

Re: [Qemu-devel] TCG: AREG0 removal planning

[Stefan Weil]

Am 10.05.2011 22:54, schrieb Blue Swirl:
> Hi,
>
> TCG uses a fixed global register (AREG0) to which points to currently
> used CPUState, also known as 'env'. Using a fixed register has the
> downsides that the register must be reserved by TCG for generated code
> and by the compiler for compiling a few critical files (op_helper.c
> etc.). The latter also means that any calls to C library may be unsafe
> from those files.
>
> Here are my sketches about transition to AREG0-less world.
>
> TCG the generator backend
> -AREG0 is used for qemu_ld/st ops for TLB access. It should be
> possible for the translators to pass instead a pointer to either
> CPUState or directly to the TLB. New qemu_ld/st ops are needed for all
> TCG targets.
> -TCG temps are stored in CPUState field temp_buf[], accessed via
> AREG0. Maybe a regular stack frame should be allocated instead?
> -A generator can support translators with and without AREG0 without
> performance impact and minor #ifdeffery.
>
> Translators/op helpers
> -Op helpers should not use global 'env' but take a CPUState or more
> specific pointer instead. The converted helpers should be moved from
> op_helper.c to helper.c. As Paul suggested, a new TCG_CALL_TYPE could
> be added.
> -cpu_env: still needed. Maybe a new TCG temp type should be added?
> Special magic temp set up by prologue?
> -New qemu_ld/st ops must be used. This breaks the translator when used
> with the TCG backends that aren't yet converted, except with heavy
> #ifdeffery.
> -When a translator is completely AREG0 free, the register can be freed
> for normal allocations.
> -Performance will degrade until AREG0 usage is eliminated, then
> performance should be better than now.
>
> Other TCG execution loop (cpu-exec.c)
> -Convert global 'env' use.
> -It may be a bit tricky (some #ifdeffery) to support simultaneously
> targets that use AREG0 and those that don't.
> -Performance may degrade for the QEMU targets that still use AREG0.
>
> Staging
> -Ideally generators should be converted first to avoid breakages.
> -In practice, breakages and performance regressions will be hard to avoid.
>
> Cleanups
> -HELPER_CFLAGS will be eliminated.
> -All of QEMU will be plain ordinary C code for easier portability.
> -Buggy C libraries (Sparc glibc longjmp destroying %g registers...)
> won't be able to mess with global registers, no restrictions about C
> library calls from op helpers are needed.
> -dyngen_exec.h can be finally eliminated.
> -Since qemu_ld/st uses need fixing anyway, the ops can be refactored
> as well, for example taking Aurelien's constant patches into account.
> -Generic softfloat and other common ops could be added, called
> directly without a helper.
>
> History
> -In dyngen times, there used to be three global registers, AREG0 to
> AREG2 (even more were defined, but not used).
> -AREG1 and AREG2 were known as T1 and T2. Dyngen ops used these directly.
>
> Comments? There are a few blank spots too.

Just for information:

TCI (the TCG interpreter) uses a global variable (no register,
because it is designed to run on any architecture).

Code extracts:

dyngen-exec.h:
#if defined(AREG0)
# define DECLARE_QEMU_ENV(s) register struct s *env asm(AREG0)
#else
# define DECLARE_QEMU_ENV(s) extern struct s *env
#endif

target-i386/exec.h (similar for other targets):
DECLARE_QEMU_ENV(CPUX86State);

Your plan might allow co-existence of the normal TCG and TCI in
the same binary, so users could select TCI via a command line
switch (there are a few use cases where TCI has advantages).

It is also a small step towards a multicore emulation which makes
full use of real multicore cpus.

I have some regression tests here (mainly debian linux guests
with different architectures, but also an unspeakable host os
which some people don't like) and will certainly run these tests
when required.

And yes, you will discover more blank spots when you do the work.

Regards,
Stefan

Re: [Qemu-devel] TCG: AREG0 removal planning

[Richard Henderson]

On 05/10/2011 01:54 PM, Blue Swirl wrote:
> TCG the generator backend
> -AREG0 is used for qemu_ld/st ops for TLB access. It should be
> possible for the translators to pass instead a pointer to either
> CPUState or directly to the TLB.

I believe that AREG0 should continue to be present in the generated
code.  There are simply too many references to it throughout the
translated code for allocating this dynamically to be a win.

What should change, however, is the removal of AREG0 outside the
generated code.  The cpu-state pointer should be passed as a regular
parameter wherever it is required.  This includes tcg_qemu_tb_exec,
which means that the generated prologue would change, setting up
AREG0 in the process.

> New qemu_ld/st ops are needed for all TCG targets.

Yes, qemu_ld/st would have to change to accommodate the new parameter
being passed.

While we're at it, let us change things a bit further to allow guest
byte-swap load/store insns to be implemented more efficiently.  For
instance, currently a sparc load_asr (little-endian), as emulated on
an x86 host, does the byte swap twice.

There is, currently, a const int parameter to qemu_ld/st that encodes
the size of the load.  Almost all TCG backends behind the scenes 
extend this parameter with a bit to indicate byte swap needed.  Let us
formalize this, and allow this to be set in the original TCG op, with
appropriate new inlines in tcg-op.h to access it from the translators.

We can also make things easier for the backends by allowing them
to declare that they do or do not have byte swap load/store insns.
If the such are not available, a separate bswap opcode is emitted
right from tcg_gen_qemu_st32 et al.

This would allow a nice cleanup for i386, which currently has a small
register allocation problem in the store path, what with needing to
not clobber the input register while byte swapping.  (This problem is
solved by restricting the set of input registers for qemu_ld/st.)

All this does require the slow path to be changed to accommodate this.
In particular, if byte-swap memory ops are available, we need slow
path functions that also byte swap.  Indeed, I'd expect them to use
the byte-swap memory ops themselves.  Further, if byte-swap memory
ops are not available, the slow path should always return memory in
the host byte order, because a separate bswap operation will be done
on behalf of the fast path.

> -TCG temps are stored in CPUState field temp_buf[], accessed via
> AREG0. Maybe a regular stack frame should be allocated instead?

Probably.  Most of the backends manage a stack frame anyway, to
handle registers saved in the prologue.  All that would be needed
is a define from TCG to tell the backends how much memory is required,
and some value passed from the backends to tell TCG what the offset
of that area is from the stack pointer.


r~

Re: [Qemu-devel] TCG: AREG0 removal planning

[Paul Brook]

> While we're at it, let us change things a bit further to allow guest
> byte-swap load/store insns to be implemented more efficiently.  For
> instance, currently a sparc load_asr (little-endian), as emulated on
> an x86 host, does the byte swap twice.

FWIW this also ends up interacting with the device and bus models. This is 
partially implemented by the endian parameter of cpu_register_io_memory et. 
al.  This may also be a runtime property, either part of the CPU state (e.g. 
ARM where instruction and data accesses may have different endianness), or 
even a per-page TLB attribute (PPC?).

Paul

Re: [Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

On Wed, May 11, 2011 at 1:57 AM, Paul Brook <paul@codesourcery.com> wrote:
>> While we're at it, let us change things a bit further to allow guest
>> byte-swap load/store insns to be implemented more efficiently.  For
>> instance, currently a sparc load_asr (little-endian), as emulated on
>> an x86 host, does the byte swap twice.
>
> FWIW this also ends up interacting with the device and bus models. This is
> partially implemented by the endian parameter of cpu_register_io_memory et.
> al.  This may also be a runtime property, either part of the CPU state (e.g.
> ARM where instruction and data accesses may have different endianness), or
> even a per-page TLB attribute (PPC?).

SuperSparc (Sparc32) MMU had a bit for reversing the endianness of a
page, on Sparc64 there are many levels where this can be done (global
CPU mode flag, MMU page flag and some memory accesses can use byte
swapping ASIs). I don't think they are used though.

Re: [Qemu-devel] TCG: AREG0 removal planning

[Blue Swirl]

On Wed, May 11, 2011 at 12:58 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 05/10/2011 01:54 PM, Blue Swirl wrote:
>> TCG the generator backend
>> -AREG0 is used for qemu_ld/st ops for TLB access. It should be
>> possible for the translators to pass instead a pointer to either
>> CPUState or directly to the TLB.
>
> I believe that AREG0 should continue to be present in the generated
> code.  There are simply too many references to it throughout the
> translated code for allocating this dynamically to be a win.
>
> What should change, however, is the removal of AREG0 outside the
> generated code.  The cpu-state pointer should be passed as a regular
> parameter wherever it is required.  This includes tcg_qemu_tb_exec,
> which means that the generated prologue would change, setting up
> AREG0 in the process.

I have exactly opposite feeling, AREG0 needs to be eliminated from
generated code but converting helpers won't be useful. Maybe some
experiments are needed to see what would be the real gains.

>> New qemu_ld/st ops are needed for all TCG targets.
>
> Yes, qemu_ld/st would have to change to accommodate the new parameter
> being passed.
>
> While we're at it, let us change things a bit further to allow guest
> byte-swap load/store insns to be implemented more efficiently.  For
> instance, currently a sparc load_asr (little-endian), as emulated on
> an x86 host, does the byte swap twice.
>
> There is, currently, a const int parameter to qemu_ld/st that encodes
> the size of the load.  Almost all TCG backends behind the scenes
> extend this parameter with a bit to indicate byte swap needed.  Let us
> formalize this, and allow this to be set in the original TCG op, with
> appropriate new inlines in tcg-op.h to access it from the translators.
>
> We can also make things easier for the backends by allowing them
> to declare that they do or do not have byte swap load/store insns.
> If the such are not available, a separate bswap opcode is emitted
> right from tcg_gen_qemu_st32 et al.
>
> This would allow a nice cleanup for i386, which currently has a small
> register allocation problem in the store path, what with needing to
> not clobber the input register while byte swapping.  (This problem is
> solved by restricting the set of input registers for qemu_ld/st.)
>
> All this does require the slow path to be changed to accommodate this.
> In particular, if byte-swap memory ops are available, we need slow
> path functions that also byte swap.  Indeed, I'd expect them to use
> the byte-swap memory ops themselves.  Further, if byte-swap memory
> ops are not available, the slow path should always return memory in
> the host byte order, because a separate bswap operation will be done
> on behalf of the fast path.

I agree.

>> -TCG temps are stored in CPUState field temp_buf[], accessed via
>> AREG0. Maybe a regular stack frame should be allocated instead?
>
> Probably.  Most of the backends manage a stack frame anyway, to
> handle registers saved in the prologue.  All that would be needed
> is a define from TCG to tell the backends how much memory is required,
> and some value passed from the backends to tell TCG what the offset
> of that area is from the stack pointer.

Currently the size of temp_buf is fixed, but the exact size for the
stack frame could be calculated during translation. Not that there
would be measurable performance improvements though.