From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:58696)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UhMbU-0005m2-4v
	for qemu-devel@nongnu.org; Tue, 28 May 2013 12:18:32 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UhMbQ-0000YU-FX
	for qemu-devel@nongnu.org; Tue, 28 May 2013 12:18:28 -0400
Received: from mail-qe0-f51.google.com ([209.85.128.51]:58264)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <rth7680@gmail.com>) id 1UhMbQ-0000YP-Bi
	for qemu-devel@nongnu.org; Tue, 28 May 2013 12:18:24 -0400
Received: by mail-qe0-f51.google.com with SMTP id nd7so4389906qeb.24
	for <qemu-devel@nongnu.org>; Tue, 28 May 2013 09:18:24 -0700 (PDT)
Sender: Richard Henderson <rth7680@gmail.com>
Message-ID: <51A4D8CB.8060904@twiddle.net>
Date: Tue, 28 May 2013 09:18:19 -0700
From: Richard Henderson <rth@twiddle.net>
MIME-Version: 1.0
References: <51A4CBD7.9020105@huawei.com> <51A4CD19.1080108@huawei.com>
In-Reply-To: <51A4CD19.1080108@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 2/3] tcg/aarch64: implement new TCG
	target for aarch64
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: Claudio Fontana <claudio.fontana@huawei.com>
Cc: Laurent Desnogues <laurent.desnogues@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Jani Kokkonen <Jani.Kokkonen@huawei.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On 05/28/2013 08:28 AM, Claudio Fontana wrote:
> +static inline void tcg_out_movi_aux(TCGContext *s,
> +                                    TCGReg rd, uint64_t value)
> +{
> +    uint32_t half, base, movk = 0, shift = 0;
> +
> +    /* construct halfwords of the immediate with MOVZ/MOVK with LSL */
> +    /* using MOVZ 0x52800000 | extended reg.. */
> +    base = (value > 0xffffffff) ? 0xd2800000 : 0x52800000;
> +
> +    do {
> +        int skip_zeros = ctz64(value) & (63 & -16);
> +        value >>= skip_zeros;
> +        shift += skip_zeros << 17;
> +        half = value & 0xffff;
> +        tcg_out32(s, base | movk | shift | half << 5 | rd);
> +        movk = 0x20000000; /* morph next MOVZs into MOVKs */
> +        value >>= 16;
> +        shift += 16 << 17;

This is way more confusing than it needs to be.  I don't think you
should modify VALUE by shifting at all.  If you don't do that then
you don't need to make SHIFT loop carried, since we compute its
exact correct value every time with the ctz.

Was the only bug in the code that I pasted the lack of the shift-by-17
when encoding SHIFT into the tcg_out32?

> +static inline void tcg_out_movi(TCGContext *s, TCGType type,
> +                                TCGReg rd, tcg_target_long value)
> +{
> +    if (type == TCG_TYPE_I64) {
> +        tcg_out_movi_aux(s, rd, value);
> +    } else {
> +        tcg_out_movi_aux(s, rd, value & 0xffffffff);
> +    }
> +}

Any reason you're splitting out tcg_out_movi_aux to a separate function?

> +    tcg_regset_clear(s->reserved_regs);
> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_SP);
> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_TMP);
> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_X18); /* platform register */

Reserve the frame pointer.


r~