[Mesa-dev] [PATCH] gallivm: fix SCALED -> NORM conversions

[Jose Fonseca]

On 17/06/14 13:58, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> Such conversions (which are most likely rather pointless in practice) were
> resulting in shifts with negative shift counts and shifts with counts the same
> as the bit width. This was always undefined in llvm, the code generated was
> rather horrendous but happened to work.
> So make sure such shifts are filtered out and replaced with something that
> works (the generated code is still just as horrendous as before).
>
> This fixes lp_test_format, https://bugs.freedesktop.org/show_bug.cgi?id=73846.
> ---
>   src/gallium/auxiliary/gallivm/lp_bld_conv.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_conv.c b/src/gallium/auxiliary/gallivm/lp_bld_conv.c
> index d3bf621..fe8f08b 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_conv.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_conv.c
> @@ -794,7 +794,8 @@ lp_build_conv(struct gallivm_state *gallivm,
>         unsigned dst_offset = lp_const_offset(dst_type);
>   
>         /* Compensate for different offsets */
> -      if (dst_offset > src_offset && src_type.width > dst_type.width) {
> +      /* sscaled -> unorm and similar would cause negative shift count, skip */
> +      if (dst_offset > src_offset && src_type.width > dst_type.width && src_shift > 0) {
>            for (i = 0; i < num_tmps; ++i) {
>               LLVMValueRef shifted;
>               LLVMValueRef shift = lp_build_const_int_vec(gallivm, tmp_type, src_shift - 1);
> @@ -903,11 +904,24 @@ lp_build_conv(struct gallivm_state *gallivm,
>   
>          if (src_shift < dst_shift) {
>             LLVMValueRef pre_shift[LP_MAX_VECTOR_LENGTH];
> -          LLVMValueRef shift = lp_build_const_int_vec(gallivm, tmp_type, dst_shift - src_shift);
>   
> -          for (i = 0; i < num_tmps; ++i) {
> -             pre_shift[i] = tmp[i];
> -             tmp[i] = LLVMBuildShl(builder, tmp[i], shift, "");
> +          if (dst_shift - src_shift < dst_type.width) {
> +             LLVMValueRef shift = lp_build_const_int_vec(gallivm, tmp_type,
> +                                                         dst_shift - src_shift);
> +             for (i = 0; i < num_tmps; ++i) {
> +                pre_shift[i] = tmp[i];
> +                tmp[i] = LLVMBuildShl(builder, tmp[i], shift, "");
> +             }
> +          }
> +          else {
> +             /*
> +              * This happens for things like sscaled -> unorm conversions.
> +              * Negative shift counts cause undefined results, so hack around it.
> +              */
> +             for (i = 0; i < num_tmps; ++i) {
> +                pre_shift[i] = tmp[i];
> +                tmp[i] = lp_build_zero(gallivm, dst_type);
> +             }
>             }
>   
>             /* Compensate for different offsets */

Looks okay.  Though I wonder if we could simplify the code while we are 
at it, but leveraging the lp_build_shl_imm/lp_build_shr_imm helpers.

Jose