Bug 53256

Summary: opt_dead_code_local removing assignments to array elements
Product: Mesa Reporter: Chris Wolfe <cwolfe>
Component: glsl-compilerAssignee: Ian Romanick <idr>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: eric, fjhmesabug
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 67224    

Description Chris Wolfe 2012-08-08 16:09:36 UTC
Believe I've run into a GLSL optimizer bug introduced by "glsl: Improve the local dead code optimization to eliminate unused channels" (05c200b). Have whacked together a quick workaround in Chromium OS, but would also like to get your thoughts about an actual solution. Tested with Chrome using mostly-unpatched Mesa git (82cd9c0 + portage).

Given GLSL source for a WebGL vertex shader, as logged by Mesa:

attribute vec3 aVertexPosition;
uniform mat4 uMVMatrix;
uniform mat4 uPMatrix;
void main(){
vec4 tmp;
(tmp[3] = 1.0);
(tmp.xyz = aVertexPosition);
(gl_Position = ((uPMatrix * uMVMatrix) * tmp));
}

Chromium does some mangling to get that exact source; believe only the 'tmp[3] = ...' and 'tmp.xyz = ...' matter.

The opt_dead_code_local tracing yields:
...
(assign  (x) (array_ref (var_ref tmp) (constant int (3)) )  (var_ref assignment_tmp) ) 
kill assignment_tmp (0x1 - 0xffffffff)
add tmp
current entries
    tmp (0x1)
(declare (temporary ) vec3 assignment_tmp)
(assign  (xyz) (var_ref tmp)  (swiz xyz (swiz xyz (var_ref aVertexPosition) ))) 
looking for tmp.0x7 to remove
tmp 0x1 - 0x1 = 0x0
rewriting:
  (assign  (x) (array_ref (var_ref tmp) (constant int (3)) )  (var_ref assignment_tmp) ) 
add tmp
current entries
    tmp (0x7)
...

Because tmp[3] = 1.0 is encoded as an assign with write_mask (x), and this mask is copied into the assignment_entry, it gets blown away by the assignment to (xyz). Expect the same problem will also happen if the 3 is a variable.

As a quick fix in CrOS, I am only adding dereference_variables to the assignment list <https://gerrit.chromium.org/gerrit/#/c/29531/3/media-libs/mesa/files/8.1-dead-code-local-hack.patch>. That's not a long-term fix, but I think it's safest (and fixes the regression). If there's obvious problems with that hack, please yell.

For a real fix, I think we need to generate entry->available more carefully, and use it rather than entry->ir->write_mask. e.g. set 1<<index for small constant array index and ~0 for variable array index (or just ~0 and get it on the next iteration). Feels like that could allow the elimination to work on vectors and most arrays (possibly records; haven't looked yet). Those extra cases make the creation of the assignment_entry a little complex, so I propose generating 'available' in "Add this instruction to the assignment list" and passing it to the constructor explicitly.

If that sounds good I can plow ahead with a patch. Also no objection to leaving it for Eric or company.
Comment 1 Chris Wolfe 2012-08-09 15:41:30 UTC
Re-aiming at the list, guess the CC isn't automatic. Please see the bug for context.

Summary: GLSL optimizer mangles "vec4 tmp; tmp[3] = 1.0; tmp.xyz = some_3d_vector;"

Walked through the optimization behavior in opt_dead_local_code (and friends) again. Feels like the best fix is to consider assignment to a deference_array of a vector as a write to an unknown component.

That way the assignment will get removed only if there is an overriding whole_variable_written to the vector. If the array index is constant it will get converted to a swizzle (vec_index_to_swizzle), and hopefully become a deference_variable for the next pass (haven't found the code for that yet).
Comment 2 Ian Romanick 2013-01-30 01:20:31 UTC
My current guess is the optimization pass is happening before lowering tmp[3] to tmp.w.  I'll look into it.
Comment 3 Ian Romanick 2013-08-08 00:42:41 UTC
I just committed some piglit tests that I wrote back in January.  Since the ir_triop_vector_insert / ir_binop_vector_extract work, all of the piglit tests pass.  I believe this issue is fixed as of (approximately) a61a0dbe.

If the issue persists, please re-open the bug.

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.