From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41461) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqO6l-0003Q9-Ph for qemu-devel@nongnu.org; Tue, 10 Dec 2013 09:16:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VqO6d-0000LB-UR for qemu-devel@nongnu.org; Tue, 10 Dec 2013 09:16:19 -0500 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:35810 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqO6d-0000L0-OG for qemu-devel@nongnu.org; Tue, 10 Dec 2013 09:16:11 -0500 References: <1386612744-1013-1-git-send-email-peter.maydell@linaro.org> <1386612744-1013-5-git-send-email-peter.maydell@linaro.org> <52A6318B.4070609@twiddle.net> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <52A6318B.4070609@twiddle.net> Date: Tue, 10 Dec 2013 14:16:10 +0000 Message-ID: <87siu06bbp.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 4/9] target-arm: A64: add support for ld/st with reg offset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: Peter Maydell , patches@linaro.org, Michael Matz , qemu-devel@nongnu.org, Claudio Fontana , Dirk Mueller , Will Newton , Laurent Desnogues , kvmarm@lists.cs.columbia.edu, Christoffer Dall rth@twiddle.net writes: > On 12/09/2013 10:12 AM, Peter Maydell wrote: >> From: Alex Bennée >> >> This adds support for the load/store forms using a register offset. >> >> +/* >> + * C3.3.10 Load/store (register offset) >> + * >> + * 31 30 29 27 26 25 24 23 22 21 20 16 15 13 12 11 10 9 5 4 0 >> + * +----+-------+---+-----+-----+---+------+-----+--+-----+----+----+ >> + * |size| 1 1 1 | V | 0 0 | opc | 1 | Rm | opt | S| 1 0 | Rn | Rt | >> + * +----+-------+---+-----+-----+---+------+-----+--+-----+----+----+ >> + * >> + * For non-vector: >> + * size: 00-> byte, 01 -> 16 bit, 10 -> 32bit, 11 -> 64bit >> + * opc: 00 -> store, 01 -> loadu, 10 -> loads 64, 11 -> loads 32 >> + * For vector: >> + * size is opc<1>:size<1:0> so 100 -> 128 bit; 110 and 111 unallocated >> + * opc<0>: 0 -> store, 1 -> load >> + * V: 1 -> vector/simd >> + * opt: extend encoding (see DecodeRegExtend) >> + * S: if S=1 then scale (essentially index by sizeof(size)) >> + * Rt: register to transfer into/out of >> + * Rn: address register or SP for base >> + * Rm: offset register or ZR for offset >> + */ >> +static void handle_ldst_reg_roffset(DisasContext *s, uint32_t insn) >> +{ >> + int rt = extract32(insn, 0, 5); >> + int rn = extract32(insn, 5, 5); >> + int shift = extract32(insn, 12, 1); >> + int rm = extract32(insn, 16, 5); >> + int opc = extract32(insn, 22, 2); >> + int opt = extract32(insn, 13, 3); >> + int size = extract32(insn, 30, 2); >> + bool is_signed = false; >> + bool is_store = false; >> + bool is_extended = false; >> + bool is_vector = extract32(insn, 26, 1); >> + >> + TCGv_i64 tcg_rm; >> + TCGv_i64 tcg_addr; >> + >> + if (extract32(opt, 1, 1) == 0) { >> + unallocated_encoding(s); >> + return; >> + } >> + >> + if (is_vector) { >> + size |= (opc & 2) << 1; >> + if (size > 4) { >> + unallocated_encoding(s); >> + } >> + is_store = ((opc & 1) == 0); >> + } else { >> + if (size == 3 && opc == 2) { >> + /* PRFM - prefetch */ >> + return; >> + } >> + is_store = (opc == 0); >> + is_signed = opc & (1<<1); >> + is_extended = (size < 3) && (opc & 1); >> + } >> + >> + if (rn == 31) { >> + gen_check_sp_alignment(s); >> + } >> + tcg_addr = read_cpu_reg_sp(s, rn, 1); >> + >> + tcg_rm = read_cpu_reg(s, rm, 1); >> + ext_and_shift_reg(tcg_rm, tcg_rm, opt, shift ? size : 0); >> + >> + tcg_gen_add_i64(tcg_addr, tcg_addr, tcg_rm); >> + >> + if (is_vector) { >> + if (is_store) { >> + do_fp_st(s, rt, tcg_addr, size); >> + } else { >> + do_fp_ld(s, rt, tcg_addr, size); >> + } >> + } else { >> + TCGv_i64 tcg_rt = cpu_reg(s, rt); >> + if (is_store) { >> + do_gpr_st(s, tcg_rt, tcg_addr, size); >> + } else { >> + do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended); >> + } >> + } >> +} > > I wonder if it would be better to merge this function with the one from the > previous patch. There are only 3-4 lines that are different. I was trying to avoid having it jump through special cases when decoding the instruction where Rm, opt, S class with the imm field in the other form. But certainly there is an argument for the rest of the common code to be shared (size/opc decoding and final tcg_addr based load/store). However my preference unless there is a strong objection would be to clean that up in later patches. For one thing the more instructions each patch handles the longer it takes to run the instruction validation on the rather slow models to have good coverage of the decoder! > > > r~ -- Alex Bennée QEMU/KVM Hacker for Linaro