[Mesa-dev] [Mesa-stable] [PATCH 2/2] glsl: Fix chained assignments of vector channels.

[Ian Romanick]

On 01/24/2014 11:48 AM, Kenneth Graunke wrote:
> Simple shaders such as:
> 
>     void splat(vec2 v, float f) {
>         v[0] = v[1] = f;
>     }
> 
> failed to compile with the following error:
> error: value of type vec2 cannot be assigned to variable of type float
> 
> First, we would process v[1] = f, and transform:
> LHS: (expression float vector_extract (var_ref v) (constant int (1)))
> RHS: (var_ref f)
> into:
> LHS: (var_ref v)
> RHS: (expression vec2 vector_insert (var_ref v) (constant int (1))
>                  (var_ref f))
> 
> Note that the LHS type is now vec2, not a float.  This is surprising,
> but not the real problem.
> 
> After emitting assignments, this ultimately becomes:
> (declare (temporary) vec2 assignment_tmp)
> (assign (xy)
>   (var_ref assignment_tmp)
>   (expression vec2 vector_insert (var_ref v) (constant int (1))
>               (var_ref f)))
>   (assign (xy) (var_ref v) (var_ref assignment_tmp))
> 
> We would then return (var_ref assignment_tmp) as the rvalue, which has
> the wrong type---it should be float, but is instead a vec2.
> 
> To fix this, we simply return (vector_extract (var_ref assignment_temp)
> <the appropriate channel>) to pull out the desired float value.
> 
> Fixes Piglit's new chained-assignment-with-array-deref test.

In my updated tests, this is
chained-assignment-with-vector-constant-index.vert and
chained-assignment-with-vector-dynamic-index.vert.

The series is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Cc: idr at freedesktop.org
> Cc: mesa-stable at lists.freedesktop.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74026
> Reported-by: Dan Ginsburg <dang at valvesoftware.com>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/ast_to_hir.cpp | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 67ab09b..1bfb4e5 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -740,6 +740,7 @@ do_assignment(exec_list *instructions, struct _mesa_glsl_parse_state *state,
>  {
>     void *ctx = state;
>     bool error_emitted = (lhs->type->is_error() || rhs->type->is_error());
> +   ir_rvalue *extract_channel = NULL;
>  
>     /* If the assignment LHS comes back as an ir_binop_vector_extract
>      * expression, move it to the RHS as an ir_triop_vector_insert.
> @@ -755,11 +756,23 @@ do_assignment(exec_list *instructions, struct _mesa_glsl_parse_state *state,
>           if (new_rhs == NULL) {
>              return lhs;
>           } else {
> +            /* This converts:
> +             * - LHS: (expression float vector_extract <vec> <channel>)
> +             * - RHS: <scalar>
> +             * into:
> +             * - LHS: <vec>
> +             * - RHS: (expression vec2 vector_insert <vec> <channel> <scalar>)
> +             *
> +             * The LHS type is now a vector instead of a scalar.  Since GLSL
> +             * allows assignments to be used as rvalues, we need to re-extract
> +             * the channel from assignment_temp when returning the rvalue.
> +             */
> +            extract_channel = lhs_expr->operands[1];
>              rhs = new(ctx) ir_expression(ir_triop_vector_insert,
>                                           lhs_expr->operands[0]->type,
>                                           lhs_expr->operands[0],
>                                           new_rhs,
> -                                         lhs_expr->operands[1]);
> +                                         extract_channel);
>              lhs = lhs_expr->operands[0]->clone(ctx, NULL);
>           }
>        }
> @@ -856,6 +869,11 @@ do_assignment(exec_list *instructions, struct _mesa_glsl_parse_state *state,
>     if (!error_emitted)
>        instructions->push_tail(new(ctx) ir_assignment(lhs, deref_var));
>  
> +   if (extract_channel) {
> +      return new(ctx) ir_expression(ir_binop_vector_extract,
> +                                    new(ctx) ir_dereference_variable(var),
> +                                    extract_channel->clone(ctx, NULL));
> +   }
>     return new(ctx) ir_dereference_variable(var);
>  }
>  
>