[Mesa-dev] [PATCH] glsl: fix array assignments of a swizzled vector

[Ian Romanick]

On 10/04/2018 11:25 PM, Ilia Mirkin wrote:
> This happens in situations where we might do
> 
>   vec.wzyx[i] = ...

Ouch.

I haven't looked on the piglit list yet, but is there a piglit test for
this?

> The swizzle would get effectively ignored because of the interaction
> between how ir_assignment->set_lhs works and overwriting the write_mask.
> 
> Fixes the following WebGL test:
> 
> https://www.khronos.org/registry/webgl/sdk/tests/conformance2/glsl3/vector-dynamic-indexing-swizzled-lvalue.html
> 
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/compiler/glsl/lower_vector_derefs.cpp | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compiler/glsl/lower_vector_derefs.cpp b/src/compiler/glsl/lower_vector_derefs.cpp
> index 7583d1fdd3e..af046966491 100644
> --- a/src/compiler/glsl/lower_vector_derefs.cpp
> +++ b/src/compiler/glsl/lower_vector_derefs.cpp
> @@ -59,21 +59,27 @@ vector_deref_visitor::visit_enter(ir_assignment *ir)
>     if (!deref->array->type->is_vector())
>        return ir_rvalue_enter_visitor::visit_enter(ir);
>  
> -   ir_dereference *const new_lhs = (ir_dereference *) deref->array;
> -   ir->set_lhs(new_lhs);
> +   ir_rvalue *const new_lhs = deref->array;
>  
>     void *mem_ctx = ralloc_parent(ir);
>     ir_constant *old_index_constant =
>        deref->array_index->constant_expression_value(mem_ctx);
>     if (!old_index_constant) {
> +      ir->set_lhs(new_lhs);
>        ir->rhs = new(mem_ctx) ir_expression(ir_triop_vector_insert,
>                                             new_lhs->type,
>                                             new_lhs->clone(mem_ctx, NULL),
>                                             ir->rhs,
>                                             deref->array_index);
>        ir->write_mask = (1 << new_lhs->type->vector_elements) - 1;
> +   } else if (new_lhs->ir_type != ir_type_swizzle) {
> +      ir->set_lhs(new_lhs);
> +      ir->write_mask = 1 << old_index_constant->get_uint_component(0);
>     } else {
> -      ir->write_mask = 1 << old_index_constant->get_int_component(0);
> +      // If there "new" lhs is a swizzle, use the set_lhs helper to instead
> +      // swizzle the rhs.

LHS and RHS are initialisms, so they should be capitalized.  We also use
/* */ comments even in C++ code.

Aside from those two tiny nits, this patch is

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

> +      unsigned component[1] = { old_index_constant->get_uint_component(0) };
> +      ir->set_lhs(new(mem_ctx) ir_swizzle(new_lhs, component, 1));
>     }
>  
>     return ir_rvalue_enter_visitor::visit_enter(ir);
>