[Mesa-dev] [PATCH] llvmpipe: add print for int128

[Jose Fonseca]

Looks good to me in principle, but I have some remarks (inline) concerning the implementation.

Jose

----- Original Message -----
> From: Adhemerval Zanella <azanella at linux.vnet.ibm.com>
> 
> Reviewed-by: Adam Jackson <ajax at redhat.com>
> Signed-off-by: Adhemerval Zanella <azanella at linux.vnet.ibm.com>
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_printf.c | 56
>  +++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_printf.c
> b/src/gallium/auxiliary/gallivm/lp_bld_printf.c
> index 7a6bbd9..71c4d1b 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_printf.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_printf.c
> @@ -83,33 +83,47 @@ lp_build_print_value(struct gallivm_state *gallivm,
>     LLVMTypeKind type_kind;
>     LLVMTypeRef type_ref;
>     LLVMValueRef params[2 + LP_MAX_VECTOR_LENGTH];
> -   char type_fmt[6] = " %x";
> +   char type_fmt[20];
>     char format[2 + 5 * LP_MAX_VECTOR_LENGTH + 2] = "%s";
> -   unsigned length;
> +   unsigned vecsize;
> +   unsigned nargs;
>     unsigned i;
>  
>     type_ref = LLVMTypeOf(value);
>     type_kind = LLVMGetTypeKind(type_ref);
>  
>     if (type_kind == LLVMVectorTypeKind) {
> -      length = LLVMGetVectorSize(type_ref);
> +      vecsize = LLVMGetVectorSize(type_ref);
> +      nargs = vecsize;
>  
>        type_ref = LLVMGetElementType(type_ref);
>        type_kind = LLVMGetTypeKind(type_ref);
> +   } else if (LLVMGetIntTypeWidth(type_ref) == 128) {
> +      vecsize = 1;
> +      nargs = 2;
>     } else {
> -      length = 1;
> +      vecsize = 1;
> +      nargs = 1;
>     }
>  
>     if (type_kind == LLVMFloatTypeKind || type_kind == LLVMDoubleTypeKind) {
> -      type_fmt[2] = '.';
> -      type_fmt[3] = '9';
> -      type_fmt[4] = 'g';
> -      type_fmt[5] = '\0';
> +      snprintf(type_fmt, sizeof type_fmt, " %%9g");

The "." is missing here.

>     } else if (type_kind == LLVMIntegerTypeKind) {
> -      if (LLVMGetIntTypeWidth(type_ref) == 8) {
> -         type_fmt[2] = 'u';
> -      } else {
> -         type_fmt[2] = 'i';
> +      unsigned typeWidth = LLVMGetIntTypeWidth(type_ref);
> +      if (LLVMGetIntTypeWidth(type_ref) <= 32) {
> +         snprintf(type_fmt, sizeof type_fmt, " %%x");

This doesn't look equivalent to me neither. Please retain previous behavior for integers <= 32.

> +      } else if (typeWidth == 64) {
> +#if __WORDSIZE == 64

Is __WORDSIZE standard? I'm particularly concerned about windows. You could just use if (sizeof (unsigned)) here and avoid magic macro.

Or better, you could just use inttypes.h macros, which are also defined for MSVC.

> +         snprintf(type_fmt, sizeof type_fmt, " %%016lx");
> +#else
> +         snprintf(type_fmt, sizeof type_fmt, " %%016llx");
> +#endif
> +      } else if (typeWidth == 128) {
> +#if __WORDSIZE == 64
> +         snprintf(type_fmt, sizeof type_fmt, " %%016lx%%016lx");
> +#else
> +         snprintf(type_fmt, sizeof type_fmt, " %%016llx%%016llx");
> +#endif
>        }
>     } else {
>        /* Unsupported type */
> @@ -117,14 +131,22 @@ lp_build_print_value(struct gallivm_state *gallivm,
>     }
>  
>     /* Create format string and arguments */
> -   assert(strlen(format) + strlen(type_fmt) * length + 2 <= sizeof format);
> +   assert(strlen(format) + strlen(type_fmt) * nargs + 2 <= sizeof format);
>  
>     params[1] = lp_build_const_string(gallivm, msg);
> -   if (length == 1) {
> +   if (vecsize == 1) {
>        util_strncat(format, type_fmt, sizeof(format) - strlen(format) - 1);
> -      params[2] = value;
> +      if (LLVMGetIntTypeWidth(type_ref) <= 64) {
> +         params[2] = value;
> +      } else {
> +         LLVMValueRef shift =
> LLVMConstInt(LLVMIntTypeInContext(gallivm->context, 128), 64, 0);
> +         LLVMValueRef lshr = LLVMBuildLShr(builder, value, shift, "");
> +         LLVMTypeRef type64 = LLVMInt64TypeInContext(gallivm->context);
> +         params[2] = LLVMBuildTrunc(builder, lshr, type64, "");
> +         params[3] = LLVMBuildTrunc(builder, value, type64, "");
> +      }
>     } else {
> -      for (i = 0; i < length; ++i) {
> +      for (i = 0; i < vecsize; ++i) {
>           LLVMValueRef param;
>           util_strncat(format, type_fmt, sizeof(format) - strlen(format) -
>           1);
>           param = LLVMBuildExtractElement(builder, value,
>           lp_build_const_int32(gallivm, i), "");
> @@ -144,7 +166,7 @@ lp_build_print_value(struct gallivm_state *gallivm,
>     util_strncat(format, "\n", sizeof(format) - strlen(format) - 1);
>  
>     params[0] = lp_build_const_string(gallivm, format);
> -   return lp_build_print_args(gallivm, 2 + length, params);
> +   return lp_build_print_args(gallivm, 2 + nargs, params);
>  }
>  
>  
> --
> 1.7.11.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>