[Mesa-dev] [PATCH 2/2] glsl: disable array splitting for AoA

[Timothy Arceri]

Hi Jason,

I think this patch is exposing a bug in nir_opt_copy_prop_vars with AoA. 
I've tried to figure out what was going on but got a bit lost in that 
code, any ideas/pointers appreciated. An example of the bug can be seen 
with:

ES31-CTS.functional.shaders.arrays_of_arrays.constructor.implicit.int_high_dimensional_array_vertex

The problem is

   intrinsic load_var () (array_ctor at 6[0][0][1][0][0]) ()

gets incorrectly replaced with

   intrinsic load_var () (array_ctor at 6[0][0][0][0][0]) ()

See below.

Before copy prop:

   ...

	vec2 32 ssa_0 = intrinsic load_var () (a_in0) ()
	vec2 32 ssa_1 = f2i32 ssa_0
	vec2 32 ssa_19 = imov ssa_1
	vec2 32 ssa_20 = imov ssa_19
	vec1 32 ssa_3 = imov ssa_20.y
	vec1 32 ssa_21 = imov ssa_3
	vec1 32 ssa_22 = imov ssa_21
	intrinsic store_var (ssa_22) (array_ctor at 0[0][0]) (1) /* wrmask=x */
	intrinsic copy_var () (array_ctor at 1[0][*][*], array_ctor at 0[*][*]) ()
	vec2 32 ssa_23 = imov ssa_19
	vec1 32 ssa_5 = imov ssa_23.x
	vec1 32 ssa_24 = imov ssa_5
	vec1 32 ssa_25 = imov ssa_24
	intrinsic store_var (ssa_25) (array_ctor at 3[0][0]) (1) /* wrmask=x */
	intrinsic copy_var () (array_ctor at 4[0][*][*], array_ctor at 3[*][*]) ()
	intrinsic copy_var () (array_ctor at 5[0][*][*][*], array_ctor at 1[*][*][*]) ()
	intrinsic copy_var () (array_ctor at 5[1][*][*][*], array_ctor at 4[*][*][*]) ()
	intrinsic copy_var () (array_ctor at 6[0][*][*][*][*], 
array_ctor at 5[*][*][*][*]) ()
	intrinsic copy_var () (array_ctor at 7[0][*][*][*][*][*], 
array_ctor at 6[*][*][*][*][*]) ()
	vec1 32 ssa_14 = intrinsic load_var () (array_ctor at 7[0][0][0][0][0][0]) ()
	vec1 32 ssa_26 = imov ssa_14
	vec1 32 ssa_15 = intrinsic load_var () (array_ctor at 7[0][0][1][0][0][0]) ()

...


After copy prop:

   ...

	vec2 32 ssa_0 = intrinsic load_var () (a_in0) ()
	vec2 32 ssa_1 = f2i32 ssa_0
	vec2 32 ssa_19 = imov ssa_1
	vec2 32 ssa_20 = imov ssa_19
	vec1 32 ssa_3 = imov ssa_20.y
	vec1 32 ssa_21 = imov ssa_3
	vec1 32 ssa_22 = imov ssa_21
	intrinsic store_var (ssa_22) (array_ctor at 0[0][0]) (1) /* wrmask=x */
	intrinsic copy_var () (array_ctor at 1[0][*][*], array_ctor at 0[*][*]) ()
	vec2 32 ssa_23 = imov ssa_19
	vec1 32 ssa_5 = imov ssa_23.x
	vec1 32 ssa_24 = imov ssa_5
	vec1 32 ssa_25 = imov ssa_24
	intrinsic store_var (ssa_25) (array_ctor at 3[0][0]) (1) /* wrmask=x */
	intrinsic copy_var () (array_ctor at 4[0][*][*], array_ctor at 3[*][*]) ()
	intrinsic copy_var () (array_ctor at 5[0][*][*][*], array_ctor at 1[*][*][*]) ()
	intrinsic copy_var () (array_ctor at 5[1][*][*][*], array_ctor at 4[*][*][*]) ()
	intrinsic copy_var () (array_ctor at 6[0][*][*][*][*], 
array_ctor at 5[*][*][*][*]) ()
	intrinsic copy_var () (array_ctor at 7[0][*][*][*][*][*], 
array_ctor at 6[*][*][*][*][*]) ()
	vec1 32 ssa_14 = intrinsic load_var () (array_ctor at 6[0][0][0][0][0]) ()
	vec1 32 ssa_26 = imov ssa_14
	vec1 32 ssa_15 = intrinsic load_var () (array_ctor at 6[0][0][0][0][0]) ()






On 25/05/17 12:29, Timothy Arceri wrote:
> While it produces functioning code the pass creates worse code
> for arrays of arrays. See the comment added in this patch for more
> detail.
> ---
>   src/compiler/glsl/opt_array_splitting.cpp | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/src/compiler/glsl/opt_array_splitting.cpp b/src/compiler/glsl/opt_array_splitting.cpp
> index e3073b0..525953f 100644
> --- a/src/compiler/glsl/opt_array_splitting.cpp
> +++ b/src/compiler/glsl/opt_array_splitting.cpp
> @@ -133,20 +133,46 @@ ir_array_reference_visitor::get_variable_entry(ir_variable *var)
>   
>      if (!(var->type->is_array() || var->type->is_matrix()))
>         return NULL;
>   
>      /* If the array hasn't been sized yet, we can't split it.  After
>       * linking, this should be resolved.
>       */
>      if (var->type->is_unsized_array())
>         return NULL;
>   
> +   /* FIXME: arrays of arrays are not handled correctly by this pass so we
> +    * skip it for now. While the pass will create functioning code it actually
> +    * produces worse code.
> +    *
> +    * For example the array:
> +    *
> +    *    int[3][2] a;
> +    *
> +    * ends up being split up into:
> +    *
> +    *    int[3][2] a_0;
> +    *    int[3][2] a_1;
> +    *    int[3][2] a_2;
> +    *
> +    * And we end up referencing each of these new arrays for example:
> +    *
> +    *    a[0][1] will be turned into a_0[0][1]
> +    *    a[1][0] will be turned into a_1[1][0]
> +    *    a[2][0] will be turned into a_2[2][0]
> +    *
> +    * For now we continue to split AoA of matrices to avoid CTS regressions.
> +    */
> +   if (var->type->is_array() && var->type->fields.array->is_array() &&
> +       !var->type->without_array()->is_matrix())
> +      return NULL;
> +
>      foreach_in_list(variable_entry, entry, &this->variable_list) {
>         if (entry->var == var)
>            return entry;
>      }
>   
>      variable_entry *entry = new(mem_ctx) variable_entry(var);
>      this->variable_list.push_tail(entry);
>      return entry;
>   }
>   
>