[Mesa-dev] [PATCH] llvmpipe: use vpkswss when dst is signed

[Jose Fonseca]

On 17/01/16 20:55, Oded Gabbay wrote:
> This patch fixes a bug when building a pack instruction.
>
> For POWER (altivec), in case the destination is signed and the
> src width is 32, we need to use vpkswss. The original code used vpkuwus,
> which emits an unsigned result.
>
> This fixes the following piglit tests on ppc64le:
> - spec at arb_color_buffer_float@gl_rgba8-drawpixels
> - shaders at glsl-fs-fogscale
> - spec at arb_timer_query@query gl_timestamp
>
> I've also corrected some coding style issues in the function.

The if statement spacing chance is good.

But please don't split the "} else" in two lines: I know it's not the 
Mesa standard, but it's the standard we use at VMware, and I also use at 
Apitrace, so my brain just spews them out without thinking (given that 
otherwise the formatting is quite similar.)

Given there's little hope to keep else statement and other things 
consistent, I rather we just leave them alone as they are (be it in one 
or two lines) to avoid any potential merge conflicts when cherry picking 
changes etc.

Thank you regardless.

Jose

>
> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
> ---
>   src/gallium/auxiliary/gallivm/lp_bld_pack.c | 44 ++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_pack.c b/src/gallium/auxiliary/gallivm/lp_bld_pack.c
> index cdf6d80..39a5c3a 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_pack.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_pack.c
> @@ -461,15 +461,15 @@ lp_build_pack2(struct gallivm_state *gallivm,
>      assert(src_type.length * 2 == dst_type.length);
>
>      /* Check for special cases first */
> -   if((util_cpu_caps.has_sse2 || util_cpu_caps.has_altivec) &&
> -       src_type.width * src_type.length >= 128) {
> +   if ((util_cpu_caps.has_sse2 || util_cpu_caps.has_altivec) &&
> +        src_type.width * src_type.length >= 128) {
>         const char *intrinsic = NULL;
>         boolean swap_intrinsic_operands = FALSE;
>
>         switch(src_type.width) {
>         case 32:
>            if (util_cpu_caps.has_sse2) {
> -           if(dst_type.sign) {
> +           if (dst_type.sign) {
>                 intrinsic = "llvm.x86.sse2.packssdw.128";
>              }
>              else {
> @@ -477,34 +477,39 @@ lp_build_pack2(struct gallivm_state *gallivm,
>                    intrinsic = "llvm.x86.sse41.packusdw";
>                 }
>              }
> -         } else if (util_cpu_caps.has_altivec) {
> +         }
> +         else if (util_cpu_caps.has_altivec) {
>               if (dst_type.sign) {
> -              intrinsic = "llvm.ppc.altivec.vpkswus";
> -           } else {
> -              intrinsic = "llvm.ppc.altivec.vpkuwus";
> -           }
> +               intrinsic = "llvm.ppc.altivec.vpkswss";
> +            }
> +            else {
> +               intrinsic = "llvm.ppc.altivec.vpkuwus";
> +            }
>   #ifdef PIPE_ARCH_LITTLE_ENDIAN
> -           swap_intrinsic_operands = TRUE;
> +            swap_intrinsic_operands = TRUE;
>   #endif
>            }
>            break;
>         case 16:
>            if (dst_type.sign) {
>               if (util_cpu_caps.has_sse2) {
> -              intrinsic = "llvm.x86.sse2.packsswb.128";
> -            } else if (util_cpu_caps.has_altivec) {
> -              intrinsic = "llvm.ppc.altivec.vpkshss";
> +               intrinsic = "llvm.x86.sse2.packsswb.128";
> +            }
> +            else if (util_cpu_caps.has_altivec) {
> +               intrinsic = "llvm.ppc.altivec.vpkshss";
>   #ifdef PIPE_ARCH_LITTLE_ENDIAN
> -              swap_intrinsic_operands = TRUE;
> +               swap_intrinsic_operands = TRUE;
>   #endif
>               }
> -         } else {
> +         }
> +         else {
>               if (util_cpu_caps.has_sse2) {
> -              intrinsic = "llvm.x86.sse2.packuswb.128";
> -            } else if (util_cpu_caps.has_altivec) {
> -	      intrinsic = "llvm.ppc.altivec.vpkshus";
> +               intrinsic = "llvm.x86.sse2.packuswb.128";
> +            }
> +            else if (util_cpu_caps.has_altivec) {
> +               intrinsic = "llvm.ppc.altivec.vpkshus";
>   #ifdef PIPE_ARCH_LITTLE_ENDIAN
> -              swap_intrinsic_operands = TRUE;
> +               swap_intrinsic_operands = TRUE;
>   #endif
>               }
>            }
> @@ -516,7 +521,8 @@ lp_build_pack2(struct gallivm_state *gallivm,
>               LLVMTypeRef intr_vec_type = lp_build_vec_type(gallivm, intr_type);
>               if (swap_intrinsic_operands) {
>                  res = lp_build_intrinsic_binary(builder, intrinsic, intr_vec_type, hi, lo);
> -            } else {
> +            }
> +            else {
>                  res = lp_build_intrinsic_binary(builder, intrinsic, intr_vec_type, lo, hi);
>               }
>               if (dst_vec_type != intr_vec_type) {
>