[Qemu-devel] [PATCH 0/3] target-i386: minor code cleanups

[Qemu-devel] [PATCH 0/3] target-i386: minor code cleanups

[Peter Maydell]

These three patches fix some minor code cleanliness issues I
noticed while reviewing Stefan Weil's recent build-breakage
fixing patch. No behavioural changes.

Peter Maydell (3):
  target-i386: Remove unused macros
  target-i386: Remove confusing X86_64_DEF macro
  target-i386: make it clearer that op table accesses don't overrun

 target-i386/translate.c |   57 ++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 27 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/3] target-i386: Remove unused macros

[Peter Maydell]

Commit 11f8cdb removed all the uses of the X86_64_ONLY
macro. The BUGGY_64() macro has been unused for a long time:
it originally marked some ops which couldn't be enabled
because of issues with the pre-TCG code generation scheme.
Remove the now-unnecessary definitions of both macros.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-i386/translate.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index a00a6a1..8d696ea 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -38,17 +38,11 @@
 #define PREFIX_ADR    0x10
 
 #ifdef TARGET_X86_64
-#define X86_64_ONLY(x) x
 #define X86_64_DEF(...)  __VA_ARGS__
 #define CODE64(s) ((s)->code64)
 #define REX_X(s) ((s)->rex_x)
 #define REX_B(s) ((s)->rex_b)
-/* XXX: gcc generates push/pop in some opcodes, so we cannot use them */
-#if 1
-#define BUGGY_64(x) NULL
-#endif
 #else
-#define X86_64_ONLY(x) NULL
 #define X86_64_DEF(...)
 #define CODE64(s) 0
 #define REX_X(s) 0
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 1/3] target-i386: Remove unused macros

[Stefan Weil]

Am 05.07.2012 23:28, schrieb Peter Maydell:
> Commit 11f8cdb removed all the uses of the X86_64_ONLY
> macro. The BUGGY_64() macro has been unused for a long time:
> it originally marked some ops which couldn't be enabled
> because of issues with the pre-TCG code generation scheme.
> Remove the now-unnecessary definitions of both macros.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target-i386/translate.c |    6 ------
>   1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index a00a6a1..8d696ea 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -38,17 +38,11 @@
>   #define PREFIX_ADR    0x10
>   
>   #ifdef TARGET_X86_64
> -#define X86_64_ONLY(x) x
>   #define X86_64_DEF(...)  __VA_ARGS__
>   #define CODE64(s) ((s)->code64)
>   #define REX_X(s) ((s)->rex_x)
>   #define REX_B(s) ((s)->rex_b)
> -/* XXX: gcc generates push/pop in some opcodes, so we cannot use them */
> -#if 1
> -#define BUGGY_64(x) NULL
> -#endif
>   #else
> -#define X86_64_ONLY(x) NULL
>   #define X86_64_DEF(...)
>   #define CODE64(s) 0
>   #define REX_X(s) 0

Reviewed-by: Stefan Weil <sw@weilnetz.de>

[Qemu-devel] [PATCH 2/3] target-i386: Remove confusing X86_64_DEF macro

[Peter Maydell]

The X86_64_DEF macro is a confusing way of making some terms
in a conditional only appear if TARGET_X86_64 is defined. We
only use it in two places, and in both cases this is for making
the same test, so abstract that check out into a function
where we can use a more conventional #ifdef.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-i386/translate.c |   39 ++++++++++++++++++++++++---------------
 1 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 8d696ea..5899e09 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -38,12 +38,10 @@
 #define PREFIX_ADR    0x10
 
 #ifdef TARGET_X86_64
-#define X86_64_DEF(...)  __VA_ARGS__
 #define CODE64(s) ((s)->code64)
 #define REX_X(s) ((s)->rex_x)
 #define REX_B(s) ((s)->rex_b)
 #else
-#define X86_64_DEF(...)
 #define CODE64(s) 0
 #define REX_X(s) 0
 #define REX_B(s) 0
@@ -265,11 +263,30 @@ static inline void gen_op_andl_A0_ffff(void)
 #define REG_LH_OFFSET 4
 #endif
 
