From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45551)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Z7jqY-0007fF-B5
	for qemu-devel@nongnu.org; Wed, 24 Jun 2015 08:32:07 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Z7jqT-0001Ep-Bo
	for qemu-devel@nongnu.org; Wed, 24 Jun 2015 08:32:06 -0400
Received: from mailapp01.imgtec.com ([195.59.15.196]:46879)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Z7jqT-0001EO-5j
	for qemu-devel@nongnu.org; Wed, 24 Jun 2015 08:32:01 -0400
Message-ID: <558AA333.8000602@imgtec.com>
Date: Wed, 24 Jun 2015 13:31:47 +0100
From: Leon Alrae <leon.alrae@imgtec.com>
MIME-Version: 1.0
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>
In-Reply-To: <20150624110442.GA15630@aurel32.net>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 04/15] target-mips: refactor {D}LSA,
	{D}ALIGN, {D}BITSWAP
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Aurelien Jarno <aurelien@aurel32.net>, Yongbok Kim <yongbok.kim@imgtec.com>
Cc: qemu-devel@nongnu.org

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.

Thanks,
Leon