From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34002) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ92t-0001hI-Ay for qemu-devel@nongnu.org; Thu, 21 Jul 2016 04:09:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQ92p-00055m-8t for qemu-devel@nongnu.org; Thu, 21 Jul 2016 04:09:27 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:56352) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ92p-00055i-0n for qemu-devel@nongnu.org; Thu, 21 Jul 2016 04:09:23 -0400 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u6L88xEZ109637 for ; Thu, 21 Jul 2016 04:09:22 -0400 Received: from e28smtp09.in.ibm.com (e28smtp09.in.ibm.com [125.16.236.9]) by mx0a-001b2d01.pphosted.com with ESMTP id 24aeamwst3-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 21 Jul 2016 04:09:22 -0400 Received: from localhost by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 21 Jul 2016 13:39:18 +0530 From: Nikunj A Dadhania In-Reply-To: <8721d012-151c-4502-ce7b-a3eeeebdcfe2@twiddle.net> References: <1468346602-20700-1-git-send-email-nikunj@linux.vnet.ibm.com> <1468346602-20700-5-git-send-email-nikunj@linux.vnet.ibm.com> <8721d012-151c-4502-ce7b-a3eeeebdcfe2@twiddle.net> Date: Thu, 21 Jul 2016 13:38:49 +0530 MIME-Version: 1.0 Content-Type: text/plain Message-Id: <87invz9zvy.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Subject: Re: [Qemu-devel] [RFC 4/6] target-ppc: add cmprb instruction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-ppc@nongnu.org, david@gibson.dropbear.id.au Cc: qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com Richard Henderson writes: > On 07/12/2016 11:33 PM, Nikunj A Dadhania wrote: >> +/* cmprb - range comparison: isupper, isaplha, islower*/ >> +static void gen_cmprb(DisasContext *ctx) >> +{ >> + TCGLabel *lab1 = gen_new_label(); >> + TCGLabel *lab2 = gen_new_label(); >> + TCGv src1 = tcg_temp_local_new(); >> + TCGv src2 = tcg_temp_local_new(); >> + TCGv src2lo = tcg_temp_local_new(); >> + TCGv src2hi = tcg_temp_local_new(); >> + >> + tcg_gen_andi_tl(src1, cpu_gpr[rA(ctx->opcode)], 0xFF); >> + tcg_gen_andi_tl(src2, cpu_gpr[rB(ctx->opcode)], 0xFFFFFFFF); > > There's no point in this mask, since it's covered by > >> + >> + tcg_gen_andi_tl(src2lo, src2, 0xFF); >> + tcg_gen_shri_tl(src2hi, src2, 8); >> + tcg_gen_andi_tl(src2hi, src2hi, 0xFF); > > these ones. Right. >> + >> + tcg_gen_brcond_tl(TCG_COND_GTU, src1, src2hi, lab1); >> + tcg_gen_brcond_tl(TCG_COND_LTU, src1, src2lo, lab1); >> + tcg_gen_movi_i32(cpu_crf[crfD(ctx->opcode)], 1 << CRF_GT); >> + tcg_gen_br(lab2); >> + gen_set_label(lab1); >> + >> + if (ctx->opcode & 0x00200000) { >> + tcg_gen_shri_tl(src2hi, src2, 24); >> + tcg_gen_andi_tl(src2hi, src2hi, 0xFF); >> + tcg_gen_shri_tl(src2lo, src2, 16); >> + tcg_gen_andi_tl(src2lo, src2lo, 0xFF); >> + tcg_gen_brcond_tl(TCG_COND_GTU, src1, src2hi, lab2); >> + tcg_gen_brcond_tl(TCG_COND_LTU, src1, src2lo, lab2); >> + tcg_gen_movi_i32(cpu_crf[crfD(ctx->opcode)], 1 << CRF_GT); >> + } >> + gen_set_label(lab2); >> + tcg_temp_free(src1); >> + tcg_temp_free(src2); >> + tcg_temp_free(src2lo); >> + tcg_temp_free(src2hi); >> +} > > You've forgotten to clear crf in the false case. Yes, next version has the fix. > This is better implemented without branches, like > > TCGv_i32 src1, src2, src2lo, src2hi; > TCGv_i32 crf = cpu_crf[cdfD(ctx->opcode)]; > > // allocate all 4 "src" temps > > tcg_gen_trunc_tl_i32(src1, cpu_gpr[rA(ctx->opcode)]); > tcg_gen_trunc_tl_i32(src2, cpu_gpr[rB(ctx->opcode)]); > > tcg_gen_ext8u_i32(src2lo, src2); > tcg_gen_shri_i32(src2, src2, 8); > tcg_gen_extu8_i32(src2hi, src2hi); > > tcg_gen_setcond_tl(TCG_COND_LEU, src2lo, src2lo, src1); > tcg_gen_setcond_tl(TCG_COND_LEU, src2hi, src1, src2hi); > tcg_gen_and_tl(crf, src2lo, src2hi); > > if (ctx->opcode & 0x00200000) { > tcg_gen_shri_i32(src2, src2, 8); > tcg_gen_ext8u_i32(src2lo, src2); > tcg_gen_shri_i32(src2, src2, 8); > tcg_gen_ext8u_i32(src2hi, src2); > tcg_gen_setcond_i32(TCG_COND_LEU, src2lo, src2lo, src1); > tcg_gen_setcond_i32(TCG_COND_LEU, src2hi, src1, src2hi); > tcg_gen_and_i32(src2lo, src2lo, src2hi); > tcg_gen_or_i32(crf, crf, src2lo); > } > > tcg_gen_shli_i32(crf, crf, CRF_GT); > > // free all 4 "src" temps Sure. Regards Nikunj