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

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

[Jose Ricardo Ziviani]

v2:
 - use div128 and mul64 functions to make code easier to understand
 - fixed int128 neg
 - improved functions bcdcpsgn and bcdsetsgn to do less work
   than necessary
 - rebased on ppc-for-2.9

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             | 127 ++++++++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  25 +++++++
 target-ppc/translate/vmx-ops.inc.c  |   2 +-
 4 files changed, 157 insertions(+), 1 deletion(-)

-- 
2.7.4

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

[Jose Ricardo Ziviani]

bcdcfsq.: Decimal convert from signed quadword. It is not 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             | 45 +++++++++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  7 ++++++
 3 files changed, 53 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 8886a72..751909c 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2874,6 +2874,51 @@ 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 cr;
+    int i;
+    int ox_flag = 0;
+    uint64_t lo_value;
+    uint64_t hi_value;
+    uint64_t max = 0x38d7ea4c68000;
+    ppc_avr_t ret = { .u64 = { 0, 0 } };
+
+    if (b->s64[HI_IDX] < 0) {
+        lo_value = -b->s64[LO_IDX];
+        hi_value = ~b->u64[HI_IDX] + !lo_value;
+        bcd_put_digit(&ret, 0xD, 0);
+    } else {
+        lo_value = b->u64[LO_IDX];
+        hi_value = b->u64[HI_IDX];
+        bcd_put_digit(&ret, bcd_preferred_sgn(0, ps), 0);
+    }
+
+    if (divu128(&lo_value, &hi_value, max)) {
+        ox_flag = 1;
+    } else if (lo_value >= max && hi_value == 0) {
+        ox_flag = 1;
+    }
+
+    for (i = 1; hi_value; hi_value /= 10, i++) {
+        bcd_put_digit(&ret, hi_value % 10, i);
+    }
+
+    for (; lo_value; lo_value /= 10, i++) {
+        bcd_put_digit(&ret, lo_value % 10, i);
+    }
+
+    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 v2 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             | 40 +++++++++++++++++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  7 +++++++
 3 files changed, 48 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 751909c..ca0d0b8 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2919,6 +2919,46 @@ 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;
+    uint64_t carry;
+    uint64_t unused;
+    uint64_t lo_value;
+    uint64_t hi_value = 0;
+    int sgnb = bcd_get_sgn(b);
+    int invalid = (sgnb == 0);
+
+    lo_value = bcd_get_digit(b, 31, &invalid);
+    for (i = 30; i > 0; i--) {
+        mulu64(&lo_value, &carry, lo_value, 10ULL);
+        mulu64(&hi_value, &unused, hi_value, 10ULL);
+        lo_value += bcd_get_digit(b, i, &invalid);
+        hi_value += carry;
+
+        if (unlikely(invalid)) {
+            break;
+        }
+    }
+
+    if (sgnb == -1) {
+        r->s64[LO_IDX] = -lo_value;
+        r->s64[HI_IDX] = ~hi_value + !r->s64[LO_IDX];
+    } else {
+        r->s64[LO_IDX] = lo_value;
+        r->s64[HI_IDX] = hi_value;
+    }
+
+    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 v2 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             | 23 +++++++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  3 +++
 target-ppc/translate/vmx-ops.inc.c  |  2 +-
 4 files changed, 28 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 ca0d0b8..833c9d2 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2959,6 +2959,29 @@ 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 invalid = 0;
+
+    if (bcd_get_sgn(a) == 0 || bcd_get_sgn(b) == 0) {
+        return 1 << CRF_SO;
+    }
+
+    *r = *a;
+    bcd_put_digit(r, b->u8[BCD_DIG_BYTE(0)] & 0xF, 0);
+
+    for (i = 1; i < 32; i++) {
+        bcd_get_digit(a, i, &invalid);
+        bcd_get_digit(b, i, &invalid);
+        if (unlikely(invalid)) {
+            return 1 << CRF_SO;
+        }
+    }
+
+    return bcd_cmp_zero(r);
+}
+
 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 v2 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             | 19 +++++++++++++++++++
 target-ppc/translate/vmx-impl.inc.c |  8 ++++++++
 3 files changed, 28 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 833c9d2..db430ef 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -2982,6 +2982,25 @@ uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
     return bcd_cmp_zero(r);
 }
 
