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

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

[Tristan Gingold]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]

Hi,

this patch switches a very few instructions to TCG.

Tristan.

[-- Attachment #2: q-alpha1.diff --]
[-- Type: application/octet-stream, Size: 4138 bytes --]

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)
 {
     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);
+}
+
 /* 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);
 }
 
 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();

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

[Thiemo Seufer]

Tristan Gingold wrote:
> Hi,
>
> this patch switches a very few instructions to TCG.

Do you have a way to test alpha changes?


Thiemo

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

[Tristan Gingold]

On Sep 1, 2008, at 4:32 PM, Thiemo Seufer wrote:

> Tristan Gingold wrote:
>> Hi,
>>
>> this patch switches a very few instructions to TCG.
>
> Do you have a way to test alpha changes?

Yes, these three instructions were testing using a small linux-alpha  
program.

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

[Tristan Gingold]

Hi,

any comment on this patch ?  If not, could it be committed please (I  
have more pending patches to convert
alpha to tcg).

Thank you in advance,
Tristan.

On Sep 1, 2008, at 1:38 PM, Tristan Gingold wrote:

> Hi,
>
> this patch switches a very few instructions to TCG.
>
> Tristan.<q-alpha1.diff>

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

[Aurelien Jarno]

On Wed, Sep 03, 2008 at 10:07:55AM +0200, Tristan Gingold wrote:
> Hi,
>
> any comment on this patch ?  If not, could it be committed please (I  
> have more pending patches to convert
> alpha to tcg).

My main comment about this patch is the use of a function to load/store
gpr registers. It should map all gprs registers to TCG instead as
explained by Paul Brook last week.

However, due to r31, I don't really know how to do.

> Thank you in advance,
> Tristan.
>
> On Sep 1, 2008, at 1:38 PM, Tristan Gingold wrote:
>
>> Hi,
>>
>> this patch switches a very few instructions to TCG.
>>
>> Tristan.<q-alpha1.diff>
>
>
>
>

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

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

[Aurelien Jarno]

On Mon, Sep 01, 2008 at 05:54:44PM +0200, Tristan Gingold wrote:
>
> On Sep 1, 2008, at 4:32 PM, Thiemo Seufer wrote:
>
>> Tristan Gingold wrote:
>>> Hi,
>>>
>>> this patch switches a very few instructions to TCG.
>>
>> Do you have a way to test alpha changes?
>
> Yes, these three instructions were testing using a small linux-alpha  
> program.

What kind of programs are you using? I am unable to get the glibc
correctly working, it segfaults as soon as a shared library is used.
Therefore I am only able to execute ld.so, probably not enough to test
regressions.

Without a proper way to test the changes, I am afraid I can't commit
them.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

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

[Tristan Gingold]

On Sep 3, 2008, at 10:27 AM, Aurelien Jarno wrote:

> On Wed, Sep 03, 2008 at 10:07:55AM +0200, Tristan Gingold wrote:
>> Hi,
>>
>> any comment on this patch ?  If not, could it be committed please (I
>> have more pending patches to convert
>> alpha to tcg).
>
> My main comment about this patch is the use of a function to load/ 
> store
> gpr registers. It should map all gprs registers to TCG instead as
> explained by Paul Brook last week.

Ok.

> However, due to r31, I don't really know how to do.

r31 can be a special case.

Thanks for the comment,
Tristan.

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

[Aurelien Jarno]

> 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

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

[Tristan Gingold]

On Sep 3, 2008, at 10:32 AM, Aurelien Jarno wrote:
> What kind of programs are you using? I am unable to get the glibc
> correctly working, it segfaults as soon as a shared library is used.
> Therefore I am only able to execute ld.so, probably not enough to test
> regressions.
>
> Without a proper way to test the changes, I am afraid I can't commit
> them.

I am using a very small program with its own crt1 and fully static.
Do you want me to add a test/ subdirectory ?

Tristan.

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

[Thiemo Seufer]

Tristan Gingold wrote:
>
> On Sep 3, 2008, at 10:32 AM, Aurelien Jarno wrote:
>> What kind of programs are you using? I am unable to get the glibc
>> correctly working, it segfaults as soon as a shared library is used.
>> Therefore I am only able to execute ld.so, probably not enough to test
>> regressions.
>>
>> Without a proper way to test the changes, I am afraid I can't commit
>> them.
>
> I am using a very small program with its own crt1 and fully static.
> Do you want me to add a test/ subdirectory ?

Yes please.


Thiemo

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

[Tristan Gingold]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

Hi,

all the issues have been fixed in this second version.

Tristan.

Signed-off-by: Tristan Gingold <gingold@adacore.com>


[-- Attachment #2: q-alpha1.diff --]
[-- Type: application/octet-stream, Size: 4697 bytes --]

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, helper_tb_flush, (void))
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"
 
@@ -44,15 +45,34 @@
 };
 
 static TCGv cpu_env;
+static TCGv cpu_ir[31];
+static TCGv cpu_pc;
 
 #include "gen-icount.h"
 
 static void alpha_translate_init(void)
 {
     static int done_init = 0;
+    static char cpu_reg_names[4*31];
+    char *p;
+    int i;
+
     if (done_init)
-        return;
+	return;
+
     cpu_env = tcg_global_reg_new(TCG_TYPE_PTR, TCG_AREG0, "env");
+
+    p = cpu_reg_names;
+    for (i = 0; i < 31; i++) {
+        sprintf(p, "r%d", i);
+        cpu_ir[i] = tcg_global_mem_new(TCG_TYPE_I64, TCG_AREG0,
+				       offsetof(CPUState, ir[i]), p);
+	p += 4;
+    }
+
+    cpu_pc = tcg_global_mem_new(TCG_TYPE_I64, TCG_AREG0,
+				offsetof(CPUState, pc), "pc");
+
     done_init = 1;
 }
 
@@ -126,6 +146,20 @@
     }
 }
 
+static inline void get_ir (TCGv t, int reg)
+{
+    if (reg == 31)
+        tcg_gen_movi_i64(t, 0);
+    else
+        tcg_gen_mov_i64(t, cpu_ir[reg]);
+}
+
+static inline void set_ir (TCGv t, int reg)
+{
+    if (reg != 31)
+        tcg_gen_mov_i64(cpu_ir[reg], t);
+}
+
 /* FIR moves */
 /* Special hacks for fir31 */
 #define gen_op_load_FT0_fir31 gen_op_reset_FT0
