From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52198)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Wv0UF-0004yk-I5
	for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:36:01 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Wv0U9-0004Tk-8O
	for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:35:55 -0400
Received: from mailapp01.imgtec.com ([195.59.15.196]:36672)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1Wv0U9-0004Te-1y
	for qemu-devel@nongnu.org; Thu, 12 Jun 2014 04:35:49 -0400
Message-ID: <5399665D.4020503@imgtec.com>
Date: Thu, 12 Jun 2014 09:35:41 +0100
From: Leon Alrae <leon.alrae@imgtec.com>
MIME-Version: 1.0
References: <1402499992-64851-1-git-send-email-leon.alrae@imgtec.com>
	<1402499992-64851-13-git-send-email-leon.alrae@imgtec.com>
	<53988650.30605@twiddle.net>
In-Reply-To: <53988650.30605@twiddle.net>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 12/22] target-mips: add ALIGN, DALIGN,
 BITSWAP and DBITSWAP instructions
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: Richard Henderson <rth@twiddle.net>, qemu-devel@nongnu.org
Cc: yongbok.kim@imgtec.com, cristian.cuna@imgtec.com, aurelien@aurel32.net

On 11/06/2014 17:39, Richard Henderson wrote:
> On 06/11/2014 08:19 AM, Leon Alrae wrote:
>> * add missing zero register case
> 
> What missing zero register case?
> 
>> +        if (rd == 0) {
>> +            /* Treat as NOP. */
>> +            break;
>> +        }
> 
> This is normally handled by gen_store_gpr...
> 
>> +            if (rt == 0) {
>> +                tcg_gen_movi_tl(cpu_gpr[rd], 0);
>> +            } else {
>> +                gen_helper_dbitswap(cpu_gpr[rd], cpu_gpr[rt]);
>> +            }
>> +            break;
> 
> ... and this is normally handed by gen_load_gpr.
> 
> Open-coding these tests just clutters the code, making it harder to read.  C.f.
> the 1500 lines removed during a cleanup of target-alpha for exactly this sort
> of thing.

Yes, it clutters the code a bit. However, by looking at the other
instructions I got the impression that preferable approach is to avoid
generating unnecessary code if we can. This seems to be sensible so I'm
trying to stick to it. So in bitswap example I think there is no point
in calling the helper if we know that the result will be 0.

Regards,
Leon