Re: [Qemu-devel] [PATCH 1/9] TCG "sync" op

[Aurelien Jarno]

On Fri, Oct 16, 2009 at 02:38:47PM +0200, Ulrich Hecht wrote:
> sync allows concurrent accesses to locations in memory through different TCG
> variables. This comes in handy when you are emulating CPU registers that can
> be used as either 32 or 64 bit, as TCG doesn't know anything about aliases.
> See the s390x target for an example.
> 
> Fixed sync_i64 build failure on 32-bit targets.

It don't really see the point of such a new op, especially the way it is
used in the S390 target.

If a global is "synced" before each load/store, it will be load/stored 
from/to memory each time it is used. This is exactly what tcg_gen_ld/st
do, except it's only one op. The benefit of globals in TCG is to hold 
them as long as possible in host register and avoid costly memory 
load/store. tcg_gen_ld/st would probably be even more efficient, as 
it is one op instead of two, and also because mapping more globals 
means more time spent in the code looping over all globals.

IMHO, the correct way to do it is to use the following code, assuming 
you want to use 64-bit TCG regs to hold 32-bit values (that's something
that is not really clear in your next patch):

- for register load:

| static TCGv load_reg(int reg)
| {
|    TCGv r = tcg_temp_new_i64();
|    tcg_gen_ext32u_i64(r, tcgregs[reg]);
|    return r;
| }
|
| static void store_reg32(int reg, TCGv v)
| {
|    tcg_gen_ext32u_i64(v, v); /* may be optional */
|    tcg_gen_andi_i64(tcgregs[reg], tcgregs[reg], 0xffffffff00000000ULL);
|    tcg_gen_or_i64(tcgregs[reg], tcgregs[reg], v);
| }

If you want to do the same using 32-bit TCG regs:

| static TCGv_i32 load_reg(int reg)
| {
|    TCGv_i32 r = tcg_temp_new_i32();
|    tcg_gen_extu_i32_i64(r, tcgregs[reg]);
|    return r;
| }
|
| static void store_reg32(int reg, TCGv_i32 v)
| {
|    TCGv tmp = tcg_temp_new();
|    tcg_gen_extu_i32_i64(tmp, v);
|    tcg_gen_andi_i64(tcgregs[reg], tcgregs[reg], 0xffffffff00000000ULL);
|    tcg_gen_or_i64(tcgregs[reg], tcgregs[reg], tmp);
|    tcg_temp_free(tmp);
| }

Regards,
Aurelien

> Signed-off-by: Ulrich Hecht <address@hidden>
> ---
>  tcg/tcg-op.h  |   12 ++++++++++++
>  tcg/tcg-opc.h |    2 ++
>  tcg/tcg.c     |    6 ++++++
>  3 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
> index faf2e8b..c1b4710 100644
> --- a/tcg/tcg-op.h
> +++ b/tcg/tcg-op.h
> @@ -316,6 +316,18 @@ static inline void tcg_gen_br(int label)
>      tcg_gen_op1i(INDEX_op_br, label);
>  }
>  
> +static inline void tcg_gen_sync_i32(TCGv_i32 arg)
> +{
> +    tcg_gen_op1_i32(INDEX_op_sync_i32, arg);
> +}
> +
> +#if TCG_TARGET_REG_BITS == 64
> +static inline void tcg_gen_sync_i64(TCGv_i64 arg)
> +{
> +    tcg_gen_op1_i64(INDEX_op_sync_i64, arg);
> +}
> +#endif
> +
>  static inline void tcg_gen_mov_i32(TCGv_i32 ret, TCGv_i32 arg)
>  {
>      if (!TCGV_EQUAL_I32(ret, arg))
> diff --git a/tcg/tcg-opc.h b/tcg/tcg-opc.h
> index b7f3fd7..5dcdeba 100644
> --- a/tcg/tcg-opc.h
> +++ b/tcg/tcg-opc.h
> @@ -40,6 +40,7 @@ DEF2(call, 0, 1, 2, TCG_OPF_SIDE_EFFECTS) /* variable 
> number of parameters */
>  DEF2(jmp, 0, 1, 0, TCG_OPF_BB_END | TCG_OPF_SIDE_EFFECTS)
>  DEF2(br, 0, 0, 1, TCG_OPF_BB_END | TCG_OPF_SIDE_EFFECTS)
>  
> +DEF2(sync_i32, 0, 1, 0, 0)
>  DEF2(mov_i32, 1, 1, 0, 0)
>  DEF2(movi_i32, 1, 0, 1, 0)
>  /* load/store */
> @@ -109,6 +110,7 @@ DEF2(neg_i32, 1, 1, 0, 0)
>  #endif
>  
>  #if TCG_TARGET_REG_BITS == 64
> +DEF2(sync_i64, 0, 1, 0, 0)
>  DEF2(mov_i64, 1, 1, 0, 0)
>  DEF2(movi_i64, 1, 0, 1, 0)
>  /* load/store */
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 3c0e296..8eb60f8 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1930,6 +1930,12 @@ static inline int tcg_gen_code_common(TCGContext *s, 
> uint8_t *gen_code_buf,
>          //        dump_regs(s);
>  #endif
>          switch(opc) {
> +        case INDEX_op_sync_i32:
> +#if TCG_TARGET_REG_BITS == 64
> +        case INDEX_op_sync_i64:
> +#endif
> +            temp_save(s, args[0], s->reserved_regs);
> +            break;
>          case INDEX_op_mov_i32:
>  #if TCG_TARGET_REG_BITS == 64
>          case INDEX_op_mov_i64:
> -- 
> 1.6.2.1
> 
> 
> 
> 

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