@@ -356,15 +390,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_mov_i64(cpu_pc, v);
+    tcg_temp_free(v);
 }
 
 static always_inline void _gen_op_bcond (DisasContext *ctx)
@@ -700,17 +728,23 @@
         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 v = tcg_const_i64(disp16);
+	    if (rb != 31)
+		tcg_gen_add_i64(v, cpu_ir[rb], v);
+	    set_ir(v, ra);
+	    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 v = tcg_const_i64(disp16 << 16);
+	    if (rb != 31)
+		tcg_gen_add_i64(v, cpu_ir[rb], v);
+	    set_ir(v, ra);
+	    tcg_temp_free(v);
+	}
         break;
     case 0x0A:
         /* LDBU */
@@ -1871,13 +1905,16 @@
         break;
     case 0x30:
         /* BR */
-        gen_set_uT0(ctx, ctx->pc);
-        gen_store_ir(ctx, ra, 0);
-        if (disp21 != 0) {
-            gen_set_sT1(ctx, disp21 << 2);
-            gen_op_addq();
+	if (ra != 31) {
+	    TCGv t = tcg_const_i64(ctx->pc);
+	    set_ir(t, ra);
+	    tcg_temp_free(t);
+	}
+	{
+	    TCGv pc = tcg_const_i64(ctx->pc + (disp21 << 2));
+	    tcg_gen_mov_i64(cpu_pc, pc);
+	    tcg_temp_free(pc);
         }
-        gen_op_branch();
         ret = 1;
         break;
     case 0x31:
@@ -2059,7 +2096,7 @@
         gen_update_pc(&ctx);
     }
 #if defined (DO_TB_FLUSH)
-    gen_op_tb_flush();
+    tcg_gen_helper_0_0(helper_tb_flush);
 #endif
     if (tb->cflags & CF_LAST_IO)
         gen_io_end();

[-- Attachment #3: Type: text/plain, Size: 5366 bytes --]



On Sep 3, 2008, at 10:46 AM, Aurelien Jarno wrote:

>> 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
>
>

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

[Aurelien Jarno]

On Wed, Sep 03, 2008 at 02:44:48PM +0200, Tristan Gingold wrote:
> Hi,
>
> all the issues have been fixed in this second version.
>
> Tristan.
>
> Signed-off-by: Tristan Gingold <gingold@adacore.com>
>

I have applied a modified version of this patch, as I don't want to
spend to much time doing mail ping pong. Please see the details of the
change below.

Also please watch the indentation, target-alpha/ is indented with space,
I am not sure we want to mix that.

> 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, helper_tb_flush, (void))
> 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"
>  
> @@ -44,15 +45,34 @@
>  };
>  
>  static TCGv cpu_env;
> +static TCGv cpu_ir[31];
> +static TCGv cpu_pc;
>  
>  #include "gen-icount.h"
>  
>  static void alpha_translate_init(void)
>  {
>      static int done_init = 0;
> +    static char cpu_reg_names[4*31];
> +    char *p;
> +    int i;
> +
>      if (done_init)
> -        return;
> +	return;
> +
>      cpu_env = tcg_global_reg_new(TCG_TYPE_PTR, TCG_AREG0, "env");
> +
> +    p = cpu_reg_names;
> +    for (i = 0; i < 31; i++) {
> +        sprintf(p, "r%d", i);

ir would be better to match the name of the variable.

> +        cpu_ir[i] = tcg_global_mem_new(TCG_TYPE_I64, TCG_AREG0,
> +				       offsetof(CPUState, ir[i]), p);
> +	p += 4;
> +    }
> +
> +    cpu_pc = tcg_global_mem_new(TCG_TYPE_I64, TCG_AREG0,
> +				offsetof(CPUState, pc), "pc");
> +
>      done_init = 1;
>  }

