diff mbox

[2/7] target-ppc: Implement unsigned quadword left/right shift and unit tests

Message ID 1480741206-32737-3-git-send-email-joserz@linux.vnet.ibm.com
State New
Headers show

Commit Message

Jose Ricardo Ziviani Dec. 3, 2016, 5 a.m. UTC
This commit implements functions to right and left shifts and the
unittest for them. Such functions is needed due to instructions
that requires them.

Today, there is already a right shift implementation in int128.h
but it's for signed numbers.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
 include/qemu/host-utils.h | 29 ++++++++++++++
 tests/Makefile.include    |  5 ++-
 tests/test-shift128.c     | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 util/host-utils.c         | 38 +++++++++++++++++++
 4 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 tests/test-shift128.c

Comments

Richard Henderson Dec. 4, 2016, 1:37 a.m. UTC | #1
On 12/02/2016 09:00 PM, Jose Ricardo Ziviani wrote:
> +++ b/include/qemu/host-utils.h
> @@ -29,6 +29,33 @@
>  #include "qemu/bswap.h"
>
>  #ifdef CONFIG_INT128
> +static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
> +{
> +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> +    val >>= (shift & 127);
> +    *phigh = val >> 64;
> +    *plow = val & 0xffffffffffffffff;
> +}
> +
> +static inline void ulshift(uint64_t *plow, uint64_t *phigh,
> +                           uint32_t shift, bool *overflow)
> +{
> +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> +
> +    if (shift == 0) {
> +        return;
> +    }
> +
> +    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
> +        *overflow = true;
> +    }
> +
> +    val <<= (shift & 127);
> +
> +    *phigh = val >> 64;
> +    *plow = val & 0xffffffffffffffff;
> +}
> +

This belongs in qemu/int128.h, not here.  And certainly not predicated on 
CONFIG_INT128.


r~
David Gibson Dec. 5, 2016, 1:56 a.m. UTC | #2
On Sat, Dec 03, 2016 at 05:37:27PM -0800, Richard Henderson wrote:
> On 12/02/2016 09:00 PM, Jose Ricardo Ziviani wrote:
> > +++ b/include/qemu/host-utils.h
> > @@ -29,6 +29,33 @@
> >  #include "qemu/bswap.h"
> > 
> >  #ifdef CONFIG_INT128
> > +static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
> > +{
> > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > +    val >>= (shift & 127);
> > +    *phigh = val >> 64;
> > +    *plow = val & 0xffffffffffffffff;
> > +}
> > +
> > +static inline void ulshift(uint64_t *plow, uint64_t *phigh,
> > +                           uint32_t shift, bool *overflow)
> > +{
> > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > +
> > +    if (shift == 0) {
> > +        return;
> > +    }
> > +
> > +    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
> > +        *overflow = true;
> > +    }
> > +
> > +    val <<= (shift & 127);
> > +
> > +    *phigh = val >> 64;
> > +    *plow = val & 0xffffffffffffffff;
> > +}
> > +
> 
> This belongs in qemu/int128.h, not here.  And certainly not predicated on
> CONFIG_INT128.

Is there actually any advantage to the __uint128_t based versions over
the 64-bit versions?
Jose Ricardo Ziviani Dec. 5, 2016, 9:35 a.m. UTC | #3
On Mon, Dec 05, 2016 at 12:56:39PM +1100, David Gibson wrote:
> On Sat, Dec 03, 2016 at 05:37:27PM -0800, Richard Henderson wrote:
> > On 12/02/2016 09:00 PM, Jose Ricardo Ziviani wrote:
> > > +++ b/include/qemu/host-utils.h
> > > @@ -29,6 +29,33 @@
> > >  #include "qemu/bswap.h"
> > > 
> > >  #ifdef CONFIG_INT128
> > > +static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
> > > +{
> > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > +    val >>= (shift & 127);
> > > +    *phigh = val >> 64;
> > > +    *plow = val & 0xffffffffffffffff;
> > > +}
> > > +
> > > +static inline void ulshift(uint64_t *plow, uint64_t *phigh,
> > > +                           uint32_t shift, bool *overflow)
> > > +{
> > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > +
> > > +    if (shift == 0) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
> > > +        *overflow = true;
> > > +    }
> > > +
> > > +    val <<= (shift & 127);
> > > +
> > > +    *phigh = val >> 64;
> > > +    *plow = val & 0xffffffffffffffff;
> > > +}
> > > +
> > 
> > This belongs in qemu/int128.h, not here.  And certainly not predicated on
> > CONFIG_INT128.
> 
> Is there actually any advantage to the __uint128_t based versions over
> the 64-bit versions?

