[Mesa-dev] [PATCH] gallivm: Add SNORM clamping to lp_build_{add, sub}

[Roland Scheidegger]

Am 21.07.2014 17:53, schrieb Richard Sandiford:
> ...fixing the associated TODO.
> 
> Signed-off-by: Richard Sandiford <rsandifo at linux.vnet.ibm.com>
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_arit.c | 34 ++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_arit.c b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> index 3d34144..9ef8628 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_arit.c
> @@ -512,9 +512,20 @@ lp_build_add(struct lp_build_context *bld,
>           return lp_build_intrinsic_binary(builder, intrinsic, lp_build_vec_type(bld->gallivm, bld->type), a, b);
>     }
>  
> -   /* TODO: handle signed case */
> -   if(type.norm && !type.floating && !type.fixed && !type.sign)
> -      a = lp_build_min_simple(bld, a, lp_build_comp(bld, b), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +   if(type.norm && !type.floating && !type.fixed) {
> +      if (type.sign) {
> +         uint64_t sign = (uint64_t)1 << (type.width - 1);
> +         LLVMValueRef max_val = lp_build_const_int_vec(bld->gallivm, type, sign - 1);
> +         LLVMValueRef min_val = lp_build_const_int_vec(bld->gallivm, type, sign);
> +         /* a_clamp_max is the maximum a for positive b,
> +            a_clamp_min is the minimum a for negative b. */
> +         LLVMValueRef a_clamp_max = lp_build_min_simple(bld, a, LLVMBuildSub(builder, max_val, b, ""), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +         LLVMValueRef a_clamp_min = lp_build_max_simple(bld, a, LLVMBuildSub(builder, min_val, b, ""), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +         a = lp_build_select(bld, lp_build_cmp(bld, PIPE_FUNC_GREATER, b, bld->zero), a_clamp_max, a_clamp_min);
> +      } else {
> +         a = lp_build_min_simple(bld, a, lp_build_comp(bld, b), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +      }
> +   }
>  
>     if(LLVMIsConstant(a) && LLVMIsConstant(b))
>        if (type.floating)
> @@ -793,9 +804,20 @@ lp_build_sub(struct lp_build_context *bld,
>           return lp_build_intrinsic_binary(builder, intrinsic, lp_build_vec_type(bld->gallivm, bld->type), a, b);
>     }
>  
> -   /* TODO: handle signed case */
> -   if(type.norm && !type.floating && !type.fixed && !type.sign)
> -      a = lp_build_max_simple(bld, a, b, GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +   if(type.norm && !type.floating && !type.fixed) {
> +      if (type.sign) {
> +         uint64_t sign = (uint64_t)1 << (type.width - 1);
> +         LLVMValueRef max_val = lp_build_const_int_vec(bld->gallivm, type, sign - 1);
> +         LLVMValueRef min_val = lp_build_const_int_vec(bld->gallivm, type, sign);
> +         /* a_clamp_max is the maximum a for negative b,
> +            a_clamp_min is the minimum a for positive b. */
> +         LLVMValueRef a_clamp_max = lp_build_min_simple(bld, a, LLVMBuildAdd(builder, max_val, b, ""), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +         LLVMValueRef a_clamp_min = lp_build_max_simple(bld, a, LLVMBuildAdd(builder, min_val, b, ""), GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +         a = lp_build_select(bld, lp_build_cmp(bld, PIPE_FUNC_GREATER, b, bld->zero), a_clamp_min, a_clamp_max);
> +      } else {
> +         a = lp_build_max_simple(bld, a, b, GALLIVM_NAN_BEHAVIOR_UNDEFINED);
> +      }
> +   }
>  
>     if(LLVMIsConstant(a) && LLVMIsConstant(b))
>        if (type.floating)
> 

This looks alright to me, albeit quite complex. If you don't have the
add/sub clamping intrinsics I guess you don't have the min/max
intrinsics neither, meaning the add will be composed of 1 add, 2 subs, 3
compares, 3 selects. Though it looks like other solutions would be just
as complex.

Reviewed-by: Roland Scheidegger <sroland at vmware.com>