+uint32_t helper_bcdsetsgn(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
+{
+    int i;
+    int invalid = 0;
+    int sgnb = bcd_get_sgn(b);
+
+    *r = *b;
+    bcd_put_digit(r, bcd_preferred_sgn(sgnb, ps), 0);
+
+    for (i = 1; i < 32; i++) {
+        bcd_get_digit(b, i, &invalid);
+        if (unlikely(invalid)) {
+            return 1 << CRF_SO;
+        }
+    }
+
+    return bcd_cmp_zero(r);
+}
+
 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 v2 1/4] target-ppc: Implement bcdcfsq. instruction

[Richard Henderson]

On 11/23/2016 05:21 PM, Jose Ricardo Ziviani wrote:
> bcdcfsq.: Decimal convert from signed quadword. It is not 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             | 45 +++++++++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  7 ++++++
>  3 files changed, 53 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 8886a72..751909c 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2874,6 +2874,51 @@ 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 cr;
> +    int i;
> +    int ox_flag = 0;
> +    uint64_t lo_value;
> +    uint64_t hi_value;
> +    uint64_t max = 0x38d7ea4c68000;

This is at heart a decimal number, and should be written as such.
Also, you need ULL for a 32-bit host compile.

> +    if (divu128(&lo_value, &hi_value, max)) {
> +        ox_flag = 1;
> +    } else if (lo_value >= max && hi_value == 0) {
> +        ox_flag = 1;
> +    }

Dispense with ox_flag and set cr = CRF_SO now.

> +    for (i = 1; hi_value; hi_value /= 10, i++) {
> +        bcd_put_digit(&ret, hi_value % 10, i);
> +    }
> +
> +    for (; lo_value; lo_value /= 10, i++) {
> +        bcd_put_digit(&ret, lo_value % 10, i);
> +    }

How can this possibly work?  You know there are 15 digits between high and low, 
but you continue with i++?

If hi_value == 1 && lo_value == 1, this should not produce 11, but 
10000000000000001.


r~

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

[David Gibson]

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

On Wed, Nov 23, 2016 at 02:21:42PM -0200, Jose Ricardo Ziviani wrote:
> bcdcfsq.: Decimal convert from signed quadword. It is not possible
> to convert values less than 10^31-1 or greater than -10^31-1 to be
> represented in packed decimal format.

You have your less than / greater than the wrong way around in the
above.

> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 45 +++++++++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  7 ++++++
>  3 files changed, 53 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 8886a72..751909c 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2874,6 +2874,51 @@ 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 cr;
> +    int i;
> +    int ox_flag = 0;
> +    uint64_t lo_value;
> +    uint64_t hi_value;
> +    uint64_t max = 0x38d7ea4c68000;

In this case it would be clearer what's going on if you gave this
constant in decimal. "max" is also not a great name - see below.

> +    ppc_avr_t ret = { .u64 = { 0, 0 } };
> +
> +    if (b->s64[HI_IDX] < 0) {
> +        lo_value = -b->s64[LO_IDX];
> +        hi_value = ~b->u64[HI_IDX] + !lo_value;
> +        bcd_put_digit(&ret, 0xD, 0);
> +    } else {
> +        lo_value = b->u64[LO_IDX];
> +        hi_value = b->u64[HI_IDX];
> +        bcd_put_digit(&ret, bcd_preferred_sgn(0, ps), 0);
> +    }
> +
> +    if (divu128(&lo_value, &hi_value, max)) {
> +        ox_flag = 1;
> +    } else if (lo_value >= max && hi_value == 0) {

This isn't right.  max == 10^15, but in fact the dividend can safely
be up to 10^16 - 1.  I don't see what checking the remainder against 0
has to do with anything, either.  No overflow + (dividend < 10^16)
should be sufficient.

> +        ox_flag = 1;
> +    }
> +
> +    for (i = 1; hi_value; hi_value /= 10, i++) {
> +        bcd_put_digit(&ret, hi_value % 10, i);
> +    }
> +
> +    for (; lo_value; lo_value /= 10, i++) {
> +        bcd_put_digit(&ret, lo_value % 10, i);
> +    }
> +
> +    cr = bcd_cmp_zero(&ret);
> +
> +    if (unlikely(ox_flag)) {
> +        cr |= 1 << CRF_SO;

Since you posted I've merged a patch from Nikunj which chanes the
meaning of these CRF_* flags to be bit masks instead of shifts.  So
this will need to become just
	cr |= 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 v2 1/4] target-ppc: Implement bcdcfsq. instruction

[David Gibson]

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

