Re: [Qemu-devel] [PATCH 06/27] Correct ppc popcntb logic, implement popcntw and popcntd

[Alexander Graf]

On 01.04.2011, at 19:58, Aurelien Jarno wrote:

> On Fri, Apr 01, 2011 at 03:15:13PM +1100, David Gibson wrote:
>> From: David Gibson <address@hidden>
>> 
>> qemu already includes support for the popcntb instruction introduced
>> in POWER5 (although it doesn't actually allow you to choose POWER5).
>> 
>> However, the logic is slightly incorrect: it will generate results
>> truncated to 32-bits when the CPU is in 32-bit mode.  This is not
>> normal for powerpc - generally arithmetic instructions on a 64-bit
>> powerpc cpu will generate full 64 bit results, it's just that only the
>> low 32 bits will be significant for condition codes.
>> 
>> This patch corrects this nit, which actually simplifies the code slightly.
>> 
>> In addition, this patch implements the popcntw and popcntd
>> instructions added in POWER7, in preparation for allowing POWER7 as an
>> emulated CPU.
>> 
>> Signed-off-by: David Gibson <address@hidden>
>> ---
>> target-ppc/cpu.h       |    2 +
>> target-ppc/helper.h    |    3 +-
>> target-ppc/op_helper.c |   55 +++++++++++++++++++++++++++++++++++++++++++----
>> target-ppc/translate.c |   20 +++++++++++++----
>> 4 files changed, 69 insertions(+), 11 deletions(-)
>> 
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index f293f85..37dde39 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -1505,6 +1505,8 @@ enum {
>>     PPC_DCRX           = 0x2000000000000000ULL,
>>     /* user-mode DCR access, implemented in PowerPC 460                      
>> */
>>     PPC_DCRUX          = 0x4000000000000000ULL,
>> +    /* popcntw and popcntd instructions                                     
>>  */
>> +    PPC_POPCNTWD       = 0x8000000000000000ULL,
>> };
>> 
>> /*****************************************************************************/
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index 2b4744d..7c02be9 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -38,10 +38,11 @@ DEF_HELPER_2(mulldo, i64, i64, i64)
>> 
>> DEF_HELPER_FLAGS_1(cntlzw, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> DEF_HELPER_FLAGS_1(popcntb, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> +DEF_HELPER_FLAGS_1(popcntw, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> DEF_HELPER_2(sraw, tl, tl, tl)
>> #if defined(TARGET_PPC64)
>> DEF_HELPER_FLAGS_1(cntlzd, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> -DEF_HELPER_FLAGS_1(popcntb_64, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> +DEF_HELPER_FLAGS_1(popcntd, TCG_CALL_CONST | TCG_CALL_PURE, tl, tl)
>> DEF_HELPER_2(srad, tl, tl, tl)
>> #endif
>> 
>> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
>> index aa2e8ba..b1b883d 100644
>> --- a/target-ppc/op_helper.c
>> +++ b/target-ppc/op_helper.c
>> @@ -499,6 +499,50 @@ target_ulong helper_srad (target_ulong value, 
>> target_ulong shift)
>> }
>> #endif
>> 
>> +#if defined(TARGET_PPC64)
>> +target_ulong helper_popcntb (target_ulong val)
>> +{
>> +    val = (val & 0x5555555555555555ULL) + ((val >>  1) &
>> +                                           0x5555555555555555ULL);
>> +    val = (val & 0x3333333333333333ULL) + ((val >>  2) &
>> +                                           0x3333333333333333ULL);
>> +    val = (val & 0x0f0f0f0f0f0f0f0fULL) + ((val >>  4) &
>> +                                           0x0f0f0f0f0f0f0f0fULL);
>> +    return val;
>> +}
>> +
>> +target_ulong helper_popcntw (target_ulong val)
>> +{
>> +    val = (val & 0x5555555555555555ULL) + ((val >>  1) &
>> +                                           0x5555555555555555ULL);
>> +    val = (val & 0x3333333333333333ULL) + ((val >>  2) &
>> +                                           0x3333333333333333ULL);
>> +    val = (val & 0x0f0f0f0f0f0f0f0fULL) + ((val >>  4) &
>> +                                           0x0f0f0f0f0f0f0f0fULL);
>> +    val = (val & 0x00ff00ff00ff00ffULL) + ((val >>  8) &
>> +                                           0x00ff00ff00ff00ffULL);
>> +    val = (val & 0x0000ffff0000ffffULL) + ((val >> 16) &
>> +                                           0x0000ffff0000ffffULL);
>> +    return val;
>> +}
>> +
>> +target_ulong helper_popcntd (target_ulong val)
>> +{
>> +    val = (val & 0x5555555555555555ULL) + ((val >>  1) &
>> +                                           0x5555555555555555ULL);
>> +    val = (val & 0x3333333333333333ULL) + ((val >>  2) &
>> +                                           0x3333333333333333ULL);
>> +    val = (val & 0x0f0f0f0f0f0f0f0fULL) + ((val >>  4) &
>> +                                           0x0f0f0f0f0f0f0f0fULL);
>> +    val = (val & 0x00ff00ff00ff00ffULL) + ((val >>  8) &
>> +                                           0x00ff00ff00ff00ffULL);
>> +    val = (val & 0x0000ffff0000ffffULL) + ((val >> 16) &
>> +                                           0x0000ffff0000ffffULL);
>> +    val = (val & 0x00000000ffffffffULL) + ((val >> 32) &
>> +                                           0x00000000ffffffffULL);
>> +    return val;
>> +}
> 
> I probably arrive a bit late, but note that for this one you can use
> ctpop64() (from host-utils.h), which also uses a GCC builtin when
> available.

Ah, nice. David - sounds like a good chance for a follow-up patch :)


Alex