+/* In instruction encodings for byte register accesses the
+ * register number usually indicates "low 8 bits of register N";
+ * however there are some special cases where N 4..7 indicates
+ * [AH, CH, DH, BH], ie "bits 15..8 of register N-4". Return
+ * true for this special case, false otherwise.
+ */
+static inline bool byte_reg_is_xH(int reg)
+{
+    if (reg < 4) {
+        return false;
+    }
+#ifdef TARGET_X86_64
+    if (reg >= 8 || x86_64_hregs) {
+        return false;
+    }
+#endif
+    return true;
+}
+
 static inline void gen_op_mov_reg_v(int ot, int reg, TCGv t0)
 {
     switch(ot) {
     case OT_BYTE:
-        if (reg < 4 X86_64_DEF( || reg >= 8 || x86_64_hregs)) {
+        if (!byte_reg_is_xH(reg)) {
             tcg_gen_deposit_tl(cpu_regs[reg], cpu_regs[reg], t0, 0, 8);
         } else {
             tcg_gen_deposit_tl(cpu_regs[reg - 4], cpu_regs[reg - 4], t0, 8, 8);
@@ -324,19 +341,11 @@ static inline void gen_op_mov_reg_A0(int size, int reg)
 
 static inline void gen_op_mov_v_reg(int ot, TCGv t0, int reg)
 {
-    switch(ot) {
-    case OT_BYTE:
-        if (reg < 4 X86_64_DEF( || reg >= 8 || x86_64_hregs)) {
-            goto std_case;
-        } else {
-            tcg_gen_shri_tl(t0, cpu_regs[reg - 4], 8);
-            tcg_gen_ext8u_tl(t0, t0);
-        }
-        break;
-    default:
-    std_case:
+    if (ot == OT_BYTE && byte_reg_is_xH(reg)) {
+        tcg_gen_shri_tl(t0, cpu_regs[reg - 4], 8);
+        tcg_gen_ext8u_tl(t0, t0);
+    } else {
         tcg_gen_mov_tl(t0, cpu_regs[reg]);
-        break;
     }
 }
 
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/3] target-i386: make it clearer that op table accesses don't overrun

[Peter Maydell]

Rephrase some of the expressions used to select an entry
in the SSE op table arrays so that it's clearer that they
don't overrun the op table array size.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-i386/translate.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 5899e09..1988dae 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2964,16 +2964,16 @@ static const SSEFunc_0_pl sse_op_table3aq[] = {
 
 static const SSEFunc_i_p sse_op_table3bi[] = {
     gen_helper_cvttss2si,
-    gen_helper_cvttsd2si,
     gen_helper_cvtss2si,
+    gen_helper_cvttsd2si,
     gen_helper_cvtsd2si
 };
 
 #ifdef TARGET_X86_64
 static const SSEFunc_l_p sse_op_table3bq[] = {
     gen_helper_cvttss2sq,
-    gen_helper_cvttsd2sq,
     gen_helper_cvtss2sq,
+    gen_helper_cvttsd2sq,
     gen_helper_cvtsd2sq
 };
 #endif
@@ -3571,12 +3571,12 @@ static void gen_sse(DisasContext *s, int b, target_ulong pc_start, int rex_r)
             op1_offset = offsetof(CPUX86State,xmm_regs[reg]);
             tcg_gen_addi_ptr(cpu_ptr0, cpu_env, op1_offset);
             if (ot == OT_LONG) {
-                SSEFunc_0_pi sse_fn_pi = sse_op_table3ai[(b >> 8) - 2];
+                SSEFunc_0_pi sse_fn_pi = sse_op_table3ai[(b >> 8) & 1];
                 tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
                 sse_fn_pi(cpu_ptr0, cpu_tmp2_i32);
             } else {
 #ifdef TARGET_X86_64
-                SSEFunc_0_pl sse_fn_pl = sse_op_table3aq[(b >> 8) - 2];
+                SSEFunc_0_pl sse_fn_pl = sse_op_table3aq[(b >> 8) & 1];
                 sse_fn_pl(cpu_ptr0, cpu_T[0]);
 #else
                 goto illegal_op;
@@ -3635,13 +3635,13 @@ static void gen_sse(DisasContext *s, int b, target_ulong pc_start, int rex_r)
             tcg_gen_addi_ptr(cpu_ptr0, cpu_env, op2_offset);
             if (ot == OT_LONG) {
                 SSEFunc_i_p sse_fn_i_p =
-                    sse_op_table3bi[(b >> 8) - 2 + (b & 1) * 2];
+                    sse_op_table3bi[((b >> 7) & 2) | (b & 1)];
                 sse_fn_i_p(cpu_tmp2_i32, cpu_ptr0);
                 tcg_gen_extu_i32_tl(cpu_T[0], cpu_tmp2_i32);
             } else {
 #ifdef TARGET_X86_64
                 SSEFunc_l_p sse_fn_l_p =
-                    sse_op_table3bq[(b >> 8) - 2 + (b & 1) * 2];
+                    sse_op_table3bq[((b >> 7) & 2) | (b & 1)];
                 sse_fn_l_p(cpu_T[0], cpu_ptr0);
 #else
                 goto illegal_op;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/3] target-i386: make it clearer that op table accesses don't overrun

[Stefan Weil]

Please see comments below.

Am 05.07.2012 23:29, schrieb Peter Maydell:
> Rephrase some of the expressions used to select an entry
> in the SSE op table arrays so that it's clearer that they
> don't overrun the op table array size.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target-i386/translate.c |   12 ++++++------
>   1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 5899e09..1988dae 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -2964,16 +2964,16 @@ static const SSEFunc_0_pl sse_op_table3aq[] = {
>   
>   static const SSEFunc_i_p sse_op_table3bi[] = {
>       gen_helper_cvttss2si,
> -    gen_helper_cvttsd2si,
>       gen_helper_cvtss2si,
> +    gen_helper_cvttsd2si,
>       gen_helper_cvtsd2si
>   };
>   
>   #ifdef TARGET_X86_64
>   static const SSEFunc_l_p sse_op_table3bq[] = {
>       gen_helper_cvttss2sq,
> -    gen_helper_cvttsd2sq,
>       gen_helper_cvtss2sq,
> +    gen_helper_cvttsd2sq,
>       gen_helper_cvtsd2sq
>   };
>   #endif
> @@ -3571,12 +3571,12 @@ static void gen_sse(DisasContext *s, int b, target_ulong pc_start, int rex_r)
>               op1_offset = offsetof(CPUX86State,xmm_regs[reg]);
>               tcg_gen_addi_ptr(cpu_ptr0, cpu_env, op1_offset);
>               if (ot == OT_LONG) {
> -                SSEFunc_0_pi sse_fn_pi = sse_op_table3ai[(b >> 8) - 2];
> +                SSEFunc_0_pi sse_fn_pi = sse_op_table3ai[(b >> 8) & 1];
>                   tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
>                   sse_fn_pi(cpu_ptr0, cpu_tmp2_i32);
>               } else {
>   #ifdef TARGET_X86_64
> -                SSEFunc_0_pl sse_fn_pl = sse_op_table3aq[(b >> 8) - 2];
> +                SSEFunc_0_pl sse_fn_pl = sse_op_table3aq[(b >> 8) & 1];
>                   sse_fn_pl(cpu_ptr0, cpu_T[0]);
>   #else
>                   goto illegal_op;
> @@ -3635,13 +3635,13 @@ static void gen_sse(DisasContext *s, int b, target_ulong pc_start, int rex_r)

Here I suggest to reorder the case statements:

         case 0x22c: /* cvttss2si */
         case 0x22d: /* cvtss2si */
         case 0x32c: /* cvttsd2si */
         case 0x32d: /* cvtsd2si */

That's the numeric order, and it matches the order in the modified
array again.

>
>               tcg_gen_addi_ptr(cpu_ptr0, cpu_env, op2_offset);
>               if (ot == OT_LONG) {
>                   SSEFunc_i_p sse_fn_i_p =
> -                    sse_op_table3bi[(b >> 8) - 2 + (b & 1) * 2];
> +                    sse_op_table3bi[((b >> 7) & 2) | (b & 1)];
>                   sse_fn_i_p(cpu_tmp2_i32, cpu_ptr0);
>                   tcg_gen_extu_i32_tl(cpu_T[0], cpu_tmp2_i32);
>               } else {
>   #ifdef TARGET_X86_64
>                   SSEFunc_l_p sse_fn_l_p =
> -                    sse_op_table3bq[(b >> 8) - 2 + (b & 1) * 2];
> +                    sse_op_table3bq[((b >> 7) & 2) | (b & 1)];
>                   sse_fn_l_p(cpu_T[0], cpu_ptr0);
>   #else
>                   goto illegal_op;

Maybe a 2-dimensional array would make that code even
more readable:

sse_op_table3bq[(b >> 8) & 1][b & 1];


Anyway, you may add a

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Regards,

Stefan W.

Re: [Qemu-devel] [PATCH 0/3] target-i386: minor code cleanups

[Blue Swirl]

On Thu, Jul 5, 2012 at 9:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> These three patches fix some minor code cleanliness issues I
> noticed while reviewing Stefan Weil's recent build-breakage
> fixing patch. No behavioural changes.

Thanks, applied all.

>
> Peter Maydell (3):
>   target-i386: Remove unused macros
>   target-i386: Remove confusing X86_64_DEF macro
>   target-i386: make it clearer that op table accesses don't overrun
>
>  target-i386/translate.c |   57 ++++++++++++++++++++++++----------------------
>  1 files changed, 30 insertions(+), 27 deletions(-)
>
> --
> 1.7.5.4
>