You should also register the helpers here.

> @@ -126,6 +146,20 @@
>      }
>  }
>  
> +static inline void get_ir (TCGv t, int reg)
> +{
> +    if (reg == 31)
> +        tcg_gen_movi_i64(t, 0);
> +    else
> +        tcg_gen_mov_i64(t, cpu_ir[reg]);
> +}
> +
> +static inline void set_ir (TCGv t, int reg)
> +{
> +    if (reg != 31)
> +        tcg_gen_mov_i64(cpu_ir[reg], t);
> +}
> +

Even if the register are now mapped directly in TCG, this way of doing
will not really help to find some optimisations later: the ir registers
can't directly be a target register of a TCG instruction, the only way
to read or write those register is through a move.

I'll fix that in another patch.

>  /* FIR moves */
>  /* Special hacks for fir31 */
>  #define gen_op_load_FT0_fir31 gen_op_reset_FT0
> @@ -356,15 +390,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_mov_i64(cpu_pc, v);
> +    tcg_temp_free(v);
>  }

There is no need to use a TCG const, you can use tcg_gen_movi_i64
instead.

>  
>  static always_inline void _gen_op_bcond (DisasContext *ctx)
> @@ -700,17 +728,23 @@
>          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 v = tcg_const_i64(disp16);
> +	    if (rb != 31)
> +		tcg_gen_add_i64(v, cpu_ir[rb], v);
> +	    set_ir(v, ra);
> +	    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 v = tcg_const_i64(disp16 << 16);
> +	    if (rb != 31)
> +		tcg_gen_add_i64(v, cpu_ir[rb], v);
> +	    set_ir(v, ra);
> +	    tcg_temp_free(v);
> +	}
>          break;
>      case 0x0A:
>          /* LDBU */
> @@ -1871,13 +1905,16 @@
>          break;
>      case 0x30:
>          /* BR */
> -        gen_set_uT0(ctx, ctx->pc);
> -        gen_store_ir(ctx, ra, 0);
> -        if (disp21 != 0) {
> -            gen_set_sT1(ctx, disp21 << 2);
> -            gen_op_addq();
> +	if (ra != 31) {
> +	    TCGv t = tcg_const_i64(ctx->pc);
> +	    set_ir(t, ra);
> +	    tcg_temp_free(t);
> +	}
> +	{
> +	    TCGv pc = tcg_const_i64(ctx->pc + (disp21 << 2));
> +	    tcg_gen_mov_i64(cpu_pc, pc);
> +	    tcg_temp_free(pc);
>          }

Same here, tcg_gen_movi_i64 could be use instead.

> -        gen_op_branch();
>          ret = 1;
>          break;
>      case 0x31:
> @@ -2059,7 +2096,7 @@
>          gen_update_pc(&ctx);
>      }
>  #if defined (DO_TB_FLUSH)
> -    gen_op_tb_flush();
> +    tcg_gen_helper_0_0(helper_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

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

[Thiemo Seufer]

Aurelien Jarno wrote:
> On Wed, Sep 03, 2008 at 02:44:48PM +0200, Tristan Gingold wrote:
> > Hi,
> >
> > all the issues have been fixed in this second version.
> >
> > Tristan.
> >
> > Signed-off-by: Tristan Gingold <gingold@adacore.com>
> >
> 
> I have applied a modified version of this patch, as I don't want to
> spend to much time doing mail ping pong. Please see the details of the
> change below.
> 
> Also please watch the indentation, target-alpha/ is indented with space,
> I am not sure we want to mix that.

Apparently you missed to commit target-alpha/helper.h.


Thiemo

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

[Aurelien Jarno]

On Thu, Sep 04, 2008 at 09:51:37PM +0200, Thiemo Seufer wrote:
> Aurelien Jarno wrote:
> > On Wed, Sep 03, 2008 at 02:44:48PM +0200, Tristan Gingold wrote:
> > > Hi,
> > >
> > > all the issues have been fixed in this second version.
> > >
> > > Tristan.
> > >
> > > Signed-off-by: Tristan Gingold <gingold@adacore.com>
> > >
> > 
> > I have applied a modified version of this patch, as I don't want to
> > spend to much time doing mail ping pong. Please see the details of the
> > change below.
> > 
> > Also please watch the indentation, target-alpha/ is indented with space,
> > I am not sure we want to mix that.
> 
> Apparently you missed to commit target-alpha/helper.h.
> 

Oops, sorry about that. That should be fixed now.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net