[Mesa-dev] [PATCH 2/5] glsl: Fix implicit conversions in non-constructor function calls

[Kenneth Graunke]

On 07/27/2011 10:55 PM, Chad Versace wrote:
> Context
> -------
> In ast_function_expression::hir(), parameter_lists_match() checks if the
> function call's actual parameter list matches the signature's parameter
> list, where the match may require implicit conversion of some arguments.
> To check if an implicit conversion exists between individual arguments,
> type_compare() is used.
> 
> Problems
> --------
> type_compare() allowed the following illegal implicit conversions:
>     bool -> float
>     bvecN -> vecN
> 
>     int -> uint
>     ivecN -> uvecN
> 
>     uint -> int
>     uvecN -> ivecN
> 
> Change
> ------
> type_compare() is buggy, so replace it with glsl_type::can_be_implicitly_converted_to().
> This comprises a rewrite of parameter_lists_match().
> 
> Fixes Piglit spec/glsl-1.20/compiler/built-in-functions/outerProduct-bvec*.vert
> 
> Note: This is a candidate for the 7.10 and 7.11 branches.
> CC: Ian Romanick <ian.d.romanick at intel.com>
> CC: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Chad Versace <chad at chad-versace.us>
> ---
>  src/glsl/glsl_types.cpp  |   23 +++----------------
>  src/glsl/ir_function.cpp |   52 +++++++++++++++++++++++++++++----------------
>  2 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> index 9cfdcab..3b97aa9 100644
> --- a/src/glsl/glsl_types.cpp
> +++ b/src/glsl/glsl_types.cpp
> @@ -531,25 +531,10 @@ glsl_type::can_implicitly_convert_to(const glsl_type *desired) const
>        return true;
>  
>     /* There is no conversion among matrix types. */
> -   if (this->matrix_columns != 1 || desired->matrix_columns != 1)
> +   if (this->matrix_columns > 1 || desired->matrix_columns > 1)
>        return false;
>  
> -   switch (desired->base_type) {
> -   case GLSL_TYPE_UINT:
> -   case GLSL_TYPE_INT:
> -   case GLSL_TYPE_BOOL:
> -   case GLSL_TYPE_SAMPLER:
> -   case GLSL_TYPE_STRUCT:
> -   case GLSL_TYPE_VOID:
> -   case GLSL_TYPE_ERROR:
> -      return false;
> -
> -   case GLSL_TYPE_FLOAT:
> -      return this->is_integer()
> -	     && this->vector_elements == desired->vector_elements;
> -
> -   default:
> -      assert(0);
> -      return false;
> -   }
> +   return desired->is_float()
> +          && this->is_integer()
> +          && this->vector_elements == desired->vector_elements;
>  }

I think you meant to merge this into the previous patch.  Please do so.

> diff --git a/src/glsl/ir_function.cpp b/src/glsl/ir_function.cpp
> index 0f2f1a0..56d50a8 100644
> --- a/src/glsl/ir_function.cpp
> +++ b/src/glsl/ir_function.cpp
> @@ -85,12 +85,25 @@ type_compare(const glsl_type *a, const glsl_type *b)
>  }
>  
>  
> +/**
> + * \brief Check if two parameter lists match.
> + *
> + * \param list_a Parameters of the function definition.
> + * \param list_b Actual parameters passed to the function.
> + * \return If an exact match, return 0.
> + *         If an inexact match requiring implicit conversion, return 1.
> + *         If not a match, return -1.
> + * \see matching_signature()
> + */
>  static int
>  parameter_lists_match(const exec_list *list_a, const exec_list *list_b)
>  {
>     const exec_node *node_a = list_a->head;
>     const exec_node *node_b = list_b->head;
> -   int total_score = 0;
> +
> +   /* This is set to true if there is an inexact match requiring an implicit
> +    * conversion. */
> +   bool inexact_match = false;
>  
>     for (/* empty */
>  	; !node_a->is_tail_sentinel()
> @@ -106,12 +119,11 @@ parameter_lists_match(const exec_list *list_a, const exec_list *list_b)
>        const ir_variable *const param = (ir_variable *) node_a;
>        const ir_instruction *const actual = (ir_instruction *) node_b;
>  
> -      /* Determine whether or not the types match.  If the types are an
> -       * exact match, the match score is zero.  If the types don't match
> -       * but the actual parameter can be coerced to the type of the declared
> -       * parameter, the match score is one.
> -       */
> -      int score;
> +      if (param->type == actual->type)
> +	 continue;
> +      /* Try to find an implicit conversion from actual to param. */
> +      inexact_match = true;
>        switch ((enum ir_variable_mode)(param->mode)) {
>        case ir_var_auto:
>        case ir_var_uniform:
> @@ -125,29 +137,28 @@ parameter_lists_match(const exec_list *list_a, const exec_list *list_b)
>  
>        case ir_var_const_in:
>        case ir_var_in:
> -	 score = type_compare(param->type, actual->type);
> -	 break;
> +	 if (actual->type->can_implicitly_convert_to(param->type))
> +	    continue;
> +	 else
> +	    return -1;

I think this would be easier to read in the more conventional form:

+	 if (!actual->type->can_implicitly_convert_to(param->type))
+	    return -1;
+	 break;

>        case ir_var_out:
> -	 score = type_compare(actual->type, param->type);
> -	 break;
> +	 if (param->type->can_implicitly_convert_to(actual->type))
> +	    continue;
> +	 else
> +	    return -1;

Ditto.

>        case ir_var_inout:
>  	 /* Since there are no bi-directional automatic conversions (e.g.,
>  	  * there is int -> float but no float -> int), inout parameters must
>  	  * be exact matches.
>  	  */
> -	 score = (type_compare(actual->type, param->type) == 0) ? 0 : -1;
> -	 break;
> +	 return -1;
>  
>        default:
>  	 assert(false);
> -      }
> -
> -      if (score < 0)
>  	 return -1;
> -
> -      total_score += score;
> +      }
>     }
>  
>     /* If all of the parameters from the other parameter list have been
> @@ -157,7 +168,10 @@ parameter_lists_match(const exec_list *list_a, const exec_list *list_b)
>     if (!node_b->is_tail_sentinel())
>        return -1;
>  
> -   return total_score;
> +   if (inexact_match)
> +      return 1;
> +   else
> +      return 0;
>  }

Otherwise, looks good to me.