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

[Lijun Pan]

> 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