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

[Oded Gabbay]

On Mon, Jan 18, 2016 at 12:40 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
> 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
>
No problem, I changed the else location back to original style.

Oded

>
>>
>> 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) {
>>
>