Re: [Qemu-devel] [PATCH] convert of few alpha insn to TCG

[Aurelien Jarno <aurelien@aurel32.net>]

> On Mon, Sep 01, 2008 at 01:38:15PM +0200, Tristan Gingold wrote:
> > Hi,
> >
> > this patch switches a very few instructions to TCG.
> >
> > Tristan.

Please find my comments below.

> Index: target-alpha/op.c
> ===================================================================
> --- target-alpha/op.c	(revision 5119)
> +++ target-alpha/op.c	(working copy)
> @@ -131,12 +131,6 @@
>      RETURN();
>  }
>  
> -void OPPROTO op_tb_flush (void)
> -{
> -    helper_tb_flush();
> -    RETURN();
> -}
> -
>  /* Load and stores */
>  #define MEMSUFFIX _raw
>  #include "op_mem.h"
> @@ -685,27 +679,6 @@
>  }
>  #endif
>  
> -#if 0 // Qemu does not know how to do this...
> -void OPPROTO op_update_pc (void)
> -{
> -    env->pc = PARAM(1);
> -    RETURN();
> -}
> -#else
> -void OPPROTO op_update_pc (void)
> -{
> -    env->pc = ((uint64_t)PARAM(1) << 32) | (uint64_t)PARAM(2);
> -    RETURN();
> -}
> -#endif
> -
> -/* Optimization for 32 bits hosts architectures */
> -void OPPROTO op_update_pc32 (void)
> -{
> -    env->pc = (uint64_t)PARAM(1);
> -    RETURN();
> -}
> -
>  /* IEEE floating point arithmetic */
>  /* S floating (single) */
>  void OPPROTO op_adds (void)
> Index: target-alpha/helper.h
> ===================================================================
> --- target-alpha/helper.h	(revision 0)
> +++ target-alpha/helper.h	(revision 0)
> @@ -0,0 +1,5 @@
> +#ifndef DEF_HELPER
> +#define DEF_HELPER(ret, name, params) ret name params;
> +#endif
> +
> +DEF_HELPER(void, do_tb_flush, (void))
> Index: target-alpha/op_helper.c
> ===================================================================
> --- target-alpha/op_helper.c	(revision 5119)
> +++ target-alpha/op_helper.c	(working copy)
> @@ -45,7 +45,7 @@
>  #include "op_helper_mem.h"
>  #endif
>  
> -void helper_tb_flush (void)
> +void do_tb_flush (void)

No need to change the name of the helper...

>  {
>      tlb_flush(env, 1);
>  }
> Index: target-alpha/translate.c
> ===================================================================
> --- target-alpha/translate.c	(revision 5119)
> +++ target-alpha/translate.c	(working copy)
> @@ -25,6 +25,7 @@
>  #include "cpu.h"
>  #include "exec-all.h"
>  #include "disas.h"
> +#include "helper.h"
>  #include "tcg-op.h"
>  #include "qemu-common.h"
>  
> @@ -126,6 +127,22 @@
>      }
>  }
>  
> +static inline void tcg_gen_load_ir (TCGv t, int reg)
> +{
> +    if (reg == 31)
> +        tcg_gen_movi_i64(t, 0);
> +    else
> +        tcg_gen_ld_i64(t, cpu_env, offsetof(CPUState, ir) +
> +		       sizeof(target_ulong) * reg);
> +}
> +
> +static inline void tcg_gen_store_ir (TCGv t, int reg)
> +{
> +    if (reg != 31)
> +        tcg_gen_st_i64(t, cpu_env, offsetof(CPUState, ir) +
> +                                  sizeof(target_ulong) * reg);
> +}
> +

As said in the other mail, I have concerns about that part. TCG works
better if most (all ?) registers are mapped.

>  /* FIR moves */
>  /* Special hacks for fir31 */
>  #define gen_op_load_FT0_fir31 gen_op_reset_FT0
> @@ -356,15 +373,9 @@
>  
>  static always_inline void gen_update_pc (DisasContext *ctx)
>  {
> -    if (!(ctx->pc >> 32)) {
> -        gen_op_update_pc32(ctx->pc);
> -    } else {
> -#if 0 // Qemu does not know how to do this...
> -        gen_op_update_pc(ctx->pc);
> -#else
> -        gen_op_update_pc(ctx->pc >> 32, ctx->pc);
> -#endif
> -    }
> +    TCGv v = tcg_const_i64(ctx->pc);
> +    tcg_gen_st_i64(v, cpu_env, offsetof(CPUState, pc));
> +    tcg_temp_free(v);
>  }

pc is an often used register, it should be mapped in TCG.

>  static always_inline void _gen_op_bcond (DisasContext *ctx)
> @@ -700,17 +711,31 @@
>          goto invalid_opc;
>      case 0x08:
>          /* LDA */
> -        gen_load_ir(ctx, rb, 0);
> -        gen_set_sT1(ctx, disp16);
> -        gen_op_addq();
> -        gen_store_ir(ctx, ra, 0);
> +        {
> +	    TCGv r = tcg_temp_new(TCG_TYPE_I64);
> +	    TCGv v = tcg_const_i64(disp16);
> +	    if (rb != 31) {
> +		tcg_gen_load_ir(r, rb);
> +		tcg_gen_add_i64(v, r, v);
> +	    }
> +	    tcg_gen_store_ir(v, ra);
> +	    tcg_temp_free(r);
> +	    tcg_temp_free(v);
> +	}
>          break;
>      case 0x09:
>          /* LDAH */
> -        gen_load_ir(ctx, rb, 0);
> -        gen_set_sT1(ctx, disp16 << 16);
> -        gen_op_addq();
> -        gen_store_ir(ctx, ra, 0);
> +        {
> +	    TCGv r = tcg_temp_new(TCG_TYPE_I64);
> +	    TCGv v = tcg_const_i64(disp16 << 16);
> +	    if (rb != 31) {
> +		tcg_gen_load_ir(r, rb);
> +		tcg_gen_add_i64(v, r, v);
> +	    }
> +	    tcg_gen_store_ir(v, ra);
> +	    tcg_temp_free(r);
> +	    tcg_temp_free(v);
> +	}
>          break;
>      case 0x0A:
>          /* LDBU */
> @@ -2059,7 +2084,7 @@
>          gen_update_pc(&ctx);
>      }
>  #if defined (DO_TB_FLUSH)
> -    gen_op_tb_flush();
> +    tcg_gen_helper_0_0(do_tb_flush);
>  #endif
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> 
-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net