On Thu, Nov 24, 2016 at 01:43:18AM +0100, Richard Henderson wrote:
> On 11/23/2016 05:21 PM, Jose Ricardo Ziviani wrote:
> > bcdcfsq.: Decimal convert from signed quadword. It is not 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             | 45 +++++++++++++++++++++++++++++++++++++
> >  target-ppc/translate/vmx-impl.inc.c |  7 ++++++
> >  3 files changed, 53 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 8886a72..751909c 100644
> > --- a/target-ppc/int_helper.c
> > +++ b/target-ppc/int_helper.c
> > @@ -2874,6 +2874,51 @@ 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 cr;
> > +    int i;
> > +    int ox_flag = 0;
> > +    uint64_t lo_value;
> > +    uint64_t hi_value;
> > +    uint64_t max = 0x38d7ea4c68000;
> 
> This is at heart a decimal number, and should be written as such.
> Also, you need ULL for a 32-bit host compile.
> 
> > +    if (divu128(&lo_value, &hi_value, max)) {
> > +        ox_flag = 1;
> > +    } else if (lo_value >= max && hi_value == 0) {
> > +        ox_flag = 1;
> > +    }
> 
> Dispense with ox_flag and set cr = CRF_SO now.
> 
> > +    for (i = 1; hi_value; hi_value /= 10, i++) {
> > +        bcd_put_digit(&ret, hi_value % 10, i);
> > +    }
> > +
> > +    for (; lo_value; lo_value /= 10, i++) {
> > +        bcd_put_digit(&ret, lo_value % 10, i);
> > +    }
> 
> How can this possibly work?  You know there are 15 digits between high and
> low, but you continue with i++?
>
> If hi_value == 1 && lo_value == 1, this should not produce 11, but
> 10000000000000001.

Ah, yes good catch, I missed that.  I think it will be clearer to use
fixed bounds on each loop, rather than testing the value of the
remaining result.

-- 
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 v2 2/4] target-ppc: Implement bcdctsq. instruction

[David Gibson]

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

On Wed, Nov 23, 2016 at 02:21:43PM -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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 40 +++++++++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  7 +++++++
>  3 files changed, 48 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 751909c..ca0d0b8 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2919,6 +2919,46 @@ 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;
> +    uint64_t carry;
> +    uint64_t unused;
> +    uint64_t lo_value;
> +    uint64_t hi_value = 0;
> +    int sgnb = bcd_get_sgn(b);
> +    int invalid = (sgnb == 0);
> +
> +    lo_value = bcd_get_digit(b, 31, &invalid);
> +    for (i = 30; i > 0; i--) {
> +        mulu64(&lo_value, &carry, lo_value, 10ULL);
> +        mulu64(&hi_value, &unused, hi_value, 10ULL);
> +        lo_value += bcd_get_digit(b, i, &invalid);
> +        hi_value += carry;
> +
> +        if (unlikely(invalid)) {
> +            break;
> +        }
> +    }
> +
> +    if (sgnb == -1) {
> +        r->s64[LO_IDX] = -lo_value;
> +        r->s64[HI_IDX] = ~hi_value + !r->s64[LO_IDX];
> +    } else {
> +        r->s64[LO_IDX] = lo_value;
> +        r->s64[HI_IDX] = hi_value;
> +    }
> +
> +    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 v2 3/4] target-ppc: Implement bcdcpsgn. instruction

[David Gibson]

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

On Wed, Nov 23, 2016 at 02:21:44PM -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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 23 +++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  3 +++
>  target-ppc/translate/vmx-ops.inc.c  |  2 +-
>  4 files changed, 28 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 ca0d0b8..833c9d2 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2959,6 +2959,29 @@ 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 invalid = 0;
> +
> +    if (bcd_get_sgn(a) == 0 || bcd_get_sgn(b) == 0) {
> +        return 1 << CRF_SO;
> +    }
> +
> +    *r = *a;
> +    bcd_put_digit(r, b->u8[BCD_DIG_BYTE(0)] & 0xF, 0);
> +
> +    for (i = 1; i < 32; i++) {
> +        bcd_get_digit(a, i, &invalid);
> +        bcd_get_digit(b, i, &invalid);
> +        if (unlikely(invalid)) {
> +            return 1 << CRF_SO;
> +        }
> +    }
> +
> +    return bcd_cmp_zero(r);
> +}
> +
>  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 v2 4/4] target-ppc: Implement bcdsetsgn. instruction

[David Gibson]

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

