From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59129) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7kXF-0003x3-2u for qemu-devel@nongnu.org; Wed, 24 Jun 2015 09:16:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7kX9-00069s-1v for qemu-devel@nongnu.org; Wed, 24 Jun 2015 09:16:13 -0400 Received: from [2001:bc8:30d7:101::1] (port=45569 helo=hall.aurel32.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7kX8-00069i-Rm for qemu-devel@nongnu.org; Wed, 24 Jun 2015 09:16:06 -0400 Date: Wed, 24 Jun 2015 15:16:04 +0200 From: Aurelien Jarno Message-ID: <20150624131604.GA22928@aurel32.net> References: <1435073928-21830-1-git-send-email-yongbok.kim@imgtec.com> <1435073928-21830-5-git-send-email-yongbok.kim@imgtec.com> <20150624110442.GA15630@aurel32.net> <558AA333.8000602@imgtec.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <558AA333.8000602@imgtec.com> Subject: Re: [Qemu-devel] [PATCH v3 04/15] target-mips: refactor {D}LSA, {D}ALIGN, {D}BITSWAP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leon Alrae Cc: Yongbok Kim , qemu-devel@nongnu.org On 2015-06-24 13:31, Leon Alrae wrote: > On 24/06/2015 12:04, Aurelien Jarno wrote: > >> +static void gen_align(DisasContext *ctx, int opc, int rd, int rs, int rt, > >> + int bp) > >> { > >> + TCGv t0; > >> + if (rd == 0) { > >> + /* Treat as NOP. */ > >> + return; > >> + } > >> + t0 = tcg_temp_new(); > >> + gen_load_gpr(t0, rt); > >> + if (bp == 0) { > >> + tcg_gen_mov_tl(cpu_gpr[rd], t0); > >> + } else { > >> + TCGv t1 = tcg_temp_new(); > >> + gen_load_gpr(t1, rs); > >> + switch (opc) { > >> + case OPC_ALIGN: > >> + { > >> + TCGv_i64 t2 = tcg_temp_new_i64(); > >> + tcg_gen_concat_tl_i64(t2, t1, t0); > >> + tcg_gen_shri_i64(t2, t2, 8 * (4 - bp)); > >> + gen_move_low32(cpu_gpr[rd], t2); > >> + tcg_temp_free_i64(t2); > >> + } > >> + break; > > > > Not a problem in your patch (you basically just moved code), but I > > think this implementation is incorrect. It should be the same code as > > for DALIGN, but with the input operands zero extended to 32 bits, and > > the result sign extended to 32 bits. Something like that should work: > > > > tcg_gen_ext32u_tl(t0, t0); > > tcg_gen_shli_tl(t0, t0, 8 * bp); > > tcg_gen_ext32u_tl(t1, t1); > > tcg_gen_shri_tl(t1, t1, 8 * (4 - bp)); > > tcg_gen_or_tl(cpu_gpr[rd], t1, t0); > > tcg_gen_ext32s_tl(cpu_gpr[rd], cpu_gpr[rd]); > > > > In practice we can drop the zero extension on t0 (rt) as the bits there > > will be dropped by the sign extension on the result. Note that on > > 32-bit, the zero and sign extension will be dropped, so there is no need > > for #ifdef TARGET_MIPS64. > > I believe existing implementation is correct and does the same thing, but it > operates on the whole 64-bit temp containing merged rs and rt rather than > shifting 32-bit registers separately. We discussed this last year, and the > potential benefit is that it could be slightly faster on 64-bit host. If it is has already been discussed, then: Reviewed-by: Aurelien Jarno -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net