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

[Ian Romanick]

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>
>>
>> Three parts to the fix:
>>
>> 1. If the array dereference is a constant index, figure out the real
>> write mask.
>>
>> 2. If the array dereference not is a constant index, assume the
>> assignment may generate any component.
>>
>> 3. If the array dereference not is a constant index, assume the
>> assigment will not kill any previous component write.
>
> Man I wish we didn't leave vector array derefs in the GLSL IR.  Or
> rather, I wish we didn't try optimizing on this IR.
>
>> -static bool debug = false;
>> +static bool debug = true;
>
> left the debug on.

I fixed that in v2 of the patch that I sent a day or two after v1.

>>
>>   namespace {
>>
>> +static bool
>> +is_array_deref_of_vector(ir_rvalue *ir)
>> +{
>> +   if (ir->ir_type == ir_type_dereference_array) {
>> +      ir_dereference_array *const d = ir->as_dereference_array();
>> +
>> +      assert(d != NULL);
>
> Or just make the if statement "if (d)" and drop the assert.
>
>> + * a vector, the \c write_mask of the assignment is returned.  If the LHS of
>> + * the assignment is an array dereference, the write mask is a lie.  The
>> + * actual write mask is determined by the array index.  If the array index is
>> + * a constant, calculate the mask from that constant.
>
>> + * If the index is not a constant, the function can operation in one of two
>
>                                                         operate
>
>> + * 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.