On Wed, Nov 23, 2016 at 02:21:45PM -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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target-ppc/helper.h                 |  1 +
>  target-ppc/int_helper.c             | 19 +++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  8 ++++++++
>  3 files changed, 28 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 833c9d2..db430ef 100644
> --- a/target-ppc/int_helper.c
> +++ b/target-ppc/int_helper.c
> @@ -2982,6 +2982,25 @@ uint32_t helper_bcdcpsgn(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, uint32_t ps)
>      return bcd_cmp_zero(r);
>  }
>  
> +uint32_t helper_bcdsetsgn(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
> +{
> +    int i;
> +    int invalid = 0;
> +    int sgnb = bcd_get_sgn(b);
> +
> +    *r = *b;
> +    bcd_put_digit(r, bcd_preferred_sgn(sgnb, ps), 0);
> +
> +    for (i = 1; i < 32; i++) {
> +        bcd_get_digit(b, i, &invalid);
> +        if (unlikely(invalid)) {
> +            return 1 << CRF_SO;
> +        }
> +    }
> +
> +    return bcd_cmp_zero(r);
> +}
> +
>  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 v2 0/4] POWER9 TCG enablements - BCD functions part II

[David Gibson]

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

On Wed, Nov 23, 2016 at 02:21:41PM -0200, Jose Ricardo Ziviani wrote:
> v2:
>  - use div128 and mul64 functions to make code easier to understand
>  - fixed int128 neg
>  - improved functions bcdcpsgn and bcdsetsgn to do less work
>    than necessary
>  - rebased on ppc-for-2.9
> 
> 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

Patch 1/4 has some problems, see comments.

Patches 2..4/4 look ok - except that they'll need to be updated for
the recent change I merged from Nikunj (in ppc-for-2.9) which changes
the meaning of CRF_*.

> 
> 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             | 127 ++++++++++++++++++++++++++++++++++++
>  target-ppc/translate/vmx-impl.inc.c |  25 +++++++
>  target-ppc/translate/vmx-ops.inc.c  |   2 +-
>  4 files changed, 157 insertions(+), 1 deletion(-)
> 

-- 
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] [Qemu-ppc] [PATCH v2 1/4] target-ppc: Implement bcdcfsq. instruction

[joserz]

Hello Richard,

Thank you for your review, please read my answer below.


On Thu, Nov 24, 2016 at 01:43:18AM +0100, Richard Henderson wrote:
> On 11/23/2016 05:21 PM, Jose Ricardo Ziviani wrote:
> >bcdcfsq.: Decimal convert from signed quadword. It is not 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             | 45 +++++++++++++++++++++++++++++++++++++
> > target-ppc/translate/vmx-impl.inc.c |  7 ++++++
> > 3 files changed, 53 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 8886a72..751909c 100644
> >--- a/target-ppc/int_helper.c
> >+++ b/target-ppc/int_helper.c
> >@@ -2874,6 +2874,51 @@ 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 cr;
> >+    int i;
> >+    int ox_flag = 0;
> >+    uint64_t lo_value;
> >+    uint64_t hi_value;
> >+    uint64_t max = 0x38d7ea4c68000;
> 
> This is at heart a decimal number, and should be written as such.
> Also, you need ULL for a 32-bit host compile.
>

OK

> >+    if (divu128(&lo_value, &hi_value, max)) {
> >+        ox_flag = 1;
> >+    } else if (lo_value >= max && hi_value == 0) {
> >+        ox_flag = 1;
> >+    }
> 
> Dispense with ox_flag and set cr = CRF_SO now.
> 

OK

> >+    for (i = 1; hi_value; hi_value /= 10, i++) {
> >+        bcd_put_digit(&ret, hi_value % 10, i);
> >+    }
> >+
> >+    for (; lo_value; lo_value /= 10, i++) {
> >+        bcd_put_digit(&ret, lo_value % 10, i);
> >+    }
> 
> How can this possibly work?  You know there are 15 digits between high and
> low, but you continue with i++?
> 
> If hi_value == 1 && lo_value == 1, this should not produce 11, but
> 10000000000000001.
> 

Suppose we have hi_value = lo_value = 1

after divu128 above we will have
hi_value = 744073709551617
lo_value = 18446

then, after the first for loop:
hi_value = 0
lo_value = 18446
i = 16
ret = u128 = 0x8376822234287792511

finally, after the second loop:
hi_value = 0
lo_value = 0
i = 21
ret = u128 = 0x0000000000018446744073709551617f

Which is correct, this function converted a signed quadword to bcd correctly, using two doubleword variables:
1 << 64 | 1 = 18446744073709551617

Am I missing anything?

Thank you!

