From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X6nRW-0001wX-A3 for qemu-devel@nongnu.org; Mon, 14 Jul 2014 17:05:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X6nRM-0006ir-Ty for qemu-devel@nongnu.org; Mon, 14 Jul 2014 17:05:50 -0400 Received: from mail-qa0-x22a.google.com ([2607:f8b0:400d:c00::22a]:58184) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X6nRM-0006hz-8e for qemu-devel@nongnu.org; Mon, 14 Jul 2014 17:05:40 -0400 Received: by mail-qa0-f42.google.com with SMTP id j15so3773482qaq.15 for ; Mon, 14 Jul 2014 14:05:39 -0700 (PDT) Sender: Richard Henderson Message-ID: <53C4461F.2070005@twiddle.net> Date: Mon, 14 Jul 2014 14:05:35 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1405359671-25985-1-git-send-email-kbastian@mail.uni-paderborn.de> <1405359671-25985-7-git-send-email-kbastian@mail.uni-paderborn.de> In-Reply-To: <1405359671-25985-7-git-send-email-kbastian@mail.uni-paderborn.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/15] target-tricore: Add instructions of SRC opcode format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bastian Koppelmann , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org On 07/14/2014 10:41 AM, Bastian Koppelmann wrote: > +target_ulong helper_shac(CPUTRICOREState *env, target_ulong r1, > + target_ulong r2, target_ulong len) > +{ > + target_ulong carry_out, msk, msk_start, msk_len, ret; > + int32_t shift_count; > + int const6; > + const6 = sextract32(r2, 0, len); > + > + if (const6 >= 0) { > + if (const6 != 0) { > + msk_start = 32 - const6; > + msk_len = 31-msk_start; > + msk = ((1 << msk_len) - 1) << msk_start; > + carry_out = ((r1 & msk) != 0); > + } else { > + carry_out = 0; > + } > + ret = r1 << const6; > + } else { > + > + shift_count = 0 - const6; > + ret = (int32_t)r1 >> shift_count; > + msk = (1 << (shift_count - 1)) - 1; > + carry_out = ((r1 & msk) != 0); > + } > + if (carry_out) { > + /* TODO: carry out */ > + } > + return ret; > +} Why a helper for SHA? It's not any more difficult than SH. > +static void gen_shi(TCGv ret, TCGv r1, int32_t r2, int len) > +{ > +/* shift_count = sign_ext(const4[3:0]); > + D[a] = (shift_count >= 0) ? D[a] << shift_count : D[a] >> (-shift_count); */ > + int shift_count = sextract32(r2, 0, len); Careful with your documentation: you're adding the 16-bit documentation as opposed to the more generic 32-bit documentation. I do not think you should have a "len" parameter at all. We've already sign-extended const4 in decode_src_opc, so there's no need to do it again. > +static void gen_shaci(TCGv ret, TCGv r1, int32_t con, int len) > +{ > + TCGv temp = tcg_const_i32(con); > + > + gen_shac(ret, r1, temp, len); > + > + tcg_temp_free(temp); > +} In particular, SHACI with an immediate is pretty much exactly SHI except with an arithmetic right shift instead of a logical right shift. (Yes, there's some carry and overflow bit computation to do, but it's not like you've implemented any of that in your current implementation either.) > + > +/* > + * Functions for decoding instructions > + */ > + > +static void decode_src_opc(DisasContext *ctx, int op1) > +{ > + int r1; > + int32_t const4; > + TCGv temp, temp2; > + > + r1 = MASK_OP_SRC_S1D(ctx->opcode); > + const4 = MASK_OP_SRC_CONST4_SEXT(ctx->opcode); > + > + switch (op1) { > + > + case OPC1_16_SRC_ADD: Watch the silly blank likes, above the case. And the end-of-file blank lines in some of the other patches. > + tcg_gen_addi_tl(cpu_gpr_d[r1], cpu_gpr_d[r1], const4); Are you planning to come back to implement V and AV bits later? > + case OPC1_16_SRC_MOV_A: > + /* load const4 again unsigned */ > + const4 = MASK_OP_SRC_CONST4(ctx->opcode); > + tcg_gen_movi_tl(cpu_gpr_a[r1], const4); Err.. I don't think this is right. I see "signed" on page 3-224. > + case OPC1_16_SRC_SHA: > + /* FIXME: const too long */ > + gen_shaci(cpu_gpr_d[r1], cpu_gpr_d[r1], const4, 4); > + break; Huh? Why the fixme? r~