From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPd8f-00067A-Rl for qemu-devel@nongnu.org; Fri, 27 Sep 2013 14:51:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPd8X-0007ju-8A for qemu-devel@nongnu.org; Fri, 27 Sep 2013 14:51:41 -0400 Received: from mail-ye0-x231.google.com ([2607:f8b0:4002:c04::231]:37946) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPd8X-0007jj-2O for qemu-devel@nongnu.org; Fri, 27 Sep 2013 14:51:33 -0400 Received: by mail-ye0-f177.google.com with SMTP id q9so982149yen.36 for ; Fri, 27 Sep 2013 11:51:32 -0700 (PDT) Sender: Richard Henderson Message-ID: <5245D3AF.2060900@twiddle.net> Date: Fri, 27 Sep 2013 11:51:27 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1380242934-20953-1-git-send-email-agraf@suse.de> <1380242934-20953-16-git-send-email-agraf@suse.de> In-Reply-To: <1380242934-20953-16-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 15/60] AArch64: Add add instruction family emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Peter Maydell , Michael Matz , qemu-devel@nongnu.org, C Fontana , Dirk Mueller , Laurent Desnogues , Christoffer Dall On 09/26/2013 05:48 PM, Alexander Graf wrote: > + tcg_gen_mov_i64(tcg_src, cpu_reg(source)); > + tcg_dst = cpu_reg(dest); > + if (extend) { > + if ((shift_amount & 0x7) > 4) { > + /* reserved value */ > + unallocated_encoding(s); > + } > + if (!setflags) { > + tcg_gen_mov_i64(tcg_src, cpu_reg_sp(source)); > + tcg_dst = cpu_reg_sp(dest); > + } > + } else { > + if (shift_type == 3) { > + /* reserved value */ > + unallocated_encoding(s); > + } > + if (is_32bit && (shift_amount < 0)) { > + /* reserved value */ > + unallocated_encoding(s); > + } > + } You'd do better to load up the source and destination TCGv values in that IF sequence, and emit one tcg_gen_mov_i64 afterward. At the moment you're emitting two for the extend && !setflags case. > + if (extend) { > + tcg_op2 = tcg_temp_new_i64(); > + reg_extend(tcg_op2, shift_amount >> 3, shift_amount & 0x7, rm); > + } else { > + tcg_op2 = get_shifti(rm, shift_type, shift_amount, is_32bit); > + } Why does get_shifti return a temp, but reg_extend requires one to be passed in? > + if (is_32bit) { > + tcg_gen_ext32s_i64(tcg_src, tcg_src); > + tcg_gen_ext32s_i64(tcg_op2, tcg_op2); > + } Why? You'll zero-extend the result, and the flags setting will truncate the inputs itself. > + if (sub_op) { > + tcg_gen_sub_i64(tcg_result, tcg_src, tcg_op2); > + } else { > + tcg_gen_add_i64(tcg_result, tcg_src, tcg_op2); > + } > + > + if (is_carry) { > + TCGv_i64 tcg_carry = tcg_temp_new_i64(); > + tcg_gen_shri_i64(tcg_carry, pstate, PSTATE_C_SHIFT); > + tcg_gen_andi_i64(tcg_carry, tcg_carry, 1); > + tcg_gen_add_i64(tcg_result, tcg_result, tcg_carry); > + if (sub_op) { > + tcg_gen_subi_i64(tcg_result, tcg_result, 1); > + } > + tcg_temp_free_i64(tcg_carry); > + } For sub_op && is_carry, it's probably better to do exactly what the manual says, rd = rn + ~rm + C, as opposed to rd = rn - rm + c - 1 as you do here. This will be especially true if you eventually split up the flags as is done on the A32 side. One can compute rd plus the new carry via add2. r~