Bug 79373

Summary: Non-const initializers for matrix and vector constructors
Product: Mesa Reporter: Cody Northrop <cody>
Component: glsl-compilerAssignee: Cody Northrop <cody>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: kenneth, mattst88
Version: git   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 77449    

Description Cody Northrop 2014-05-28 14:57:47 UTC
Ran into a problem with ARB_shading_language_420pack on master.  I've got an app that uses non-const initializers for a matrix.  In debug, the compiler aborts during IR validation.  In release, it gets the incorrect answer.

The below patch to an existing piglit test shows the problem.  It may affect other data types, I've only tested with matrices and vectors.

I did enough triage on it to figure out that process_vec_mat_constructor() is trying to index into a vector, and that doesn't get poofed away by lower_vector_insert() before validation.  I don't know if the right answer is to call vector constructors or try to insert with a mask right there.

Anyway, if the original authors could take a look.  The current piglit tests get constant folded away.

Thanks,

-C


diff --git a/tests/spec/arb_shading_language_420pack/execution/aggregate-initializer-matrix.shader_test b/tests/spec/arb_shading_language_420pack/execution/aggregate-initializer-matrix.shader_test
index f5da4c9..0af82f1 100644
--- a/tests/spec/arb_shading_language_420pack/execution/aggregate-initializer-matrix.shader_test
+++ b/tests/spec/arb_shading_language_420pack/execution/aggregate-initializer-matrix.shader_test
@@ -19,9 +19,9 @@ out vec4 color;
 
 void main()
 {
-    mat2x2 a = mat2(  vec2( 1.0, 0.0 ), vec2( 0.0, 1.0 ) );
-    mat2x2 b =      { vec2( 1.0, 0.0 ), vec2( 0.0, 1.0 ) };
-    mat2x2 c =      {     { 1.0, 0.0 },     { 0.0, 1.0 } };
+    mat2x2 a = mat2(  vec2( 1.0, vertex.x ), vec2( 0.0, 1.0 ) );
+    mat2x2 b =      { vec2( 1.0, vertex.x ), vec2( 0.0, 1.0 ) };
+    mat2x2 c =      {     { 1.0, vertex.x },     { 0.0, 1.0 } };
 
     color = vec4(0.0, 1.0, 0.0, 1.0);
Comment 1 Cody Northrop 2014-05-29 15:38:27 UTC
FWIW, I'm using the following patch locally and it works for the tests I've tried it with.  No real piglit regressions (just glean noise that happens on clean driver).


diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
index 4b84470..4995af4 100644
--- a/src/glsl/ast_function.cpp
+++ b/src/glsl/ast_function.cpp
@@ -751,10 +751,20 @@ process_vec_mat_constructor(exec_list *instructions,
    int i = 0;
    foreach_list(node, &actual_parameters) {
       ir_rvalue *rhs = (ir_rvalue *) node;
-      ir_rvalue *lhs = new(ctx) ir_dereference_array(var,
-                                                     new(ctx) ir_constant(i));
+      ir_instruction *assignment = NULL;
+
+      if (var->type->is_array() || var->type->is_matrix()) {
+         ir_rvalue *lhs = new(ctx) ir_dereference_array(var,
+                                                     new(ctx) ir_constant(i));
+         assignment = new(ctx) ir_assignment(lhs, rhs, NULL);
+      } else {
+         /* use writemask rather than index for vector */
+         assert(var->type->is_vector());
+         assert(i < 4);
+         ir_dereference *lhs = new(ctx) ir_dereference_variable(var);
+         assignment = new(ctx) ir_assignment(lhs, rhs, NULL, (unsigned)(1 << i));
+      }
 
-      ir_instruction *assignment = new(ctx) ir_assignment(lhs, rhs, NULL);
       instructions->push_tail(assignment);
 
       i++;
Comment 2 Cody Northrop 2014-07-01 21:31:48 UTC
I sent the proposed fix to mesa-dev:

http://www.mail-archive.com/mesa-dev@lists.freedesktop.org/msg61738.html

And some additional piglit tests that start passing with the patch:

http://lists.freedesktop.org/archives/piglit/2014-July/011559.html
Comment 3 Timothy Arceri 2014-07-19 09:45:21 UTC
author	Cody Northrop <cody@lunarg.com>	2014-07-10 15:55:31 (GMT)
committer	Kenneth Graunke <kenneth@whitecape.org>	2014-07-14 15:36:36 (GMT)
commit	0f679f0ab5afc8c1469453b922d37ae7216136a4 (patch) (side-by-side diff)

glsl: Fix aggregates with dynamic initializers.

Vectors are falling in to the ir_dereference_array() path. 
Without this change, the following glsl aborts the debug driver, or gets the wrong answer in release: 
mat2x2 a = mat2( vec2( 1.0, vertex.x ), vec2( 0.0, 1.0 ) ); 
Also submitting piglit tests, will reference in bug.

v2: Rebase on Mesa master. 
v3: Remove unneeded check for arrays, which are covered by process_array_constructor(), recommended by Timothy Arceri. 

Signed-off-by: Cody Northrop <cody@lunarg.com>
Reviewed-by: Courtney Goeltzenleuchter <courtney@lunarg.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.