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@6[0][0][1][0][0]) ()
gets incorrectly replaced with
intrinsic load_var () (array_ctor@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@0[0][0]) (1) /* wrmask=x */
intrinsic copy_var () (array_ctor@1[0][*][*], array_ctor@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@3[0][0]) (1) /* wrmask=x */
intrinsic copy_var () (array_ctor@4[0][*][*], array_ctor@3[*][*]) ()
intrinsic copy_var () (array_ctor@5[0][*][*][*], array_ctor@1[*][*][*])
()
intrinsic copy_var () (array_ctor@5[1][*][*][*], array_ctor@4[*][*][*])
()
intrinsic copy_var () (array_ctor@6[0][*][*][*][*],
array_ctor@5[*][*][*][*]) ()
intrinsic copy_var () (array_ctor@7[0][*][*][*][*][*],
array_ctor@6[*][*][*][*][*]) ()
vec1 32 ssa_14 = intrinsic load_var () (array_ctor@7[0][0][0][0][0][0])
()
vec1 32 ssa_26 = imov ssa_14
vec1 32 ssa_15 = intrinsic load_var () (array_ctor@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@0[0][0]) (1) /* wrmask=x */
intrinsic copy_var () (array_ctor@1[0][*][*], array_ctor@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@3[0][0]) (1) /* wrmask=x */
intrinsic copy_var () (array_ctor@4[0][*][*], array_ctor@3[*][*]) ()
intrinsic copy_var () (array_ctor@5[0][*][*][*], array_ctor@1[*][*][*])
()
intrinsic copy_var () (array_ctor@5[1][*][*][*], array_ctor@4[*][*][*])
()
intrinsic copy_var () (array_ctor@6[0][*][*][*][*],
array_ctor@5[*][*][*][*]) ()
intrinsic copy_var () (array_ctor@7[0][*][*][*][*][*],
array_ctor@6[*][*][*][*][*]) ()
vec1 32 ssa_14 = intrinsic load_var () (array_ctor@6[0][0][0][0][0]) ()
vec1 32 ssa_26 = imov ssa_14
vec1 32 ssa_15 = intrinsic load_var () (array_ctor@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;
}
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
