[Mesa-dev] [PATCH 2/5] glsl: Refactor implicit conversion into its own helper

[Timothy Arceri]

On Wed, 2016-08-03 at 23:51 +0300, Andres Gomez wrote:
> Signed-off-by: Andres Gomez <agomez at igalia.com>
> ---
>  src/compiler/glsl/ast_function.cpp | 79 +++++++++++++++++++++++-----
> ----------
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp
> b/src/compiler/glsl/ast_function.cpp
> index 9dcec50..567ad6e 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -836,6 +836,47 @@ convert_component(ir_rvalue *src, const
> glsl_type *desired_type)
>     return (constant != NULL) ? (ir_rvalue *) constant : (ir_rvalue
> *) result;
>  }
>  
> +
> +/**
> + * Perform automatic type conversion of constructor parameters
> + *
> + * This implements the rules in the "Implicit Conversions" rules,
> not the
> + * "Conversion and Scalar Constructors".
> + *
> + * \param from   Operand that is being converted
> + * \param to     Base type the operand will be converted to
> + * \param dst    Destination in which to store the new operand
> + * \param state  GLSL compiler state
> + *
> + * \return
> + * If a conversion is possible (or unnecessary), \c true is
> returned.
> + * Otherwise \c false is returned.
> + */
> +static bool
> +implicitly_convert_component(ir_rvalue * from,
> +                             const glsl_base_type to,
> +                             ir_rvalue * &dst,
> +                             struct _mesa_glsl_parse_state *state)
> +{
      If you also add the IR replacement to this helper you can add
this here too.

      if (from->type->base_type != constructor_type->base_type) {

> +   const glsl_type *desired_type =
> +      glsl_type::get_instance(to,
> +                              from->type->vector_elements,
> +                              from->type->matrix_columns);
> +
> +   if (!from->type->can_implicitly_convert_to(desired_type, state))
> +      return false;
> +
> +   /* Even though convert_component() implements the constructor
> +    * conversion rules (not the implicit conversion rules), its safe
> +    * to use it here because we already checked that the implicit
> +    * conversion is legal.
> +    */
> +   dst = convert_component(from, desired_type);

     }

I think you should be able to take this helper further and add the
constant evaluation logic and IR replacement here also. 

      /* Attempt to convert the parameter to a constant valued
expression.
       * After doing so, track whether or not all the parameters to the
       * constructor are trivially constant valued expressions.
       */
      ir_rvalue *const constant = dst->constant_expression_value();

      if (constant != NULL)
         dst = constant;

      from->replace_with(dst);

      return constant != NULL;
> +}
> +
> +
>  /**
>   * Dereference a specific component from a scalar, vector, or matrix
>   */
> @@ -922,20 +963,9 @@ process_vec_mat_constructor(exec_list
> *instructions,
>  
>        /* Apply implicit conversions (not the scalar constructor
> rules!). See
>         * the spec quote above. */
> -      if (constructor_type->base_type != result->type->base_type) {
> -         const glsl_type *desired_type =
> -            glsl_type::get_instance(constructor_type->base_type,
> -                                    ir->type->vector_elements,;
> -                                    ir->type->matrix_columns);
> -         if (result->type->can_implicitly_convert_to(desired_type,
> state)) {
> -            /* Even though convert_component() implements the
> constructor
> -             * conversion rules (not the implicit conversion rules),
> its safe
> -             * to use it here because we already checked that the
> implicit
> -             * conversion is legal.
> -             */
> -            result = convert_component(ir, desired_type);
> -         }
> -      }
> +      if (constructor_type->base_type != result->type->base_type)
> +         implicitly_convert_component(ir, constructor_type-
> >base_type, result,
> +                                      state);

If there is more than one textual line after and if we always add
braces.

If you follow my suggestions above you can remove the if and just have:

all_parameters_are_constant &= implicitly_convert_component(...)

And you can remove the constant processing code:

      ir_rvalue *const constant = result->constant_expression_value();

      if (constant != NULL)
         result = constant;
      else
         all_parameters_are_constant = false;

      ir->replace_with(result);

>  
>        if (constructor_type->is_matrix()) {
>           if (result->type != constructor_type->column_type()) {
> @@ -1059,26 +1089,11 @@ process_array_constructor(exec_list
> *instructions,
>     foreach_in_list_safe(ir_rvalue, ir, &actual_parameters) {
>        ir_rvalue *result = ir;
>  
> -      const glsl_base_type element_base_type =
> -         constructor_type->fields.array->base_type;
> -
>        /* Apply implicit conversions (not the scalar constructor
> rules!). See
>         * the spec quote above. */
> -      if (element_base_type != result->type->base_type) {
> -         const glsl_type *desired_type =
> -            glsl_type::get_instance(element_base_type,
> -                                    ir->type->vector_elements,
> -                                    ir->type->matrix_columns);
> -
> -	 if (result->type->can_implicitly_convert_to(desired_type,
> state)) {
> -	    /* Even though convert_component() implements the
> constructor
> -	     * conversion rules (not the implicit conversion rules),
> its safe
> -	     * to use it here because we already checked that the
> implicit
> -	     * conversion is legal.
> -	     */
> -	    result = convert_component(ir, desired_type);
> -	 }
> -      }
> +      if (element_type->base_type != result->type->base_type)
> +         implicitly_convert_component(ir, element_type->base_type,
> result,
> +                                      state);

Same comment as above.

>  
>        if (constructor_type->fields.array->is_unsized_array()) {
>           /* As the inner parameters of the constructor are created
> without