| Message ID | 1479918105-15616-2-git-send-email-joserz@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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~
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;
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.
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~ >
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~ > > >
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;
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(+)