Nothing special here. It just looks more clear (to me) to shift all 128
bits at once than 2x64. But I agree we won't loose to have just one
function outside CONFIG_INT128.

So, I'll remove these two functions and keep only the other two using
uint64_t types.

Anyway I get a bit confused about int128.h and host-utils.h. I see
functions like divu128 and divs128 that could be in int128.h, since
there is no similar operation in int128.h. Is there any rule about it?

Thank you guys for reviewing it!

> 
> -- 
> 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
David Gibson Dec. 5, 2016, 10:59 p.m. UTC | #4
On Mon, Dec 05, 2016 at 07:35:39AM -0200, joserz@linux.vnet.ibm.com wrote:
> On Mon, Dec 05, 2016 at 12:56:39PM +1100, David Gibson wrote:
> > On Sat, Dec 03, 2016 at 05:37:27PM -0800, Richard Henderson wrote:
> > > On 12/02/2016 09:00 PM, Jose Ricardo Ziviani wrote:
> > > > +++ b/include/qemu/host-utils.h
> > > > @@ -29,6 +29,33 @@
> > > >  #include "qemu/bswap.h"
> > > > 
> > > >  #ifdef CONFIG_INT128
> > > > +static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
> > > > +{
> > > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > > +    val >>= (shift & 127);
> > > > +    *phigh = val >> 64;
> > > > +    *plow = val & 0xffffffffffffffff;
> > > > +}
> > > > +
> > > > +static inline void ulshift(uint64_t *plow, uint64_t *phigh,
> > > > +                           uint32_t shift, bool *overflow)
> > > > +{
> > > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > > +
> > > > +    if (shift == 0) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
> > > > +        *overflow = true;
> > > > +    }
> > > > +
> > > > +    val <<= (shift & 127);
> > > > +
> > > > +    *phigh = val >> 64;
> > > > +    *plow = val & 0xffffffffffffffff;
> > > > +}
> > > > +
> > > 
> > > This belongs in qemu/int128.h, not here.  And certainly not predicated on
> > > CONFIG_INT128.
> > 
> > Is there actually any advantage to the __uint128_t based versions over
> > the 64-bit versions?
> 
> Nothing special here. It just looks more clear (to me) to shift all 128
> bits at once than 2x64. But I agree we won't loose to have just one
> function outside CONFIG_INT128.

It is clearer, but having two different version makes things less
clear again.  We need to have the 64-bit only version for compilers
without int128_t support, so..

> So, I'll remove these two functions and keep only the other two using
> uint64_t types.
> 
> Anyway I get a bit confused about int128.h and host-utils.h. I see
> functions like divu128 and divs128 that could be in int128.h, since
> there is no similar operation in int128.h. Is there any rule about it?
> 
> Thank you guys for reviewing it!
> 
> > 
> 
>
diff mbox

Patch

diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
index 46187bb..b3e5f72 100644
--- a/include/qemu/host-utils.h
+++ b/include/qemu/host-utils.h
@@ -29,6 +29,33 @@ 
 #include "qemu/bswap.h"
 
 #ifdef CONFIG_INT128
