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

[Aurelien Jarno]

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.

> +#else
>  target_ulong helper_popcntb (target_ulong val)
>  {
>      val = (val & 0x55555555) + ((val >>  1) & 0x55555555);
> @@ -507,12 +551,13 @@ target_ulong helper_popcntb (target_ulong val)
>      return val;
>  }
>  
> -#if defined(TARGET_PPC64)
> -target_ulong helper_popcntb_64 (target_ulong 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 & 0x55555555) + ((val >>  1) & 0x55555555);
> +    val = (val & 0x33333333) + ((val >>  2) & 0x33333333);
> +    val = (val & 0x0f0f0f0f) + ((val >>  4) & 0x0f0f0f0f);
> +    val = (val & 0x00ff00ff) + ((val >>  8) & 0x00ff00ff);
> +    val = (val & 0x0000ffff) + ((val >> 16) & 0x0000ffff);
>      return val;
>  }
>  #endif
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 0b6bfe7..0547047 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -1483,13 +1483,21 @@ static void gen_xoris(DisasContext *ctx)
>  /* popcntb : PowerPC 2.03 specification */
>  static void gen_popcntb(DisasContext *ctx)
>  {
> +    gen_helper_popcntb(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)]);
> +}
> +
> +static void gen_popcntw(DisasContext *ctx)
> +{
> +    gen_helper_popcntw(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)]);
> +}
> +
>  #if defined(TARGET_PPC64)
> -    if (ctx->sf_mode)
> -        gen_helper_popcntb_64(cpu_gpr[rA(ctx->opcode)], 
> cpu_gpr[rS(ctx->opcode)]);
> -    else
> -#endif
> -        gen_helper_popcntb(cpu_gpr[rA(ctx->opcode)], 
> cpu_gpr[rS(ctx->opcode)]);
> +/* popcntd: PowerPC 2.06 specification */
> +static void gen_popcntd(DisasContext *ctx)
> +{
> +    gen_helper_popcntd(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)]);
>  }
> +#endif
>  
>  #if defined(TARGET_PPC64)
>  /* extsw & extsw. */
> @@ -8226,7 +8234,9 @@ GEN_HANDLER(oris, 0x19, 0xFF, 0xFF, 0x00000000, 
> PPC_INTEGER),
>  GEN_HANDLER(xori, 0x1A, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
>  GEN_HANDLER(xoris, 0x1B, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
>  GEN_HANDLER(popcntb, 0x1F, 0x03, 0x03, 0x0000F801, PPC_POPCNTB),
> +GEN_HANDLER(popcntw, 0x1F, 0x1A, 0x0b, 0x0000F801, PPC_POPCNTWD),
>  #if defined(TARGET_PPC64)
> +GEN_HANDLER(popcntd, 0x1F, 0x1A, 0x0F, 0x0000F801, PPC_POPCNTWD),
>  GEN_HANDLER(cntlzd, 0x1F, 0x1A, 0x01, 0x00000000, PPC_64B),
>  #endif
>  GEN_HANDLER(rlwimi, 0x14, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net