> 
> r~
> 

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

[joserz]

David,

Thank you again for reviewing my code, I'm making the changes. I only have a question about patch 1/4 which I didn't find the issue so I'm waiting for Richard's answer.

Thanks!


On Thu, Nov 24, 2016 at 12:28:32PM +1100, David Gibson wrote:
> On Wed, Nov 23, 2016 at 02:21:41PM -0200, Jose Ricardo Ziviani wrote:
> > v2:
> >  - use div128 and mul64 functions to make code easier to understand
> >  - fixed int128 neg
> >  - improved functions bcdcpsgn and bcdsetsgn to do less work
> >    than necessary
> >  - rebased on ppc-for-2.9
> > 
> > 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
> 
> Patch 1/4 has some problems, see comments.
> 
> Patches 2..4/4 look ok - except that they'll need to be updated for
> the recent change I merged from Nikunj (in ppc-for-2.9) which changes
> the meaning of CRF_*.
> 
> > 
> > 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             | 127 ++++++++++++++++++++++++++++++++++++
> >  target-ppc/translate/vmx-impl.inc.c |  25 +++++++
> >  target-ppc/translate/vmx-ops.inc.c  |   2 +-
> >  4 files changed, 157 insertions(+), 1 deletion(-)
> > 
> 
> -- 
> 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] [Qemu-ppc] [PATCH v2 1/4] target-ppc: Implement bcdcfsq. instruction

[David Gibson]

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

On Thu, Nov 24, 2016 at 02:31:21PM -0200, joserz@linux.vnet.ibm.com wrote:
> Hello Richard,
> 
> Thank you for your review, please read my answer below.
> 
> 
> On Thu, Nov 24, 2016 at 01:43:18AM +0100, Richard Henderson wrote:
> > On 11/23/2016 05:21 PM, Jose Ricardo Ziviani wrote:
> > >bcdcfsq.: Decimal convert from signed quadword. It is not 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             | 45 +++++++++++++++++++++++++++++++++++++
> > > target-ppc/translate/vmx-impl.inc.c |  7 ++++++
> > > 3 files changed, 53 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 8886a72..751909c 100644
> > >--- a/target-ppc/int_helper.c
> > >+++ b/target-ppc/int_helper.c
> > >@@ -2874,6 +2874,51 @@ 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 cr;
> > >+    int i;
> > >+    int ox_flag = 0;
> > >+    uint64_t lo_value;
> > >+    uint64_t hi_value;
> > >+    uint64_t max = 0x38d7ea4c68000;
> > 
> > This is at heart a decimal number, and should be written as such.
> > Also, you need ULL for a 32-bit host compile.
> >
> 
> OK
> 
> > >+    if (divu128(&lo_value, &hi_value, max)) {
> > >+        ox_flag = 1;
> > >+    } else if (lo_value >= max && hi_value == 0) {
> > >+        ox_flag = 1;
> > >+    }
> > 
> > Dispense with ox_flag and set cr = CRF_SO now.
> > 
> 
> OK
> 
> > >+    for (i = 1; hi_value; hi_value /= 10, i++) {
> > >+        bcd_put_digit(&ret, hi_value % 10, i);
> > >+    }
> > >+
> > >+    for (; lo_value; lo_value /= 10, i++) {
> > >+        bcd_put_digit(&ret, lo_value % 10, i);
> > >+    }
> > 
> > How can this possibly work?  You know there are 15 digits between high and
> > low, but you continue with i++?
> > 
> > If hi_value == 1 && lo_value == 1, this should not produce 11, but
> > 10000000000000001.
> > 
> 
> Suppose we have hi_value = lo_value = 1
> 
> after divu128 above we will have

No, Richard means if you have hi_value == 1 and lo_value == 1 *after*
the divide.  This will happen if input has *decimal* value
1000000000000001.  The point is that 'i' will have the wrong value if
the low output word has any leading zeroes.

> hi_value = 744073709551617
> lo_value = 18446
> 
> then, after the first for loop:
> hi_value = 0
> lo_value = 18446
> i = 16
> ret = u128 = 0x8376822234287792511
> 
> finally, after the second loop:
> hi_value = 0
> lo_value = 0
> i = 21
> ret = u128 = 0x0000000000018446744073709551617f
> 
> Which is correct, this function converted a signed quadword to bcd correctly, using two doubleword variables:
> 1 << 64 | 1 = 18446744073709551617
> 
> Am I missing anything?
> 
> Thank you!
> 
> > 
> > r~
> > 
> 

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