From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQVOV-0000PK-NT for qemu-devel@nongnu.org; Fri, 22 Jul 2016 04:01:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQVOQ-0006gK-R7 for qemu-devel@nongnu.org; Fri, 22 Jul 2016 04:01:15 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:39831) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQVOQ-0006fi-In for qemu-devel@nongnu.org; Fri, 22 Jul 2016 04:01:10 -0400 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u6M7sMnh050808 for ; Fri, 22 Jul 2016 04:01:09 -0400 Received: from e23smtp02.au.ibm.com (e23smtp02.au.ibm.com [202.81.31.144]) by mx0a-001b2d01.pphosted.com with ESMTP id 24assuu10k-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 22 Jul 2016 04:01:08 -0400 Received: from localhost by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 22 Jul 2016 18:01:06 +1000 From: Nikunj A Dadhania In-Reply-To: <20160722071229.GW15941@voom.fritz.box> References: <1468861517-2508-1-git-send-email-nikunj@linux.vnet.ibm.com> <1468861517-2508-6-git-send-email-nikunj@linux.vnet.ibm.com> <20160722045137.GO15941@voom.fritz.box> <87d1m69r69.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> <20160722060948.GT15941@voom.fritz.box> <874m7i9n7k.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> <20160722071229.GW15941@voom.fritz.box> Date: Fri, 22 Jul 2016 13:30:57 +0530 MIME-Version: 1.0 Content-Type: text/plain Message-Id: <87wpke85l2.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Subject: Re: [Qemu-devel] [RFC v1 05/13] target-ppc: add modulo word operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com, benh@kernel.crashing.org David Gibson writes: > [ Unknown signature status ] > On Fri, Jul 22, 2016 at 12:24:55PM +0530, Nikunj A Dadhania wrote: >> David Gibson writes: >> >> > [ Unknown signature status ] >> > On Fri, Jul 22, 2016 at 10:59:18AM +0530, Nikunj A Dadhania wrote: >> >> David Gibson writes: >> >> >> >> > [ Unknown signature status ] >> >> > On Mon, Jul 18, 2016 at 10:35:09PM +0530, Nikunj A Dadhania wrote: >> >> >> Adding following instructions: >> >> >> >> >> >> moduw: Modulo Unsigned Word >> >> >> modsw: Modulo Signed Word >> >> >> >> >> >> Signed-off-by: Nikunj A Dadhania >> >> > >> >> > As rth has already mentioned this many branches probably means this >> >> > wants a helper. >> >> > >> >> >> --- >> >> >> target-ppc/translate.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> >> >> 1 file changed, 48 insertions(+) >> >> >> >> >> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >> >> >> index d44f7af..487dd94 100644 >> >> >> --- a/target-ppc/translate.c >> >> >> +++ b/target-ppc/translate.c >> >> >> @@ -1178,6 +1178,52 @@ GEN_DIVE(divde, divde, 0); >> >> >> GEN_DIVE(divdeo, divde, 1); >> >> >> #endif >> >> >> >> >> >> +static inline void gen_op_arith_modw(DisasContext *ctx, TCGv ret, TCGv arg1, >> >> >> + TCGv arg2, int sign) >> >> >> +{ >> >> >> + TCGLabel *l1 = gen_new_label(); >> >> >> + TCGLabel *l2 = gen_new_label(); >> >> >> + TCGv_i32 t0 = tcg_temp_local_new_i32(); >> >> >> + TCGv_i32 t1 = tcg_temp_local_new_i32(); >> >> >> + TCGv_i32 t2 = tcg_temp_local_new_i32(); >> >> >> + >> >> >> + tcg_gen_trunc_tl_i32(t0, arg1); >> >> >> + tcg_gen_trunc_tl_i32(t1, arg2); >> >> >> + tcg_gen_brcondi_i32(TCG_COND_EQ, t1, 0, l1); >> >> >> >> Result for: >> >> % 0 and ... >> >> >> >> >> + if (sign) { >> >> >> + TCGLabel *l3 = gen_new_label(); >> >> >> + tcg_gen_brcondi_i32(TCG_COND_NE, t1, -1, l3); >> >> >> + tcg_gen_brcondi_i32(TCG_COND_EQ, t0, INT32_MIN, l1); >> >> >> + gen_set_label(l3); >> >> > >> >> > It's not really clear to be what the logic above is doing. >> >> >> >> ... For signed case >> >> 0x8000_0000 % -1 >> >> >> >> Is undefined, addressing those cases. >> > >> > Do you mean the tcg operations have undefined results or that the ppc >> > instructions have undefined results? >> >> TCG side, I haven't tried. >> >> > If the latter, then why do you care about those cases? >> >> Thats how divd is implemented as well, i didn't want to break that. I am >> looking at doing both div and mod as helpers. >> >> >> >> + tcg_gen_rem_i32(t2, t0, t1); >> >> >> + } else { >> >> >> + tcg_gen_remu_i32(t2, t0, t1); >> >> >> + } >> >> >> + tcg_gen_br(l2); >> >> >> + gen_set_label(l1); >> >> >> + if (sign) { >> >> >> + tcg_gen_sari_i32(t2, t0, 31); >> >> > >> >> > AFAICT this sets t2 to either 0 or -1 depending on the sign of t0, >> >> > which seems like an odd thing to do. >> >> >> >> Extending the sign later ... >> > >> > Right, so after sign extension you have a 64-bit 0 or -1. Still not >> > seeing what that 0 or -1 result is useful for. >> >> Oh ok, i got why you got confused. I am re-writing all of it though, but >> for understanding: >> >> if (divisor == 0) >> goto l1; >> >> if (signed) { >> if (divisor == -1 && dividend == INT_MIN) >> goto l1; >> compute_signed_rem(t2, t0, t1); >> } else { >> compute_unsigned_rem(t2, t0, t1); >> } >> goto l2; /* jump to setting extending result and return */ >> >> l1: /* in case of invalid input set values */ >> if (signed) >> t2 = -1 or 0; >> else >> t2 = 0; > > Ok, so why do you ever need different result values in the case of > invalid input? Why is always returning 0 not good enough? Let me go through the spec, as divd does the same thing. Regards Nikunj