* [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
@ 2020-06-13 3:55 Lijun Pan
2020-06-15 16:12 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Lijun Pan @ 2020-06-13 3:55 UTC (permalink / raw
To: qemu-ppc, qemu-devel; +Cc: Lijun Pan
vmsumudm (Power ISA 3.0) - Vector Multiply-Sum Unsigned Doubleword Modulo
VA-form.
vmsumcud (Power ISA 3.1) - Vector Multiply-Sum & write Carry-out Unsigned
Doubleword VA-form.
Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
v2: move vmsumcudm() to qemu/int128.h as Richard Henderson suggested,
also rename addu128() to uint128_add() and include it in qemu/int128.h
disas/ppc.c | 2 +
include/qemu/int128.h | 97 +++++++++++++++++++++++++++++
target/ppc/helper.h | 4 +-
target/ppc/int_helper.c | 19 +++++-
target/ppc/translate.c | 1 -
target/ppc/translate/vmx-impl.inc.c | 39 ++++++------
target/ppc/translate/vmx-ops.inc.c | 2 +
7 files changed, 143 insertions(+), 21 deletions(-)
diff --git a/disas/ppc.c b/disas/ppc.c
index 63e97cfe1d..3ed4d23ed3 100644
--- a/disas/ppc.c
+++ b/disas/ppc.c
@@ -2261,7 +2261,9 @@ const struct powerpc_opcode powerpc_opcodes[] = {
{ "vmsumshs", VXA(4, 41), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
{ "vmsumubm", VXA(4, 36), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
{ "vmsumuhm", VXA(4, 38), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
+{ "vmsumudm", VXA(4, 35), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
{ "vmsumuhs", VXA(4, 39), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
+{ "vmsumcud", VXA(4, 23), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
{ "vmulesb", VX(4, 776), VX_MASK, PPCVEC, { VD, VA, VB } },
{ "vmulesh", VX(4, 840), VX_MASK, PPCVEC, { VD, VA, VB } },
{ "vmuleub", VX(4, 520), VX_MASK, PPCVEC, { VD, VA, VB } },
diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 5c9890db8b..3362973cc5 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -3,6 +3,7 @@
#ifdef CONFIG_INT128
#include "qemu/bswap.h"
+#include "qemu/host-utils.h"
typedef __int128_t Int128;
@@ -143,6 +144,55 @@ static inline Int128 bswap128(Int128 a)
return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
}
+/**
+ * uint128_add - add two 128-bit values (r=a+b, ca=carry-out)
+ * @ah: high 64 bits of a
+ * @al: low 64 bits of a
+ * @bh: high 64 bits of b
+ * @bl: low 64 bits of b
+ * @rh: high 64 bits of r to be returned
+ * @rl: low 64 bits of r to be returned
+ * @ca: carry out to be returned.
+ */
+static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
+ uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
+{
+ __uint128_t a = (__uint128_t)ah << 64 | (__uint128_t)al;
+ __uint128_t b = (__uint128_t)bh << 64 | (__uint128_t)bl;
+ __uint128_t r = a + b;
+
+ *rh = (uint64_t)(r >> 64);
+ *rl = (uint64_t)r;
+ *ca = (~a < b);
+}
+
+/**
+ * mulsum - (rh, rl) = ah*bh + al*bl + (ch, cl)
+ * @ah: high 64 bits of a
+ * @al: low 64 bits of a
+ * @bh: high 64 bits of b
+ * @bl: low 64 bits of b
+ * @ch: high 64 bits of c
+ * @cl: low 64 bits of c
+ * @rh: high 64 bits of r to be returned
+ * @rl: low 64 bits of r to be returned
+ * @ca: carry-out to be returned.
+ */
+static inline void mulsum(uint64_t ah, uint64_t al, uint64_t bh,
+ uint64_t bl, uint64_t ch, uint64_t cl, uint64_t *rh,
+ uint64_t *rl, uint64_t *ca)
+{
+ __uint128_t prod1, prod2, r;
+ __uint128_t c = (__uint128_t)ch << 64 | (__uint128_t)cl;
+
+ prod1 = (__uint128_t)ah * (__uint128_t)bh;
+ prod2 = (__uint128_t)al * (__uint128_t)bl;
+ r = prod1 + prod2 + c;
+ *rh = (uint64_t)(r >> 64);
+ *rl = (uint64_t)r;
+ *ca = (~prod1 < prod2) + (~c < (prod1 + prod2));
+}
+
#else /* !CONFIG_INT128 */
typedef struct Int128 Int128;
@@ -301,5 +351,52 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
*a = int128_sub(*a, b);
}
+/**
+ * uint128_add - add two 128-bit values (r=a+b, ca=carry-out)
+ * @ah: high 64 bits of a
+ * @al: low 64 bits of a
+ * @bh: high 64 bits of b
+ * @bl: low 64 bits of b
+ * @rh: high 64 bits of r to be returned
+ * @rl: low 64 bits of r to be returned
+ * @ca: carry out to be returned.
+ */
+static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
+ uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
+{
+ uint64_t lo = al + bl;
+ uint64_t hi = ah + bh + (~al < bl);
+ uint64_t hi_t = ah + bh;
+ uint64_t carry = (~ah < bh) + (~hi_t < (~al < bl));
+
+ *rl = lo;
+ *rh = hi;
+ *ca = carry;
+}
+
+/**
+ * mulsum - (rh, rl) = ah*bh + al*bl + (ch, cl)
+ * @ah: high 64 bits of a
+ * @al: low 64 bits of a
+ * @bh: high 64 bits of b
+ * @bl: low 64 bits of b
+ * @ch: high 64 bits of c
+ * @cl: low 64 bits of c
+ * @rh: high 64 bits of r to be returned
+ * @rl: low 64 bits of r to be returned
+ * @ca: carry-out to be returned.
+ */
+static inline void mulsum(uint64_t ah, uint64_t al, uint64_t bh,
+ uint64_t bl, uint64_t ch, uint64_t cl, uint64_t *rh,
+ uint64_t *rl, uint64_t *ca)
+{
+ uint64_t p1h, p1l, p2h, p2l, sh, sl, ca1, ca2;
+ mulu64(&p1l, &p1h ,ah, bh);
+ mulu64(&p2l, &p2h ,al, bl);
+ uint128_add(p1h, p1l, p2h, p2l, &sh, &sl, &ca1);
+ uint128_add(sh, sl, ch, cl, rh, rl, &ca2);
+ *ca = ca1 + ca2;
+}
+
#endif /* CONFIG_INT128 */
#endif /* INT128_H */
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 2dfa1c6942..d540e8f30b 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -263,10 +263,12 @@ DEF_HELPER_3(vpkpx, void, avr, avr, avr)
DEF_HELPER_5(vmhaddshs, void, env, avr, avr, avr, avr)
DEF_HELPER_5(vmhraddshs, void, env, avr, avr, avr, avr)
DEF_HELPER_5(vmsumuhm, void, env, avr, avr, avr, avr)
+DEF_HELPER_5(vmsumudm, void, env, avr, avr, avr, avr)
DEF_HELPER_5(vmsumuhs, void, env, avr, avr, avr, avr)
DEF_HELPER_5(vmsumshm, void, env, avr, avr, avr, avr)
DEF_HELPER_5(vmsumshs, void, env, avr, avr, avr, avr)
-DEF_HELPER_4(vmladduhm, void, avr, avr, avr, avr)
+DEF_HELPER_5(vmsumcud, void, env, avr, avr, avr, avr)
+DEF_HELPER_5(vmladduhm, void, env, avr, avr, avr, avr)
DEF_HELPER_FLAGS_2(mtvscr, TCG_CALL_NO_RWG, void, env, i32)
DEF_HELPER_FLAGS_1(mfvscr, TCG_CALL_NO_RWG, i32, env)
DEF_HELPER_3(lvebx, void, env, avr, tl)
diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index be53cd6f68..5f257b7b86 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -926,7 +926,8 @@ void helper_vmhraddshs(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
}
}
-void helper_vmladduhm(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, ppc_avr_t *c)
+void helper_vmladduhm(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
+ ppc_avr_t *b, ppc_avr_t *c)
{
int i;
@@ -1064,6 +1065,22 @@ void helper_vmsumuhs(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
}
}
+void helper_vmsumudm(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
+ ppc_avr_t *b, ppc_avr_t *c)
+{
+ uint64_t ca;
+ mulsum(a->VsrD(0), a->VsrD(1), b->VsrD(0), b->VsrD(1), c->VsrD(0), c->VsrD(1),
+ &r->VsrD(0), &r->VsrD(1), &ca);
+}
+void helper_vmsumcud(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
+ ppc_avr_t *b, ppc_avr_t *c)
+{
+ uint64_t rh, rl;
+ mulsum(a->VsrD(0), a->VsrD(1), b->VsrD(0), b->VsrD(1), c->VsrD(0), c->VsrD(1),
+ &rh, &rl, &r->VsrD(1));
+ r->VsrD(0) = 0;
+}
+
#define VMUL_DO_EVN(name, mul_element, mul_access, prod_access, cast) \
void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \
{ \
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 4ce3d664b5..35ff1aa77e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7281,7 +7281,6 @@ GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
GEN_HANDLER(mtvscr, 0x04, 0x2, 0x19, 0x03ff0000, PPC_ALTIVEC),
-GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
#if defined(TARGET_PPC64)
GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
PPC2_ISA300),
diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c
index 403ed3a01c..5c0e44d7fb 100644
--- a/target/ppc/translate/vmx-impl.inc.c
+++ b/target/ppc/translate/vmx-impl.inc.c
@@ -1248,6 +1248,25 @@ static void gen_vsldoi(DisasContext *ctx)
tcg_temp_free_i32(sh);
}
+#define GEN_VAFORM(name, opc2) \
+static void glue(gen_, name)(DisasContext *ctx) \
+{ \
+ TCGv_ptr ra, rb, rc, rd; \
+ if (unlikely(!ctx->altivec_enabled)) { \
+ gen_exception(ctx, POWERPC_EXCP_VPU); \
+ return; \
+ } \
+ ra = gen_avr_ptr(rA(ctx->opcode)); \
+ rb = gen_avr_ptr(rB(ctx->opcode)); \
+ rc = gen_avr_ptr(rC(ctx->opcode)); \
+ rd = gen_avr_ptr(rD(ctx->opcode)); \
+ gen_helper_##name(cpu_env, rd, ra, rb, rc); \
+ tcg_temp_free_ptr(ra); \
+ tcg_temp_free_ptr(rb); \
+ tcg_temp_free_ptr(rc); \
+ tcg_temp_free_ptr(rd); \
+}
+
#define GEN_VAFORM_PAIRED(name0, name1, opc2) \
static void glue(gen_, name0##_##name1)(DisasContext *ctx) \
{ \
@@ -1272,24 +1291,8 @@ static void glue(gen_, name0##_##name1)(DisasContext *ctx) \
}
GEN_VAFORM_PAIRED(vmhaddshs, vmhraddshs, 16)
-
-static void gen_vmladduhm(DisasContext *ctx)
-{
- TCGv_ptr ra, rb, rc, rd;
- if (unlikely(!ctx->altivec_enabled)) {
- gen_exception(ctx, POWERPC_EXCP_VPU);
- return;
- }
- ra = gen_avr_ptr(rA(ctx->opcode));
- rb = gen_avr_ptr(rB(ctx->opcode));
- rc = gen_avr_ptr(rC(ctx->opcode));
- rd = gen_avr_ptr(rD(ctx->opcode));
- gen_helper_vmladduhm(rd, ra, rb, rc);
- tcg_temp_free_ptr(ra);
- tcg_temp_free_ptr(rb);
- tcg_temp_free_ptr(rc);
- tcg_temp_free_ptr(rd);
-}
+GEN_VAFORM(vmsumcud, 11)
+GEN_VAFORM_PAIRED(vmladduhm, vmsumudm, 17)
static void gen_vpermr(DisasContext *ctx)
{
diff --git a/target/ppc/translate/vmx-ops.inc.c b/target/ppc/translate/vmx-ops.inc.c
index 84e05fb827..aee23e31c6 100644
--- a/target/ppc/translate/vmx-ops.inc.c
+++ b/target/ppc/translate/vmx-ops.inc.c
@@ -276,6 +276,8 @@ GEN_VAFORM_PAIRED(vmsumuhm, vmsumuhs, 19),
GEN_VAFORM_PAIRED(vmsumshm, vmsumshs, 20),
GEN_VAFORM_PAIRED(vsel, vperm, 21),
GEN_VAFORM_PAIRED(vmaddfp, vnmsubfp, 23),
+GEN_HANDLER(vmsumcud, 0x4, 11, 0xFF, 0x00000000, PPC_ALTIVEC),
+GEN_VAFORM_PAIRED(vmladduhm, vmsumudm, 17),
GEN_VXFORM_DUAL(vclzb, vpopcntb, 1, 28, PPC_NONE, PPC2_ALTIVEC_207),
GEN_VXFORM_DUAL(vclzh, vpopcnth, 1, 29, PPC_NONE, PPC2_ALTIVEC_207),
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
2020-06-13 3:55 [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions Lijun Pan
@ 2020-06-15 16:12 ` Richard Henderson
2020-06-15 20:53 ` Lijun Pan
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2020-06-15 16:12 UTC (permalink / raw
To: Lijun Pan, qemu-ppc, qemu-devel
On 6/12/20 8:55 PM, Lijun Pan wrote:
> vmsumudm (Power ISA 3.0) - Vector Multiply-Sum Unsigned Doubleword Modulo
> VA-form.
> vmsumcud (Power ISA 3.1) - Vector Multiply-Sum & write Carry-out Unsigned
> Doubleword VA-form.
>
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> ---
> v2: move vmsumcudm() to qemu/int128.h as Richard Henderson suggested,
> also rename addu128() to uint128_add() and include it in qemu/int128.h
>
> disas/ppc.c | 2 +
> include/qemu/int128.h | 97 +++++++++++++++++++++++++++++
> target/ppc/helper.h | 4 +-
> target/ppc/int_helper.c | 19 +++++-
> target/ppc/translate.c | 1 -
> target/ppc/translate/vmx-impl.inc.c | 39 ++++++------
> target/ppc/translate/vmx-ops.inc.c | 2 +
> 7 files changed, 143 insertions(+), 21 deletions(-)
>
> diff --git a/disas/ppc.c b/disas/ppc.c
> index 63e97cfe1d..3ed4d23ed3 100644
> --- a/disas/ppc.c
> +++ b/disas/ppc.c
> @@ -2261,7 +2261,9 @@ const struct powerpc_opcode powerpc_opcodes[] = {
> { "vmsumshs", VXA(4, 41), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> { "vmsumubm", VXA(4, 36), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> { "vmsumuhm", VXA(4, 38), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> +{ "vmsumudm", VXA(4, 35), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> { "vmsumuhs", VXA(4, 39), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> +{ "vmsumcud", VXA(4, 23), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
> { "vmulesb", VX(4, 776), VX_MASK, PPCVEC, { VD, VA, VB } },
> { "vmulesh", VX(4, 840), VX_MASK, PPCVEC, { VD, VA, VB } },
> { "vmuleub", VX(4, 520), VX_MASK, PPCVEC, { VD, VA, VB } },
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index 5c9890db8b..3362973cc5 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -3,6 +3,7 @@
>
> #ifdef CONFIG_INT128
> #include "qemu/bswap.h"
> +#include "qemu/host-utils.h"
>
> typedef __int128_t Int128;
>
> @@ -143,6 +144,55 @@ static inline Int128 bswap128(Int128 a)
> return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
> }
>
> +/**
> + * uint128_add - add two 128-bit values (r=a+b, ca=carry-out)
> + * @ah: high 64 bits of a
> + * @al: low 64 bits of a
> + * @bh: high 64 bits of b
> + * @bl: low 64 bits of b
> + * @rh: high 64 bits of r to be returned
> + * @rl: low 64 bits of r to be returned
> + * @ca: carry out to be returned.
> + */
> +static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
> + uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
> +{
> + __uint128_t a = (__uint128_t)ah << 64 | (__uint128_t)al;
> + __uint128_t b = (__uint128_t)bh << 64 | (__uint128_t)bl;
> + __uint128_t r = a + b;
> +
> + *rh = (uint64_t)(r >> 64);
> + *rl = (uint64_t)r;
> + *ca = (~a < b);
> +}
This is *not* what I had in mind at all.
int128.h should be operating on Int128, and *not* component uint64_t values.
> +
> +/**
> + * mulsum - (rh, rl) = ah*bh + al*bl + (ch, cl)
> + * @ah: high 64 bits of a
> + * @al: low 64 bits of a
> + * @bh: high 64 bits of b
> + * @bl: low 64 bits of b
> + * @ch: high 64 bits of c
> + * @cl: low 64 bits of c
> + * @rh: high 64 bits of r to be returned
> + * @rl: low 64 bits of r to be returned
> + * @ca: carry-out to be returned.
> + */
> +static inline void mulsum(uint64_t ah, uint64_t al, uint64_t bh,
> + uint64_t bl, uint64_t ch, uint64_t cl, uint64_t *rh,
> + uint64_t *rl, uint64_t *ca)
> +{
> + __uint128_t prod1, prod2, r;
> + __uint128_t c = (__uint128_t)ch << 64 | (__uint128_t)cl;
> +
> + prod1 = (__uint128_t)ah * (__uint128_t)bh;
> + prod2 = (__uint128_t)al * (__uint128_t)bl;
> + r = prod1 + prod2 + c;
> + *rh = (uint64_t)(r >> 64);
> + *rl = (uint64_t)r;
> + *ca = (~prod1 < prod2) + (~c < (prod1 + prod2));
> +}
Why is mulsum an interesting primitive for int128.h?
I would think int128_mul and int128_add sufficient here.
I did not ask you to place the entire ppc instruction in int128.h.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
2020-06-15 16:12 ` Richard Henderson
@ 2020-06-15 20:53 ` Lijun Pan
2020-06-18 23:09 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Lijun Pan @ 2020-06-15 20:53 UTC (permalink / raw
To: Richard Henderson; +Cc: qemu-ppc, Lijun Pan, qemu-devel
> On Jun 15, 2020, at 11:12 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
>
> On 6/12/20 8:55 PM, Lijun Pan wrote:
>> vmsumudm (Power ISA 3.0) - Vector Multiply-Sum Unsigned Doubleword Modulo
>> VA-form.
>> vmsumcud (Power ISA 3.1) - Vector Multiply-Sum & write Carry-out Unsigned
>> Doubleword VA-form.
>>
>> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
>> ---
>> v2: move vmsumcudm() to qemu/int128.h as Richard Henderson suggested,
>> also rename addu128() to uint128_add() and include it in qemu/int128.h
>>
>> disas/ppc.c | 2 +
>> include/qemu/int128.h | 97 +++++++++++++++++++++++++++++
>> target/ppc/helper.h | 4 +-
>> target/ppc/int_helper.c | 19 +++++-
>> target/ppc/translate.c | 1 -
>> target/ppc/translate/vmx-impl.inc.c | 39 ++++++------
>> target/ppc/translate/vmx-ops.inc.c | 2 +
>> 7 files changed, 143 insertions(+), 21 deletions(-)
>>
>> diff --git a/disas/ppc.c b/disas/ppc.c
>> index 63e97cfe1d..3ed4d23ed3 100644
>> --- a/disas/ppc.c
>> +++ b/disas/ppc.c
>> @@ -2261,7 +2261,9 @@ const struct powerpc_opcode powerpc_opcodes[] = {
>> { "vmsumshs", VXA(4, 41), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> { "vmsumubm", VXA(4, 36), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> { "vmsumuhm", VXA(4, 38), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> +{ "vmsumudm", VXA(4, 35), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> { "vmsumuhs", VXA(4, 39), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> +{ "vmsumcud", VXA(4, 23), VXA_MASK, PPCVEC, { VD, VA, VB, VC } },
>> { "vmulesb", VX(4, 776), VX_MASK, PPCVEC, { VD, VA, VB } },
>> { "vmulesh", VX(4, 840), VX_MASK, PPCVEC, { VD, VA, VB } },
>> { "vmuleub", VX(4, 520), VX_MASK, PPCVEC, { VD, VA, VB } },
>> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
>> index 5c9890db8b..3362973cc5 100644
>> --- a/include/qemu/int128.h
>> +++ b/include/qemu/int128.h
>> @@ -3,6 +3,7 @@
>>
>> #ifdef CONFIG_INT128
>> #include "qemu/bswap.h"
>> +#include "qemu/host-utils.h"
>>
>> typedef __int128_t Int128;
>>
>> @@ -143,6 +144,55 @@ static inline Int128 bswap128(Int128 a)
>> return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
>> }
>>
>> +/**
>> + * uint128_add - add two 128-bit values (r=a+b, ca=carry-out)
>> + * @ah: high 64 bits of a
>> + * @al: low 64 bits of a
>> + * @bh: high 64 bits of b
>> + * @bl: low 64 bits of b
>> + * @rh: high 64 bits of r to be returned
>> + * @rl: low 64 bits of r to be returned
>> + * @ca: carry out to be returned.
>> + */
>> +static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
>> + uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
>> +{
>> + __uint128_t a = (__uint128_t)ah << 64 | (__uint128_t)al;
>> + __uint128_t b = (__uint128_t)bh << 64 | (__uint128_t)bl;
>> + __uint128_t r = a + b;
>> +
>> + *rh = (uint64_t)(r >> 64);
>> + *rl = (uint64_t)r;
>> + *ca = (~a < b);
>> +}
>
> This is *not* what I had in mind at all.
>
> int128.h should be operating on Int128, and *not* component uint64_t values.
Should uint128_add() be included in a new file called uint128.h? or still at host-utils.h?
>
>
>> +
>> +/**
>> + * mulsum - (rh, rl) = ah*bh + al*bl + (ch, cl)
>> + * @ah: high 64 bits of a
>> + * @al: low 64 bits of a
>> + * @bh: high 64 bits of b
>> + * @bl: low 64 bits of b
>> + * @ch: high 64 bits of c
>> + * @cl: low 64 bits of c
>> + * @rh: high 64 bits of r to be returned
>> + * @rl: low 64 bits of r to be returned
>> + * @ca: carry-out to be returned.
>> + */
>> +static inline void mulsum(uint64_t ah, uint64_t al, uint64_t bh,
>> + uint64_t bl, uint64_t ch, uint64_t cl, uint64_t *rh,
>> + uint64_t *rl, uint64_t *ca)
>> +{
>> + __uint128_t prod1, prod2, r;
>> + __uint128_t c = (__uint128_t)ch << 64 | (__uint128_t)cl;
>> +
>> + prod1 = (__uint128_t)ah * (__uint128_t)bh;
>> + prod2 = (__uint128_t)al * (__uint128_t)bl;
>> + r = prod1 + prod2 + c;
>> + *rh = (uint64_t)(r >> 64);
>> + *rl = (uint64_t)r;
>> + *ca = (~prod1 < prod2) + (~c < (prod1 + prod2));
>> +}
>
> Why is mulsum an interesting primitive for int128.h?
> I would think int128_mul and int128_add sufficient here.
But prod1, prod2, r are unsigned 128-bit values. Changing above code to the following
implementation doesn’t seem right.
prod1 = int128_mul((__uint128_t)ah, (__uint128_t)bh);
prod2 = int128_mul((__uint128_t)al * (__uint128_t)bl);
r = int128_add(prod1, prod2);
r = int128_add(r, c);
Maybe you mean using uint128_mul & uint128_add?
>
> I did not ask you to place the entire ppc instruction in int128.h.
vmsumudm/vmsumcud operate as follows:
1. 128-bit prod1 = (high 64 bits of a) * (high 64 bits of b), // I reuse mulu64()
2. 128-bit prod2 = (high 64 bits of b) * (high 64 bits of b), // I reuse mulu64()
3. 128-bit result = prod1 + prod2 + c; // I added addu128() in v1, renamed it to uint128_add() in v2
vmsumudm takes the result,
vmsumcud takes the carry-out
v1 patch adds addu128() in host-utils.h and reuse the mulu64() from host-utils.h.
To better understand your request, may I ask you several questions:
1. keep mulsum() in target/ppc/int_helper.c?
If so, it will inevitably have #ifdef CONFIG_INT128 #else #endif in that function.
2. still add addu128()/uint128_add() in host-utils.h?
3. Do you want int128_mul() to replace mulu64()?
4. Do you want int128_add() to relace uint128_add()?
5. If I add int128_mul and int128_add, shouldn’t I also add uint128_mul and uint128_add?
should I also create uint128.h to include uint128_add & uint128_mul?
Thanks,
Lijun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
2020-06-15 20:53 ` Lijun Pan
@ 2020-06-18 23:09 ` Richard Henderson
2020-06-19 5:10 ` Lijun Pan
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2020-06-18 23:09 UTC (permalink / raw
To: Lijun Pan; +Cc: qemu-ppc, Lijun Pan, qemu-devel
On 6/15/20 1:53 PM, Lijun Pan wrote:
>>> +static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
>>> + uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
>>> +{
>>> + __uint128_t a = (__uint128_t)ah << 64 | (__uint128_t)al;
>>> + __uint128_t b = (__uint128_t)bh << 64 | (__uint128_t)bl;
>>> + __uint128_t r = a + b;
>>> +
>>> + *rh = (uint64_t)(r >> 64);
>>> + *rl = (uint64_t)r;
>>> + *ca = (~a < b);
>>> +}
>>
>> This is *not* what I had in mind at all.
>>
>> int128.h should be operating on Int128, and *not* component uint64_t values.
>
> Should uint128_add() be included in a new file called uint128.h? or still at host-utils.h?
If you want this sort of specific operation, you should leave it in target/ppc/.
I had been hoping that you could make use of Int128 as-is, or with minimal
adjustment in the same style.
> vmsumudm/vmsumcud operate as follows:
> 1. 128-bit prod1 = (high 64 bits of a) * (high 64 bits of b), // I reuse mulu64()
> 2. 128-bit prod2 = (high 64 bits of b) * (high 64 bits of b), // I reuse mulu64()
> 3. 128-bit result = prod1 + prod2 + c; // I added addu128() in v1, renamed it to uint128_add() in v2
Really? That seems a very odd computation. Your code,
> + prod1 = (__uint128_t)ah * (__uint128_t)bh;
> + prod2 = (__uint128_t)al * (__uint128_t)bl;
> + r = prod1 + prod2 + c;
is slightly different, but still very odd.
Why would we be adding the intermediate 128th bit of the 256-bit product
(prod1, bit 0) with the 0th bit of the 256-bit product (prod2, bit 0).
Unfortunately, I can't find the v3.1 spec online yet, so I can't look at this
myself. What is the instruction supposed to produce?
> To better understand your request, may I ask you several questions:
> 1. keep mulsum() in target/ppc/int_helper.c?
Probably.
> If so, it will inevitably have #ifdef CONFIG_INT128 #else #endif in that function.
No, you don't have to ifdef. You can use uint64_t alone and not rely on
compiler support for __uint128_t at all.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
2020-06-18 23:09 ` Richard Henderson
@ 2020-06-19 5:10 ` Lijun Pan
2020-06-19 20:45 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Lijun Pan @ 2020-06-19 5:10 UTC (permalink / raw
To: Richard Henderson; +Cc: Lijun Pan, qemu-ppc, qemu-devel
> On Jun 18, 2020, at 6:09 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
>
> On 6/15/20 1:53 PM, Lijun Pan wrote:
>>>> +static inline void uint128_add(uint64_t ah, uint64_t al, uint64_t bh,
>>>> + uint64_t bl, uint64_t *rh, uint64_t *rl, uint64_t *ca)
>>>> +{
>>>> + __uint128_t a = (__uint128_t)ah << 64 | (__uint128_t)al;
>>>> + __uint128_t b = (__uint128_t)bh << 64 | (__uint128_t)bl;
>>>> + __uint128_t r = a + b;
>>>> +
>>>> + *rh = (uint64_t)(r >> 64);
>>>> + *rl = (uint64_t)r;
>>>> + *ca = (~a < b);
>>>> +}
>>>
>>> This is *not* what I had in mind at all.
>>>
>>> int128.h should be operating on Int128, and *not* component uint64_t values.
>>
>> Should uint128_add() be included in a new file called uint128.h? or still at host-utils.h?
>
> If you want this sort of specific operation, you should leave it in target/ppc/.
>
> I had been hoping that you could make use of Int128 as-is, or with minimal
> adjustment in the same style.
>
>> vmsumudm/vmsumcud operate as follows:
>> 1. 128-bit prod1 = (high 64 bits of a) * (high 64 bits of b), // I reuse mulu64()
This is an implementation not relying on 128 bit compiler support (not defined CONFIG_INT128),
hence using mulu64().
>> 2. 128-bit prod2 = (high 64 bits of b) * (high 64 bits of b), // I reuse mulu64()
>> 3. 128-bit result = prod1 + prod2 + c; // I added addu128() in v1, renamed it to uint128_add() in v2
>
> Really? That seems a very odd computation. Your code,
>
>> + prod1 = (__uint128_t)ah * (__uint128_t)bh;
>> + prod2 = (__uint128_t)al * (__uint128_t)bl;
>> + r = prod1 + prod2 + c;
>
> is slightly different, but still very odd.
Above 3 lines of code are using 128 bit compiler suppor (#ifdef CONFIG_INT128).
>
> Why would we be adding the intermediate 128th bit of the 256-bit product
> (prod1, bit 0) with the 0th bit of the 256-bit product (prod2, bit 0).
>
> Unfortunately, I can't find the v3.1 spec online yet, so I can't look at this
> myself. What is the instruction supposed to produce?
https://ibm.ent.box.com/s/hhjfw0x0lrbtyzmiaffnbxh2fuo0fog0
>
>> To better understand your request, may I ask you several questions:
>> 1. keep mulsum() in target/ppc/int_helper.c?
>
> Probably.
>
>> If so, it will inevitably have #ifdef CONFIG_INT128 #else #endif in that function.
>
> No, you don't have to ifdef. You can use uint64_t alone and not rely on
> compiler support for __uint128_t at all.
>
>
> r~
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions
2020-06-19 5:10 ` Lijun Pan
@ 2020-06-19 20:45 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2020-06-19 20:45 UTC (permalink / raw
To: Lijun Pan; +Cc: qemu-ppc, qemu-devel
On 6/18/20 10:10 PM, Lijun Pan wrote:
>> Unfortunately, I can't find the v3.1 spec online yet, so I can't look at this
>> myself. What is the instruction supposed to produce?
>
> https://ibm.ent.box.com/s/hhjfw0x0lrbtyzmiaffnbxh2fuo0fog0
Thank you. So it really is the sum of an input and two separate 64x64->128 bit
multiplies.
I suggest
void helper_vmsumudm(CPUPPCState *env, ppc_avr_t *r,
ppc_avr_t *a, ppc_avr_t *b, ppc_avr_t *c)
{
Int128 sum;
uint64_t lo, hi;
sum = int128_make128(c->VsrD(1), c->VsrD(0));
mulu64(&lo, &hi, a->VsrD(0), b->VsrD(0));
sum = int128_add(sum, int128_make128(lo, hi));
mulu64(&lo, &hi, a->VsrD(1), b->VsrD(1));
sum = int128_add(sum, int128_make128(lo, hi));
r->VsrD(0) = int128_gethi(sum);
r->VsrD(1) = int128_getlo(sum);
}
void helper_vmsumcud(CPUPPCState *env, ppc_avr_t *r,
ppc_avr_t *a, ppc_avr_t *b, ppc_avr_t *c)
{
Int128 sum;
uint64_t p1lo, p1hi, p2lo, p2hi;
mulu64(&p1lo, &p1hi, a->VsrD(0), b->VsrD(0));
mulu64(&p2lo, &p2hi, a->VsrD(1), b->VsrD(1));
/* Sum lowest 64-bit elements. */
sum = int128_make128(c->VsrD(1), 0);
sum = int128_add(sum, int128_make128(p1lo, 0));
sum = int128_add(sum, int128_make128(p2lo, 0));
/*
* Discard low 64-bits, leaving the carry into bit 64.
* Then sum the higher 64-bit elements.
*/
sum = int128_rshift(sum, 64);
sum = int128_add(sum, int128_make128(c->VsrD(0), 0));
sum = int128_add(sum, int128_make128(p1hi, 0));
sum = int128_add(sum, int128_make128(p2hi, 0));
/* The result is only the carry into bits 64 & 65. */
r->VsrD(1) = int128_gethi(sum);
r->VsrD(0) = 0;
}
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-19 20:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-13 3:55 [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions Lijun Pan
2020-06-15 16:12 ` Richard Henderson
2020-06-15 20:53 ` Lijun Pan
2020-06-18 23:09 ` Richard Henderson
2020-06-19 5:10 ` Lijun Pan
2020-06-19 20:45 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.