Re: [PATCH v2] target/ppc: add vmsumudm vmsumcud instructions

[Richard Henderson]

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~