From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33925) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7QXd-0005KE-BH for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:32:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7QXY-0006CY-Ek for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:32:05 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:51206 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c7QXY-0006C1-9g for qemu-devel@nongnu.org; Thu, 17 Nov 2016 12:32:00 -0500 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAHHSuhM022789 for ; Thu, 17 Nov 2016 12:31:58 -0500 Received: from e24smtp03.br.ibm.com (e24smtp03.br.ibm.com [32.104.18.24]) by mx0b-001b2d01.pphosted.com with ESMTP id 26sc768h70-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 17 Nov 2016 12:31:58 -0500 Received: from localhost by e24smtp03.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 17 Nov 2016 15:31:56 -0200 Date: Thu, 17 Nov 2016 15:31:48 -0200 From: joserz@linux.vnet.ibm.com References: <1479326850-8369-1-git-send-email-joserz@linux.vnet.ibm.com> <1479326850-8369-2-git-send-email-joserz@linux.vnet.ibm.com> <20161117034243.GF18808@umbus.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161117034243.GF18808@umbus.fritz.box> Message-Id: <20161117173148.GA25702@pacoca> Subject: Re: [Qemu-devel] [PATCH 1/4] target-ppc: Implement bcdcfsq. instruction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, nikunj@linux.vnet.ibm.com, bharata@linux.vnet.ibm.com Hello David, Thank you for your review. I have just one question below, could you help me to address it please? Thank you! Ziviani On Thu, Nov 17, 2016 at 02:42:43PM +1100, David Gibson wrote: > On Wed, Nov 16, 2016 at 06:07:27PM -0200, Jose Ricardo Ziviani wrote: > > bcdcfsq.: Decimal convert from signed quadword. It is possible to > > I think there should be a "not" in there. > > > convert values less than 10^31-1 or greater than -10^31-1 to be > > represented in packed decimal format. > > > > Signed-off-by: Jose Ricardo Ziviani > > --- > > target-ppc/helper.h | 1 + > > target-ppc/int_helper.c | 48 +++++++++++++++++++++++++++++++++++++ > > target-ppc/translate/vmx-impl.inc.c | 7 ++++++ > > 3 files changed, 56 insertions(+) > > > > diff --git a/target-ppc/helper.h b/target-ppc/helper.h > > index da00f0a..87f533c 100644 > > --- a/target-ppc/helper.h > > +++ b/target-ppc/helper.h > > @@ -382,6 +382,7 @@ DEF_HELPER_3(bcdcfn, i32, avr, avr, i32) > > DEF_HELPER_3(bcdctn, i32, avr, avr, i32) > > DEF_HELPER_3(bcdcfz, i32, avr, avr, i32) > > DEF_HELPER_3(bcdctz, i32, avr, avr, i32) > > +DEF_HELPER_3(bcdcfsq, i32, avr, avr, i32) > > > > DEF_HELPER_2(xsadddp, void, env, i32) > > DEF_HELPER_2(xssubdp, void, env, i32) > > diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c > > index 9ac204a..db65a51 100644 > > --- a/target-ppc/int_helper.c > > +++ b/target-ppc/int_helper.c > > @@ -2874,6 +2874,54 @@ uint32_t helper_bcdctz(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps) > > return cr; > > } > > > > +uint32_t helper_bcdcfsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps) > > +{ > > + int i; > > + int cr = 0; > > + int ox_flag = 0; > > + uint64_t digit = 0; > > + uint64_t carry = 0; > > + uint64_t lo_value = 0; > > + uint64_t hi_value = 0; > > Most of the variables above don't need initializers. > > > + uint64_t max = ULLONG_MAX; > > + ppc_avr_t ret = { .u64 = { 0, 0 } }; > > + > > + if (b->s64[HI_IDX] < 0) { > > + hi_value = -b->s64[HI_IDX]; > > + lo_value = b->s64[LO_IDX]; > > I'm pretty sure this is wrong. Take for example 128-bit -1: > ffffffff ffffffff ffffffff ffffffff > Upper word is negative (64-bit -1), so > hi_value = 00000000 00000001 > lo_value = ffffffff ffffffff > > 0x1 ffffffff ffffffff != +1 > > > + bcd_put_digit(&ret, 0xD, 0); > > + } else if (b->s64[HI_IDX] == 0 && b->s64[LO_IDX] < 0) { > > + lo_value = -b->s64[LO_IDX]; > > + bcd_put_digit(&ret, 0xD, 0); > > + } else { > > + hi_value = b->s64[HI_IDX]; > > + lo_value = b->s64[LO_IDX]; > > + bcd_put_digit(&ret, bcd_preferred_sgn(0, ps), 0); > > + } > > + > > + if (unlikely(hi_value > 0x7e37be2022)) { > > This doesn't look right. Unless by chance 10^31-1 is equal to (k*2^64 > - 1) you need to look at the lo_value as well. > > > + ox_flag = 1; > > You might as well just return 1<< CRF_SO here - no point actually > computing a meaningless value. > > > + } > > + > > + carry = hi_value; > > + for (i = 0; i < 32; i++, max /= 10, lo_value /= 10) { > > Looks like this loop has one too many iterations - there are 32 > iterations, but you only have 31 digits. > > > + digit = ((max % 10) * hi_value) + (lo_value % 10) + carry; > > + carry = (digit > 9) ? digit / 10 : 0; > > + > > + bcd_put_digit(&ret, (carry) ? digit % 10 : digit, i + 1); > > Ugh, this is hard to follow. We're already using an Int128 library in > the memory region code; wonder if we should just use that here as well. > today we have divu128 (host-utils.h) but it doesn't work for me because it coerces the 128bits result in a 64bits variable: __uint128_t result = dividend / divisor; *plow = result; // -> plow is an uint64_t So I wonder if you suggest me (like the idea) to implement div and mod in Int128 lib. Something like: static inline Int128 int128_div64(Int128 a, uint64_t b); static inline Int128 int128_mod64(Int128 a, uint64_t b); But, anyway, these functions would have to implement those hi/lo multiplications for the case "!CONFIG_INT128". > > + } > > + > > + cr = bcd_cmp_zero(&ret); > > + > > + if (unlikely(ox_flag)) { > > + cr |= 1 << CRF_SO; > > + } > > + > > + *r = ret; > > + > > + return cr; > > +} > > + > > void helper_vsbox(ppc_avr_t *r, ppc_avr_t *a) > > { > > int i; > > diff --git a/target-ppc/translate/vmx-impl.inc.c b/target-ppc/translate/vmx-impl.inc.c > > index 7143eb3..36141e5 100644 > > --- a/target-ppc/translate/vmx-impl.inc.c > > +++ b/target-ppc/translate/vmx-impl.inc.c > > @@ -989,10 +989,14 @@ GEN_BCD2(bcdcfn) > > GEN_BCD2(bcdctn) > > GEN_BCD2(bcdcfz) > > GEN_BCD2(bcdctz) > > +GEN_BCD2(bcdcfsq) > > > > static void gen_xpnd04_1(DisasContext *ctx) > > { > > switch (opc4(ctx->opcode)) { > > + case 2: > > + gen_bcdcfsq(ctx); > > + break; > > case 4: > > gen_bcdctz(ctx); > > break; > > @@ -1014,6 +1018,9 @@ static void gen_xpnd04_1(DisasContext *ctx) > > static void gen_xpnd04_2(DisasContext *ctx) > > { > > switch (opc4(ctx->opcode)) { > > + case 2: > > + gen_bcdcfsq(ctx); > > + break; > > case 4: > > gen_bcdctz(ctx); > > break; > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson