[Qemu-devel] [PATCH 0/4] POWER9 TCG enablements - BCD functions part II

[Qemu-devel] [PATCH 0/4] POWER9 TCG enablements - BCD functions part II

[Jose Ricardo Ziviani]

This serie contains 4 new instructions for POWER9 ISA3.0

bcdcfsq.: Convert signed quadword to packed BCD
bcdctsq.: Convert packed BCD to signed quadword
bcdcpsgn.: Copy the sign of a register to another
bcdsetsgn.: Set the BCD sign according to a preferred sign

Jose Ricardo Ziviani (4):
  target-ppc: Implement bcdcfsq. instruction
  target-ppc: Implement bcdctsq. instruction
  target-ppc: Implement bcdcpsgn. instruction
  target-ppc: Implement bcdsetsgn. instruction

 target-ppc/helper.h                 |   4 ++
 target-ppc/int_helper.c             | 126 ++++++++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  25 +++++++
 target-ppc/translate/vmx-ops.inc.c  |   2 +-
 4 files changed, 156 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] target-ppc: Implement bcdcfsq. instruction

[Jose Ricardo Ziviani]

bcdcfsq.: Decimal convert from signed quadword. It is possible to
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 <joserz@linux.vnet.ibm.com>
---
 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;
+    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];
+        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)) {
+        ox_flag = 1;
+    }
+
+    carry = hi_value;
+    for (i = 0; i < 32; i++, max /= 10, lo_value /= 10) {
+        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);
+    }
+
+    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;
-- 
2.7.4

[Qemu-devel] [PATCH 2/4] target-ppc: Implement bcdctsq. instruction

[Jose Ricardo Ziviani]

bcdctsq.: Decimal convert to signed quadword. It is possible to
convert packed decimal values to signed quadwords.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 target-ppc/helper.h                 |  1 +
 target-ppc/int_helper.c             | 39 +++++++++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  7 +++++++
 3 files changed, 47 insertions(+)

diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 87f533c..503f257 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -383,6 +383,7 @@ 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_3(bcdctsq, 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 db65a51..1025438 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2922,6 +2922,45 @@ uint32_t helper_bcdcfsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
     return cr;
 }
 
