[Mesa-dev] [PATCH] glsl: Fix handling of array dereferences of vectors in opt_dead_code_local

[Ian Romanick]

On 02/08/2013 10:41 PM, Eric Anholt wrote:
> Ian Romanick <idr at freedesktop.org> writes:
>
>> On 02/08/2013 04:24 PM, Eric Anholt wrote:
>>> Ian Romanick <idr at freedesktop.org> writes:
>>>
>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>> + * modes.  In the first mode, when \c want_availability_mask is \c true, it is
>>>> + * assumed that any field of the vector may be written.  This is used when
>>>> + * adding an assignment to the assignment list.  In the second mode, when
>>>> + * \c want_availability_mask is \c false, it is assumed that no fields of the
>>>> + * vector are written.  This is used when trying to remove earlier assignments
>>>> + * from the list.
>>>> + */
>>>> +static int
>>>> +may_set_mask(ir_assignment *ir, bool want_availability_mask)
>>>> +{
>>>> +   int mask = ir->write_mask;
>>>> +
>>>> +   /* If the LHS is an array derefernce of a vector, try to figure out what
>>>> +    * the real write mask is.  If the index is not a constant, assume that any
>>>> +    * element may be written.
>>>> +    */
>>>> +   if (is_array_deref_of_vector(ir->lhs)) {
>>>> +      ir_dereference_array *const d = ir->lhs->as_dereference_array();
>>>> +      ir_constant *const c = d->array_index->as_constant();
>>>> +
>>>> +      if (c != NULL) {
>>>> +         const int idx = (c != NULL) ? c->get_uint_component(0) : -1;
>>>> +
>>>> +         if (idx >= 0 && idx <= 3)
>>>> +            mask = 1U << idx;
>>>> +         else
>>>> +            mask = 0;
>>>> +      } else {
>>>> +         /* Set the write-mask depending on the size of the vector.
>>>> +          */
>>>> +         if (want_availability_mask)
>>>> +            mask = (1U << d->array->type->vector_components()) - 1;
>>>> +         else
>>>> +            mask = 0;
>>>> +      }
>>>> +   }
>>>> +
>>>> +   return mask;
>>>> +}
>>>
>>>
>>>> +
>>>>    class assignment_entry : public exec_node
>>>>    {
>>>>    public:
>>>> @@ -51,7 +114,7 @@ public:
>>>>          assert(ir);
>>>>          this->lhs = lhs;
>>>>          this->ir = ir;
>>>> -      this->available = ir->write_mask;
>>>> +      this->available = may_set_mask(ir, true);
>>>
>>> I don't think the want_availability_mask makes sense. If you're noting
>>> an array deref of a vector for later dead code elimination, a later
>>> instruction that sets the vector's .x shouldn't go turn off .x of this
>>> instruction.
>>
>> I did it like this so that we could still optimize things like:
>>
>>       vec4 v = ...;
>>
>>       v[i] =  ...;   // i is a uniform or similar
>>       ...
>>       v.x = ...;
>>       ...
>>       v.y = ...;
>>       ...
>>       v.z = ...;
>>       ...
>>       v.w = ...;
>>
>> Even though the initial assignment(s) set "any field", the later
>> assignments wipe them all out.
>
> But for code that's just:
>
>>       v[i] =  ...;   // i is a uniform or similar
>>       ...
>>       v.x = ...;
>
> you'll turn off the write_mask of the v[i] write, right?  And now the
> read of v.yzw is broken.

I sent out a test case for this this very case.  See "[PATCH v2] Test 
bad interaction with optimizer and "array" accesses to vector elements" 
on the piglit list.  The glsl-vs-channel-overwrite-03.shader_test case 
is the one that checks this:

#version 120
attribute vec3 vertex;
uniform mat4 mvp = mat4(1.);
uniform int i = 3;

void main()
{
     vec4 tmp;

     /* These two blocks of code should produce the same result, but or some
      * reason the tmp[3] assignment in the first version gets eliminated by
      * one of Mesa's optimization passes.
      */
#if 1
     tmp[i] = 1.0;
     tmp.xyz = vertex;
#else
     tmp.w = 1.0;
     tmp.xyz = vertex;
#endif
     gl_Position = mvp * tmp;
}

For instructions with variable indexing, I change the available mask, 
but I only modify the instruction when the available mask becomes zero. 
  This is handled by the last hunk in the patch (with the "give up" 
comment).