From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46894) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn1Pu-0000Ss-Tn for qemu-devel@nongnu.org; Fri, 06 Jul 2012 01:49:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sn1Pt-0003KT-2H for qemu-devel@nongnu.org; Fri, 06 Jul 2012 01:49:22 -0400 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:58007) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn1Ps-0003KN-ON for qemu-devel@nongnu.org; Fri, 06 Jul 2012 01:49:20 -0400 Message-ID: <4FF67C5C.4090900@weilnetz.de> Date: Fri, 06 Jul 2012 07:49:16 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1341523740-22711-1-git-send-email-peter.maydell@linaro.org> <1341523740-22711-4-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1341523740-22711-4-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] target-i386: make it clearer that op table accesses don't overrun List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Blue Swirl , qemu-devel@nongnu.org, patches@linaro.org 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 > --- > 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 Regards, Stefan W.