+static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
+{
+    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
+    val >>= (shift & 127);
+    *phigh = val >> 64;
+    *plow = val & 0xffffffffffffffff;
+}
+
+static inline void ulshift(uint64_t *plow, uint64_t *phigh,
+                           uint32_t shift, bool *overflow)
+{
+    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
+
+    if (shift == 0) {
+        return;
+    }
+
+    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
+        *overflow = true;
+    }
+
+    val <<= (shift & 127);
+
+    *phigh = val >> 64;
+    *plow = val & 0xffffffffffffffff;
+}
+
 static inline void mulu64(uint64_t *plow, uint64_t *phigh,
                           uint64_t a, uint64_t b)
 {
@@ -81,6 +108,8 @@  void muls64(uint64_t *phigh, uint64_t *plow, int64_t a, int64_t b);
 void mulu64(uint64_t *phigh, uint64_t *plow, uint64_t a, uint64_t b);
 int divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
 int divs128(int64_t *plow, int64_t *phigh, int64_t divisor);
+void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift);
+void ulshift(uint64_t *plow, uint64_t *phigh, uint32_t shift, bool *overflow);
 
 static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 {
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e98d3b6..89e5e85 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -65,6 +65,8 @@  check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF)
 endif
 check-unit-y += tests/test-cutils$(EXESUF)
 gcov-files-test-cutils-y += util/cutils.c
+check-unit-y += tests/test-shift128$(EXESUF)
+gcov-files-test-shift128-y = util/host-utils.c
 check-unit-y += tests/test-mul64$(EXESUF)
 gcov-files-test-mul64-y = util/host-utils.c
 check-unit-y += tests/test-int128$(EXESUF)
@@ -460,7 +462,7 @@  test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
 	tests/test-opts-visitor.o tests/test-qmp-event.o \
 	tests/rcutorture.o tests/test-rcu-list.o \
-	tests/test-qdist.o \
+	tests/test-qdist.o tests/test-shift128.o \
 	tests/test-qht.o tests/qht-bench.o tests/test-qht-par.o \
 	tests/atomic_add-bench.o
 
@@ -568,6 +570,7 @@  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marsh
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
 tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
 
+tests/test-shift128$(EXESUF): tests/test-shift128.o $(test-util-obj-y)
 tests/test-mul64$(EXESUF): tests/test-mul64.o $(test-util-obj-y)
 tests/test-bitops$(EXESUF): tests/test-bitops.o $(test-util-obj-y)
 tests/test-crypto-hash$(EXESUF): tests/test-crypto-hash.o $(test-crypto-obj-y)
