From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uhb4O-0005uJ-NG for qemu-devel@nongnu.org; Wed, 29 May 2013 03:45:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uhb4G-00008A-7l for qemu-devel@nongnu.org; Wed, 29 May 2013 03:45:16 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:60997) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uhb4F-00007O-WF for qemu-devel@nongnu.org; Wed, 29 May 2013 03:45:08 -0400 Message-ID: <51A5B1F0.2050305@huawei.com> Date: Wed, 29 May 2013 09:44:48 +0200 From: Claudio Fontana MIME-Version: 1.0 References: <51A4CBD7.9020105@huawei.com> <51A4CD19.1080108@huawei.com> <51A4D8CB.8060904@twiddle.net> In-Reply-To: <51A4D8CB.8060904@twiddle.net> 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: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: Laurent Desnogues , Peter Maydell , Jani Kokkonen , "qemu-devel@nongnu.org" On 28.05.2013 18:18, Richard Henderson wrote: > 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? yes, you only forgot to encode the shift in the tcg_out32, the variation above was an attempt to make it easier to understand. I agree that the approach that avoids changing value in the right shift is more concise, I'll go back to that, adding a comment about how the function works. >> +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_out_movi is an interface with tcg, and as such the prototype is fixed. I'd rather work with a value that is unsigned, because of the right shift. Having a separate _aux function does that without the need for adding another local variable and another operation to understand in the function with the actual algorithm. > >> + 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. Ok. > r~ Claudio