+uint32_t helper_bcdctsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
+{
+    uint8_t i;
+    int cr = 0;
+    uint64_t hi = 0;
+    int sgnb = bcd_get_sgn(b);
+    int invalid = (sgnb == 0);
+    ppc_avr_t ret = { .u64 = { 0, 0 } };
+
+    ret.u64[LO_IDX] = bcd_get_digit(b, 31, &invalid);
+    for (i = 30; i > 0; i--) {
+        mulu64(&ret.u64[LO_IDX], &hi,
+                ret.u64[LO_IDX], 10ULL);
+
+        ret.u64[HI_IDX] = (ret.u64[HI_IDX]) ? ret.u64[HI_IDX] * 10 + hi : hi;
+        ret.u64[LO_IDX] += bcd_get_digit(b, i, &invalid);
+
+        if (unlikely(invalid)) {
+            break;
+        }
+    }
+
+    if (sgnb == -1) {
+        if (ret.s64[HI_IDX] > 0) {
+            ret.s64[HI_IDX] = -ret.s64[HI_IDX];
+        } else {
+            ret.s64[LO_IDX] = -ret.s64[LO_IDX];
+        }
+    }
+
+    cr = bcd_cmp_zero(b);
+
+    if (unlikely(invalid)) {
+        cr = 1 << CRF_SO;
+    }
+
+    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 36141e5..1579b58 100644
--- a/target-ppc/translate/vmx-impl.inc.c
+++ b/target-ppc/translate/vmx-impl.inc.c
@@ -990,10 +990,14 @@ GEN_BCD2(bcdctn)
 GEN_BCD2(bcdcfz)
 GEN_BCD2(bcdctz)
 GEN_BCD2(bcdcfsq)
+GEN_BCD2(bcdctsq)
 
 static void gen_xpnd04_1(DisasContext *ctx)
 {
     switch (opc4(ctx->opcode)) {
+    case 0:
+        gen_bcdctsq(ctx);
+        break;
     case 2:
         gen_bcdcfsq(ctx);
         break;
@@ -1018,6 +1022,9 @@ static void gen_xpnd04_1(DisasContext *ctx)
 static void gen_xpnd04_2(DisasContext *ctx)
 {
     switch (opc4(ctx->opcode)) {
+    case 0:
+        gen_bcdctsq(ctx);
+        break;
     case 2:
         gen_bcdcfsq(ctx);
         break;
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] target-ppc: Implement bcdcpsgn. instruction

[Jose Ricardo Ziviani]

bcdcpsgn.: Decimal copy sign. Given two registers vra and vrb, it
copies the vra value with vrb sign to the result register vrt.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 target-ppc/helper.h                 |  1 +
 target-ppc/int_helper.c             | 30 ++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  3 +++
 target-ppc/translate/vmx-ops.inc.c  |  2 +-
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 503f257..dada48e 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -384,6 +384,7 @@ 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_3(bcdctsq, i32, avr, avr, i32)
+DEF_HELPER_4(bcdcpsgn, i32, avr, 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 1025438..a215bfe 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2961,6 +2961,36 @@ uint32_t helper_bcdctsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
     return cr;
 }
 
+uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
+{
+    int i;
+    int cr = 0;
+    int sgna = bcd_get_sgn(a);
+    int sgnb = bcd_get_sgn(b);
+    int invalid = (sgna == 0) || (sgnb == 0);
+    ppc_avr_t ret = { .u64 = { 0, 0 } };
+
+    for (i = 1; i < 32; i++) {
+        bcd_put_digit(&ret, bcd_get_digit(a, i, &invalid), i);
+        bcd_get_digit(b, i, &invalid);
+
+        if (unlikely(invalid)) {
+            break;
+        }
+    }
+    bcd_put_digit(&ret, bcd_get_digit(b, 0, &invalid), 0);
+
+    cr = bcd_cmp_zero(a);
+
+    if (unlikely(invalid)) {
+        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 1579b58..c14b666 100644
--- a/target-ppc/translate/vmx-impl.inc.c
+++ b/target-ppc/translate/vmx-impl.inc.c
@@ -991,6 +991,7 @@ GEN_BCD2(bcdcfz)
 GEN_BCD2(bcdctz)
 GEN_BCD2(bcdcfsq)
 GEN_BCD2(bcdctsq)
+GEN_BCD(bcdcpsgn);
 
 static void gen_xpnd04_1(DisasContext *ctx)
 {
@@ -1056,6 +1057,8 @@ GEN_VXFORM_DUAL(vsubuhm, PPC_ALTIVEC, PPC_NONE, \
                 bcdsub, PPC_NONE, PPC2_ALTIVEC_207)
 GEN_VXFORM_DUAL(vsubuhs, PPC_ALTIVEC, PPC_NONE, \
                 bcdsub, PPC_NONE, PPC2_ALTIVEC_207)
+GEN_VXFORM_DUAL(vaddshs, PPC_ALTIVEC, PPC_NONE, \
+                bcdcpsgn, PPC_NONE, PPC2_ISA300)
 
 static void gen_vsbox(DisasContext *ctx)
 {
diff --git a/target-ppc/translate/vmx-ops.inc.c b/target-ppc/translate/vmx-ops.inc.c
index f02b3be..70d7d2b 100644
--- a/target-ppc/translate/vmx-ops.inc.c
+++ b/target-ppc/translate/vmx-ops.inc.c
@@ -131,7 +131,7 @@ GEN_VXFORM_DUAL(vaddubs, vmul10uq, 0, 8, PPC_ALTIVEC, PPC_NONE),
 GEN_VXFORM_DUAL(vadduhs, vmul10euq, 0, 9, PPC_ALTIVEC, PPC_NONE),
 GEN_VXFORM(vadduws, 0, 10),
 GEN_VXFORM(vaddsbs, 0, 12),
-GEN_VXFORM(vaddshs, 0, 13),
+GEN_VXFORM_DUAL(vaddshs, bcdcpsgn, 0, 13, PPC_ALTIVEC, PPC_NONE),
 GEN_VXFORM(vaddsws, 0, 14),
 GEN_VXFORM_DUAL(vsububs, bcdadd, 0, 24, PPC_ALTIVEC, PPC_NONE),
 GEN_VXFORM_DUAL(vsubuhs, bcdsub, 0, 25, PPC_ALTIVEC, PPC_NONE),
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] target-ppc: Implement bcdsetsgn. instruction

[Jose Ricardo Ziviani]

bcdsetsgn.: Decimal set sign. This instruction copies the register
value to the result register but adjust the signal according to
the preferred sign value.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 target-ppc/helper.h                 | 1 +
 target-ppc/int_helper.c             | 9 +++++++++
 target-ppc/translate/vmx-impl.inc.c | 8 ++++++++
 3 files changed, 18 insertions(+)

diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index dada48e..cddac8e 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -385,6 +385,7 @@ DEF_HELPER_3(bcdctz, i32, avr, avr, i32)
 DEF_HELPER_3(bcdcfsq, i32, avr, avr, i32)
 DEF_HELPER_3(bcdctsq, i32, avr, avr, i32)
 DEF_HELPER_4(bcdcpsgn, i32, avr, avr, avr, i32)
+DEF_HELPER_3(bcdsetsgn, 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 a215bfe..38af503 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2991,6 +2991,15 @@ uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
     return cr;
 }
 
+uint32_t helper_bcdsetsgn(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
+{
+    int sgnb = bcd_get_sgn(b);
+    ppc_avr_t ret = { .u64 = { 0, 0 } };
+
+    bcd_put_digit(&ret, bcd_preferred_sgn(sgnb, ps), 0);
+    return helper_bcdcpsgn(r, b, &ret, ps);
+}
+
 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 c14b666..b188e60 100644
--- a/target-ppc/translate/vmx-impl.inc.c
+++ b/target-ppc/translate/vmx-impl.inc.c
@@ -991,6 +991,7 @@ GEN_BCD2(bcdcfz)
 GEN_BCD2(bcdctz)
 GEN_BCD2(bcdcfsq)
 GEN_BCD2(bcdctsq)
+GEN_BCD2(bcdsetsgn)
 GEN_BCD(bcdcpsgn);
 
 static void gen_xpnd04_1(DisasContext *ctx)
@@ -1014,6 +1015,9 @@ static void gen_xpnd04_1(DisasContext *ctx)
     case 7:
         gen_bcdcfn(ctx);
         break;
+    case 31:
+        gen_bcdsetsgn(ctx);
+        break;
     default:
         gen_invalid(ctx);
         break;
@@ -1038,12 +1042,16 @@ static void gen_xpnd04_2(DisasContext *ctx)
     case 7:
         gen_bcdcfn(ctx);
         break;
+    case 31:
+        gen_bcdsetsgn(ctx);
+        break;
     default:
         gen_invalid(ctx);
         break;
     }
 }
 
+
 GEN_VXFORM_DUAL(vsubcuw, PPC_ALTIVEC, PPC_NONE, \
                 xpnd04_1, PPC_NONE, PPC2_ISA300)
 GEN_VXFORM_DUAL(vsubsws, PPC_ALTIVEC, PPC_NONE, \
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] target-ppc: Implement bcdcfsq. instruction

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4635 bytes --]

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 <joserz@linux.vnet.ibm.com>
> ---
>  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.

> +    }
> +
> +    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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/4] target-ppc: Implement bcdctsq. instruction

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]

On Wed, Nov 16, 2016 at 06:07:28PM -0200, Jose Ricardo Ziviani wrote:
> bcdctsq.: Decimal convert to signed quadword. It is possible to
> convert packed decimal values to signed quadwords.
> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 39 +++++++++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  7 +++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 87f533c..503f257 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -383,6 +383,7 @@ 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_3(bcdctsq, 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 db65a51..1025438 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2922,6 +2922,45 @@ uint32_t helper_bcdcfsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
>      return cr;
>  }
>  
> +uint32_t helper_bcdctsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
> +{
> +    uint8_t i;
> +    int cr = 0;
> +    uint64_t hi = 0;
> +    int sgnb = bcd_get_sgn(b);
> +    int invalid = (sgnb == 0);
> +    ppc_avr_t ret = { .u64 = { 0, 0 } };
> +
> +    ret.u64[LO_IDX] = bcd_get_digit(b, 31, &invalid);
> +    for (i = 30; i > 0; i--) {
> +        mulu64(&ret.u64[LO_IDX], &hi,
> +                ret.u64[LO_IDX], 10ULL);
> +
> +        ret.u64[HI_IDX] = (ret.u64[HI_IDX]) ? ret.u64[HI_IDX] * 10 + hi : hi;
> +        ret.u64[LO_IDX] += bcd_get_digit(b, i, &invalid);

Again, it might be simpler to use the int128 code we already have in qemu.

> +        if (unlikely(invalid)) {
> +            break;
> +        }
> +    }
> +
> +    if (sgnb == -1) {
> +        if (ret.s64[HI_IDX] > 0) {
> +            ret.s64[HI_IDX] = -ret.s64[HI_IDX];
> +        } else {
> +            ret.s64[LO_IDX] = -ret.s64[LO_IDX];
> +        }

As on the other direction, I don't think this looks like a correct
128-bit negate.

> +    }
> +
> +    cr = bcd_cmp_zero(b);
> +
> +    if (unlikely(invalid)) {
> +        cr = 1 << CRF_SO;
> +    }
> +
> +    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 36141e5..1579b58 100644
> --- a/target-ppc/translate/vmx-impl.inc.c
> +++ b/target-ppc/translate/vmx-impl.inc.c
> @@ -990,10 +990,14 @@ GEN_BCD2(bcdctn)
>  GEN_BCD2(bcdcfz)
>  GEN_BCD2(bcdctz)
>  GEN_BCD2(bcdcfsq)
> +GEN_BCD2(bcdctsq)
>  
>  static void gen_xpnd04_1(DisasContext *ctx)
>  {
>      switch (opc4(ctx->opcode)) {
> +    case 0:
> +        gen_bcdctsq(ctx);
> +        break;
>      case 2:
>          gen_bcdcfsq(ctx);
>          break;
> @@ -1018,6 +1022,9 @@ static void gen_xpnd04_1(DisasContext *ctx)
>  static void gen_xpnd04_2(DisasContext *ctx)
>  {
>      switch (opc4(ctx->opcode)) {
> +    case 0:
> +        gen_bcdctsq(ctx);
> +        break;
>      case 2:
>          gen_bcdcfsq(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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/4] target-ppc: Implement bcdcpsgn. instruction

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]

On Wed, Nov 16, 2016 at 06:07:29PM -0200, Jose Ricardo Ziviani wrote:
> bcdcpsgn.: Decimal copy sign. Given two registers vra and vrb, it
> copies the vra value with vrb sign to the result register vrt.
> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 30 ++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  3 +++
>  target-ppc/translate/vmx-ops.inc.c  |  2 +-
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 503f257..dada48e 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -384,6 +384,7 @@ 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_3(bcdctsq, i32, avr, avr, i32)
> +DEF_HELPER_4(bcdcpsgn, i32, avr, 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 1025438..a215bfe 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2961,6 +2961,36 @@ uint32_t helper_bcdctsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
>      return cr;
>  }
>  
> +uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
> +{
> +    int i;
> +    int cr = 0;
> +    int sgna = bcd_get_sgn(a);
> +    int sgnb = bcd_get_sgn(b);
> +    int invalid = (sgna == 0) || (sgnb == 0);
> +    ppc_avr_t ret = { .u64 = { 0, 0 } };
> +
> +    for (i = 1; i < 32; i++) {
> +        bcd_put_digit(&ret, bcd_get_digit(a, i, &invalid), i);
> +        bcd_get_digit(b, i, &invalid);

This is a lot of bit fiddling to accomplish what you could pretty much
do just by copying the entire register.  Checking for invalid input
makes it a bit more complex than that, of course, but you still don't
need to assemble every digit separately in the target.

> +
> +        if (unlikely(invalid)) {
> +            break;
> +        }
> +    }
> +    bcd_put_digit(&ret, bcd_get_digit(b, 0, &invalid), 0);

This won't work.  bcd_get_digit() will set invalid if digit 0 is an
invalid *digit*.  But some valid sign indicators are not valid digits,
so this will erroneously set invalid in those cases.

> +
> +    cr = bcd_cmp_zero(a);
> +
> +    if (unlikely(invalid)) {
> +        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 1579b58..c14b666 100644
> --- a/target-ppc/translate/vmx-impl.inc.c
> +++ b/target-ppc/translate/vmx-impl.inc.c
> @@ -991,6 +991,7 @@ GEN_BCD2(bcdcfz)
>  GEN_BCD2(bcdctz)
>  GEN_BCD2(bcdcfsq)
>  GEN_BCD2(bcdctsq)
> +GEN_BCD(bcdcpsgn);
>  
>  static void gen_xpnd04_1(DisasContext *ctx)
>  {
> @@ -1056,6 +1057,8 @@ GEN_VXFORM_DUAL(vsubuhm, PPC_ALTIVEC, PPC_NONE, \
>                  bcdsub, PPC_NONE, PPC2_ALTIVEC_207)
>  GEN_VXFORM_DUAL(vsubuhs, PPC_ALTIVEC, PPC_NONE, \
>                  bcdsub, PPC_NONE, PPC2_ALTIVEC_207)
> +GEN_VXFORM_DUAL(vaddshs, PPC_ALTIVEC, PPC_NONE, \
> +                bcdcpsgn, PPC_NONE, PPC2_ISA300)
>  
>  static void gen_vsbox(DisasContext *ctx)
>  {
> diff --git a/target-ppc/translate/vmx-ops.inc.c b/target-ppc/translate/vmx-ops.inc.c
> index f02b3be..70d7d2b 100644
> --- a/target-ppc/translate/vmx-ops.inc.c
> +++ b/target-ppc/translate/vmx-ops.inc.c
> @@ -131,7 +131,7 @@ GEN_VXFORM_DUAL(vaddubs, vmul10uq, 0, 8, PPC_ALTIVEC, PPC_NONE),
>  GEN_VXFORM_DUAL(vadduhs, vmul10euq, 0, 9, PPC_ALTIVEC, PPC_NONE),
>  GEN_VXFORM(vadduws, 0, 10),
>  GEN_VXFORM(vaddsbs, 0, 12),
> -GEN_VXFORM(vaddshs, 0, 13),
> +GEN_VXFORM_DUAL(vaddshs, bcdcpsgn, 0, 13, PPC_ALTIVEC, PPC_NONE),
>  GEN_VXFORM(vaddsws, 0, 14),
>  GEN_VXFORM_DUAL(vsububs, bcdadd, 0, 24, PPC_ALTIVEC, PPC_NONE),
>  GEN_VXFORM_DUAL(vsubuhs, bcdsub, 0, 25, PPC_ALTIVEC, PPC_NONE),

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 4/4] target-ppc: Implement bcdsetsgn. instruction

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

On Wed, Nov 16, 2016 at 06:07:30PM -0200, Jose Ricardo Ziviani wrote:
> bcdsetsgn.: Decimal set sign. This instruction copies the register
> value to the result register but adjust the signal according to
> the preferred sign value.
> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 | 1 +
>  target-ppc/int_helper.c             | 9 +++++++++
>  target-ppc/translate/vmx-impl.inc.c | 8 ++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index dada48e..cddac8e 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -385,6 +385,7 @@ DEF_HELPER_3(bcdctz, i32, avr, avr, i32)
>  DEF_HELPER_3(bcdcfsq, i32, avr, avr, i32)
>  DEF_HELPER_3(bcdctsq, i32, avr, avr, i32)
>  DEF_HELPER_4(bcdcpsgn, i32, avr, avr, avr, i32)
> +DEF_HELPER_3(bcdsetsgn, 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 a215bfe..38af503 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2991,6 +2991,15 @@ uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
>      return cr;
>  }
>  
> +uint32_t helper_bcdsetsgn(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
> +{
> +    int sgnb = bcd_get_sgn(b);
> +    ppc_avr_t ret = { .u64 = { 0, 0 } };
> +
> +    bcd_put_digit(&ret, bcd_preferred_sgn(sgnb, ps), 0);
> +    return helper_bcdcpsgn(r, b, &ret, ps);

This is doing a lot of work just to canonicalize the sign indicator.

> +}
> +
>  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 c14b666..b188e60 100644
> --- a/target-ppc/translate/vmx-impl.inc.c
> +++ b/target-ppc/translate/vmx-impl.inc.c
> @@ -991,6 +991,7 @@ GEN_BCD2(bcdcfz)
>  GEN_BCD2(bcdctz)
>  GEN_BCD2(bcdcfsq)
>  GEN_BCD2(bcdctsq)
> +GEN_BCD2(bcdsetsgn)
>  GEN_BCD(bcdcpsgn);
>  
>  static void gen_xpnd04_1(DisasContext *ctx)
> @@ -1014,6 +1015,9 @@ static void gen_xpnd04_1(DisasContext *ctx)
>      case 7:
>          gen_bcdcfn(ctx);
>          break;
> +    case 31:
> +        gen_bcdsetsgn(ctx);
> +        break;
>      default:
>          gen_invalid(ctx);
>          break;
> @@ -1038,12 +1042,16 @@ static void gen_xpnd04_2(DisasContext *ctx)
>      case 7:
>          gen_bcdcfn(ctx);
>          break;
> +    case 31:
> +        gen_bcdsetsgn(ctx);
> +        break;
>      default:
>          gen_invalid(ctx);
>          break;
>      }
>  }
>  
> +
>  GEN_VXFORM_DUAL(vsubcuw, PPC_ALTIVEC, PPC_NONE, \
>                  xpnd04_1, PPC_NONE, PPC2_ISA300)
>  GEN_VXFORM_DUAL(vsubsws, PPC_ALTIVEC, PPC_NONE, \

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/4] target-ppc: Implement bcdcfsq. instruction

[joserz]

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 <joserz@linux.vnet.ibm.com>
> > ---
> >  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

Re: [Qemu-devel] [PATCH 1/4] target-ppc: Implement bcdcfsq. instruction

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6738 bytes --]

On Thu, Nov 17, 2016 at 03:31:48PM -0200, joserz@linux.vnet.ibm.com wrote:
> 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 <joserz@linux.vnet.ibm.com>
> > > ---
> > >  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);

*thinks*  no.. it's simpler than that.

You can use divu128() to divide your input by 10^15, and it gives you
both the dividend and the remainder.  The dividend gives you the value
you need in the upper BCD word, and the remainder gives you the value
needed in the lower BCD word.  If the dividend doesn't fit in 64-bits,
you'ver overflowed (bceause the original value was > 2^64*10^15 which
is greater than 10^31)

Then you can use regular 64-bit arithmetic to split up those two
halves into individual digits.

> But, anyway, these functions would have to implement those hi/lo
> multiplications for the case "!CONFIG_INT128".

They are already implemented for !CONFIG_INT128 - see the #else case
and util/host-utils.c

> 
> 
> > > +    }
> > > +
> > > +    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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]