diff --git a/tests/test-shift128.c b/tests/test-shift128.c
new file mode 100644
index 0000000..67615c9
--- /dev/null
+++ b/tests/test-shift128.c
@@ -0,0 +1,97 @@ 
+/*
+ * Test unsigned left and right shift
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+
+typedef struct {
+    uint64_t low;
+    uint64_t high;
+    uint64_t rlow;
+    uint64_t rhigh;
+    int32_t shift;
+    bool overflow;
+} test_data;
+
+static const test_data test_ltable[] = {
+    { 1223ULL, 0, 1223ULL,   0, 0, false },
+    { 1ULL,    0, 2ULL,   0, 1, false },
+    { 1ULL,    0, 4ULL,   0, 2, false },
+    { 1ULL,    0, 16ULL,  0, 4, false },
+    { 1ULL,    0, 256ULL, 0, 8, false },
+    { 1ULL,    0, 65536ULL, 0, 16, false },
+    { 1ULL,    0, 2147483648ULL, 0, 31, false },
+    { 1ULL,    0, 35184372088832ULL, 0, 45, false },
+    { 1ULL,    0, 1152921504606846976ULL, 0, 60, false },
+    { 1ULL,    0, 0, 1ULL, 64, false },
+    { 1ULL,    0, 0, 65536ULL, 80, false },
+    { 1ULL,    0, 0, 9223372036854775808ULL, 127, false },
+    { 0ULL,    1, 0, 0, 64, true },
+    { 0x8888888888888888ULL, 0x9999999999999999ULL,
+        0x8000000000000000ULL, 0x9888888888888888ULL, 60, true },
+    { 0x8888888888888888ULL, 0x9999999999999999ULL,
+        0, 0x8888888888888888ULL, 64, true },
+    { 0x8ULL, 0, 0, 0x8ULL, 64, false },
+    { 0x8ULL, 0, 0, 0x8000000000000000ULL, 124, false },
+    { 0x1ULL, 0, 0, 0x4000000000000000ULL, 126, false },
+    { 0x1ULL, 0, 0, 0x8000000000000000ULL, 127, false },
+    { 0x1ULL, 0, 0x1ULL, 0, 128, true },
+};
+
+static const test_data test_rtable[] = {
+    { 1223ULL, 0, 1223ULL,   0, 0, false },
+    { 9223372036854775808ULL, 9223372036854775808ULL,
+        2147483648L, 2147483648ULL, 32, false },
+    { 9223372036854775808ULL, 9223372036854775808ULL,
+        9223372036854775808ULL, 0, 64, false },
+    { 9223372036854775808ULL, 9223372036854775808ULL,
+        36028797018963968ULL, 0, 72, false },
+    { 9223372036854775808ULL, 9223372036854775808ULL,
+        1ULL, 0, 127, false },
+    { 9223372036854775808ULL, 0, 4611686018427387904ULL, 0, 1, false },
+    { 9223372036854775808ULL, 0, 2305843009213693952ULL, 0, 2, false },
+    { 9223372036854775808ULL, 0, 36028797018963968ULL, 0, 8, false },
+    { 9223372036854775808ULL, 0, 140737488355328ULL, 0, 16, false },
+    { 9223372036854775808ULL, 0, 2147483648ULL, 0, 32, false },
+    { 9223372036854775808ULL, 0, 1ULL, 0, 63, false },
+    { 9223372036854775808ULL, 0, 0ULL, 0, 64, false },
+};
+
+static void test_lshift(void)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(test_ltable); ++i) {
+        bool overflow = false;
+        test_data tmp = test_ltable[i];
+        ulshift(&tmp.low, &tmp.high, tmp.shift, &overflow);
+        g_assert_cmpuint(tmp.low, ==, tmp.rlow);
+        g_assert_cmpuint(tmp.high, ==, tmp.rhigh);
+        g_assert_cmpuint(tmp.overflow, ==, tmp.overflow);
+    }
+}
+
+static void test_rshift(void)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(test_rtable); ++i) {
+        test_data tmp = test_rtable[i];
+        urshift(&tmp.low, &tmp.high, tmp.shift);
+        g_assert_cmpuint(tmp.low, ==, tmp.rlow);
+        g_assert_cmpuint(tmp.high, ==, tmp.rhigh);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/host-utils/test_lshift", test_lshift);
+    g_test_add_func("/host-utils/test_rshift", test_rshift);
+    return g_test_run();
+}
diff --git a/util/host-utils.c b/util/host-utils.c
index b166e57..7a97397 100644
--- a/util/host-utils.c
+++ b/util/host-utils.c
@@ -159,3 +159,41 @@  int divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
     return overflow;
 }
 
+void urshift(uint64_t *plow, uint64_t *phigh, uint32_t shift)
+{
+    uint64_t h = *phigh >> (shift & 63);
+    if (shift == 0) {
+        return;
+    } else if (shift >= 64) {
+        *plow = h;
+        *phigh = 0;
+    } else {
+        *plow = (*plow >> (shift & 63)) | (*phigh << (64 - (shift & 63)));
+        *phigh = h;
+    }
+}
+
+void ulshift(uint64_t *plow, uint64_t *phigh, uint32_t shift, bool *overflow)
+{
+    uint64_t low = *plow;
+    uint64_t high = *phigh;
+    shift &= 127;
+
+    if (shift == 0) {
+        return;
+    }
+
+    urshift(&low, &high, 128 - (shift & 127));
+
+    if (shift > 127 || low > 0 || high > 0) {
+        *overflow = true;
+    }
+
+    if (shift >= 64) {
+        *phigh = *plow << (shift & 63);
+        *plow = 0;
+    } else {
+        *phigh = (*plow >> (64 - (shift & 63))) | (*phigh << (shift & 63));
+        *plow = *plow << shift;
+    }
+}