[09/15] tcg-sparc: Do not use a global register for AREG0.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 dyngen-exec.h |   20 +++++++++++---------
 exec.c        |   16 ++++++++++++++--
 2 files changed, 25 insertions(+), 11 deletions(-)

On Sun, Mar 25, 2012 at 22:27, Richard Henderson <rth@twiddle.net> wrote:
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  dyngen-exec.h |   20 +++++++++++---------
>  exec.c        |   16 ++++++++++++++--
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/dyngen-exec.h b/dyngen-exec.h
> index 65fcb43..d673f9f 100644
> --- a/dyngen-exec.h
> +++ b/dyngen-exec.h
> @@ -41,13 +41,8 @@
>  #elif defined(__mips__)
>  #define AREG0 "s0"
>  #elif defined(__sparc__)
> -#ifdef CONFIG_SOLARIS
> -#define AREG0 "g2"
> -#elif HOST_LONG_BITS == 64
> -#define AREG0 "g5"
> -#else
> -#define AREG0 "g6"
> -#endif
> +/* Don't use a global register.  Working around glibc clobbering these
> +   global registers is more trouble than just using TLS.  */
>  #elif defined(__s390__)
>  #define AREG0 "r10"
>  #elif defined(__alpha__)
> @@ -62,12 +57,19 @@
>  #error unsupported CPU
>  #endif
>
> -#if defined(AREG0)
> +#ifdef AREG0
>  register CPUArchState *env asm(AREG0);
>  #else
> -/* TODO: Try env = cpu_single_env. */
> +/* It's tempting to #define env cpu_single_cpu, but that runs afoul of
> +   the other macro usage in target-foo/helper.h.  Instead use an alias.
> +   That has to happen where cpu_single_cpu is defined, so just a
> +   declaration here.  */
> +#ifdef __linux__
> +extern __thread CPUArchState *env;
> +#else
>  extern CPUArchState *env;
>  #endif
> +#endif /* AREG0 */
>
>  #endif /* !CONFIG_TCG_PASS_AREG0 */
>  #endif /* !defined(__DYNGEN_EXEC_H__) */
> diff --git a/exec.c b/exec.c
> index 6731ab8..d84caa5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -124,9 +124,21 @@ static MemoryRegion io_mem_subpage_ram;
>  #endif
>
>  CPUArchState *first_cpu;
> -/* current CPU in the current thread. It is only valid inside
> -   cpu_exec() */
> +
> +/* Current CPU in the current thread. It is only valid inside cpu_exec().  */
>  DEFINE_TLS(CPUArchState *,cpu_single_env);
> +
> +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env.
> +   We can't actually tell from here whether that's needed or not, but it does
> +   not hurt to go ahead and make the declaration.  */
> +#ifndef CONFIG_TCG_PASS_AREG0
> +extern
> +#ifdef __linux__
> +  __thread
> +#endif
> +  CPUArchState *env __attribute__((alias("tls__cpu_single_env")));
> +#endif /* CONFIG_TCG_PASS_AREG0 */

Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also
use tls_var().

> +
>  /* 0 = Do not count executed instructions.
>    1 = Precise instruction counting.
>    2 = Adaptive rate instruction counting.  */
> --
> 1.7.7.6
>

On 03/26/12 09:31, Blue Swirl wrote:
>> > +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env.
>> > +   We can't actually tell from here whether that's needed or not, but it does
>> > +   not hurt to go ahead and make the declaration.  */
>> > +#ifndef CONFIG_TCG_PASS_AREG0
>> > +extern
>> > +#ifdef __linux__
>> > +  __thread
>> > +#endif
>> > +  CPUArchState *env __attribute__((alias("tls__cpu_single_env")));
>> > +#endif /* CONFIG_TCG_PASS_AREG0 */
> Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also
> use tls_var().
> 

That won't work.

This is intended to be a drop-in replacement for the "env" symbol that
we declare in dyngen-exec.h.  For all other hosts, this symbol is a
global register variable.  We can't go wrapping tls_var around all uses
in all target backends.

As I say in the comment, the most natural replacement is a preprocessor
macro, but then that fails with the uses of "env" in the DEF_HELPER_N
macros.

Which leaves no alternative -- short of converting *all* targets to
CONFIG_TCG_PASS_AREG0 first -- except the symbol alias you see there.

Hmm... actually... I'm wrong about the use of preprocessor macros.
The simple solution there is to re-order the includes on a few ports.
I.e. "helper.h" must come before "dyngen-exec.h".  Now that's a much
simpler fix...


r~

On Mon, Mar 26, 2012 at 16:52, Richard Henderson <rth@twiddle.net> wrote:
> On 03/26/12 09:31, Blue Swirl wrote:
>>> > +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env.
>>> > +   We can't actually tell from here whether that's needed or not, but it does
>>> > +   not hurt to go ahead and make the declaration.  */
>>> > +#ifndef CONFIG_TCG_PASS_AREG0
>>> > +extern
>>> > +#ifdef __linux__
>>> > +  __thread
>>> > +#endif
>>> > +  CPUArchState *env __attribute__((alias("tls__cpu_single_env")));
>>> > +#endif /* CONFIG_TCG_PASS_AREG0 */
>> Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also
>> use tls_var().
>>
>
> That won't work.
>
> This is intended to be a drop-in replacement for the "env" symbol that
> we declare in dyngen-exec.h.  For all other hosts, this symbol is a
> global register variable.  We can't go wrapping tls_var around all uses
> in all target backends.
>
> As I say in the comment, the most natural replacement is a preprocessor
> macro, but then that fails with the uses of "env" in the DEF_HELPER_N
> macros.
>
> Which leaves no alternative -- short of converting *all* targets to
> CONFIG_TCG_PASS_AREG0 first -- except the symbol alias you see there.

But at that point there will be no global env use anymore, so
dyngen-exec.h etc. can be removed. Perhaps this patch and its
dependencies should wait for that to happen. As an intermediate hack
it's sort of OK.

> Hmm... actually... I'm wrong about the use of preprocessor macros.
> The simple solution there is to re-order the includes on a few ports.
> I.e. "helper.h" must come before "dyngen-exec.h".  Now that's a much
> simpler fix...

OK.

>
>
> r~