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

[Ilia Mirkin]

On Sat, Oct 6, 2018 at 1:24 AM Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>
> This happens in situations where we might do
>
>   vec.wzyx[i] = ...
>
> The swizzle would get effectively ignored because of the interaction
> between how ir_assignment->set_lhs works and overwriting the write_mask.
> There are two cases, one where i is a constant, and another where i is
> variable. We have to be extra-careful in both cases.
>
> Fixes the following WebGL test:
>
>   https://www.khronos.org/registry/webgl/sdk/tests/conformance2/glsl3/vector-dynamic-indexing-swizzled-lvalue.html
>
> And the new piglit tests:
>
>   swizzled-writemask-indexing-nonconst.shader_test
>   swizzled-writemask-indexing.shader_test
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>
> v1 -> v2:
>  - include info about piglit tests
>  - add a fix for the nonconst case -- doing set_lhs last instead of
>    first.
>
> Ian Romanick reviewed v1, but given the subtlety of the additional
> change for v2, going to leave that off, prefering another review.
>
> No regressions running this through Intel CI.
>
>  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..223e420550b 100644
> --- a/src/compiler/glsl/lower_vector_derefs.cpp
> +++ b/src/compiler/glsl/lower_vector_derefs.cpp
> @@ -59,8 +59,7 @@ 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 =
> @@ -72,8 +71,15 @@ vector_deref_visitor::visit_enter(ir_assignment *ir)
>                                             ir->rhs,
>                                             deref->array_index);
>        ir->write_mask = (1 << new_lhs->type->vector_elements) - 1;
> +      ir->set_lhs(new_lhs);
> +   } 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.

Urgh, forgot to fix this based on Ian's feedback last time. This now reads

      /* If there "new" LHS is a swizzle, use the set_lhs helper to instead
       * swizzle the RHS.
       */

locally.

> +      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);
> --
> 2.16.4
>