diff mbox

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

Message ID 1341523740-22711-4-git-send-email-peter.maydell@linaro.org
State Accepted
Commit bedc2ac1a746e61e2a42c98603922c488b82cddb
Headers show

Commit Message

Peter Maydell July 5, 2012, 9:29 p.m. UTC
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(-)

Comments

Stefan Weil July 6, 2012, 5:49 a.m. UTC | #1
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.
diff mbox

Patch

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;