From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53406) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPc02-0006M0-Vr for qemu-devel@nongnu.org; Fri, 27 Sep 2013 13:38:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPbzu-0002aB-Hp for qemu-devel@nongnu.org; Fri, 27 Sep 2013 13:38:42 -0400 Received: from mail-pb0-x230.google.com ([2607:f8b0:400e:c01::230]:43568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPbzu-0002a4-A2 for qemu-devel@nongnu.org; Fri, 27 Sep 2013 13:38:34 -0400 Received: by mail-pb0-f48.google.com with SMTP id ma3so2832247pbc.21 for ; Fri, 27 Sep 2013 10:38:33 -0700 (PDT) Sender: Richard Henderson Message-ID: <5245C295.2020907@twiddle.net> Date: Fri, 27 Sep 2013 10:38:29 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1380242934-20953-1-git-send-email-agraf@suse.de> <1380242934-20953-12-git-send-email-agraf@suse.de> In-Reply-To: <1380242934-20953-12-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/60] AArch64: Add STP instruction 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: > +static int get_reg(uint32_t inst) > +{ > + return get_bits(inst, 0, 5); > +} Surely get_rt or some such related to the actual field name, not something so generic as "reg" which applies equally to any of several fields. > +static void ldst_do_vec(DisasContext *s, int dest, TCGv_i64 tcg_addr_real, > + int size, bool is_store) > +{ > + TCGv_i64 tcg_addr = tcg_temp_new_i64(); > + int freg_offs = offsetof(CPUARMState, vfp.regs[dest * 2]); > + > + /* we don't want to modify the caller's tcg_addr */ > + tcg_gen_mov_i64(tcg_addr, tcg_addr_real); Surely merge with ldst_do_vec_int, for the single case of 16 byte size? > + > + if (!is_store) { > + /* normal ldst clears non-loaded bits */ > + clear_fpreg(dest); > + } Surely merge with ldst_do_vec_int, to avoid redundant stores into the vfp register set? > +static void handle_stp(DisasContext *s, uint32_t insn) > +{ This handles both ldp and stp though, right? > + ldst_do(s, rt, tcg_addr, size, is_store, is_signed, is_vector); > + tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size); > + ldst_do(s, rt2, tcg_addr, size, is_store, is_signed, is_vector); > + tcg_gen_subi_i64(tcg_addr, tcg_addr, 1 << size); Why subtract instead of using another temporary? > + /* Typical major opcode encoding */ > switch ((insn >> 24) & 0x1f) { > + case 0x08: > + case 0x09: > + if (get_bits(insn, 29, 1)) { > + handle_stp(s, insn); > + } else { > + unallocated_encoding(s); > + } > + break; > + case 0x0c: > + if (get_bits(insn, 29, 1)) { > + handle_stp(s, insn); > + } else { > + unallocated_encoding(s); > + } > + break; > + case 0x0d: > + if (get_bits(insn, 29, 1)) { > + handle_stp(s, insn); > + } else { > + unallocated_encoding(s); > + } > + break; This leads me to believe that this isn't the best arrangement of the decoder. I would expect the structure to look more like the arrangement in chapter C3, especially the major encoding groups in table C3-1. r~