[Mesa-dev] Fwd: [PATCH 2/6] glsl: Mark entire UBO array active if indexed with non-constant.

[Chris Forbes]

Accidentally replied privately only, sorry.

---------- Forwarded message ----------
From: Chris Forbes <chrisf at ijw.co.nz>
Date: Tue, Jul 15, 2014 at 10:27 PM
Subject: Re: [Mesa-dev] [PATCH 2/6] glsl: Mark entire UBO array active
if indexed with non-constant.
To: Ilia Mirkin <imirkin at alum.mit.edu>


On Tue, Jul 15, 2014 at 10:20 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Sat, Jul 12, 2014 at 9:51 PM, Chris Forbes <chrisf at ijw.co.nz> wrote:
>> Without doing a lot more work, we have no idea which indices may
>> be used at runtime, so just mark them all.
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> ---
>>  src/glsl/link_uniform_block_active_visitor.cpp | 51 ++++++++++++++++----------
>>  1 file changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/glsl/link_uniform_block_active_visitor.cpp b/src/glsl/link_uniform_block_active_visitor.cpp
>> index d19ce20..4bf7349 100644
>> --- a/src/glsl/link_uniform_block_active_visitor.cpp
>> +++ b/src/glsl/link_uniform_block_active_visitor.cpp
>> @@ -109,32 +109,45 @@ link_uniform_block_active_visitor::visit_enter(ir_dereference_array *ir)
>>     assert((b->num_array_elements == 0) == (b->array_elements == NULL));
>>     assert(b->type != NULL);
>>
>> -   /* Determine whether or not this array index has already been added to the
>> -    * list of active array indices.  At this point all constant folding must
>> -    * have occured, and the array index must be a constant.
>> -    */
>>     ir_constant *c = ir->array_index->as_constant();
>> -   assert(c != NULL);
>>
>> -   const unsigned idx = c->get_uint_component(0);
>> +   if (c) {
>> +      /* Index is a constant, so mark just that element used, if not already */
>> +      const unsigned idx = c->get_uint_component(0);
>>
>> -   unsigned i;
>> -   for (i = 0; i < b->num_array_elements; i++) {
>> -      if (b->array_elements[i] == idx)
>> -        break;
>> -   }
>> +      unsigned i;
>> +      for (i = 0; i < b->num_array_elements; i++) {
>> +         if (b->array_elements[i] == idx)
>> +            break;
>> +      }
>>
>> -   assert(i <= b->num_array_elements);
>> +      assert(i <= b->num_array_elements);
>>
>> -   if (i == b->num_array_elements) {
>> -      b->array_elements = reralloc(this->mem_ctx,
>> -                                  b->array_elements,
>> -                                  unsigned,
>> -                                  b->num_array_elements + 1);
>> +      if (i == b->num_array_elements) {
>> +         b->array_elements = reralloc(this->mem_ctx,
>> +                                      b->array_elements,
>> +                                      unsigned,
>> +                                      b->num_array_elements + 1);
>>
>> -      b->array_elements[b->num_array_elements] = idx;
>> +         b->array_elements[b->num_array_elements] = idx;
>>
>> -      b->num_array_elements++;
>> +         b->num_array_elements++;
>> +      }
>> +   } else {
>> +      /* The array index is not a constant, so mark the entire array used. */
>> +      assert(b->type->is_array());
>> +      if (b->num_array_elements < b->type->length) {
>
> This condition is a little different than the other case. Is this
> basically to cover the first time that the array is indexed
> dynamically?

Right, so if this check fails, then we've already marked every element
and so don't need to do anything.
Otherwise, we need to expand to the full size of the array and mark
everything in one go.

>
>> +         b->num_array_elements = b->type->length;
>> +         b->array_elements = reralloc(this->mem_ctx,
>> +                                      b->array_elements,
>> +                                      unsigned,
>> +                                      b->num_array_elements);
>> +
>> +         unsigned i;
>
> You'll get yelled at by the MSVC people for this... needs to be at the
> beginning of a block.

This file is C++, so that shouldn't be a problem. I'll happily move it
to the start of the block though.

>
>> +         for (i = 0; i < b->num_array_elements; i++) {
>> +            b->array_elements[i] = i;
>> +         }
>
> I think this is fine, but just want to raise the issue of a situation
> where you start out with some const accesses, and then do a nonconst
> access. The nonconst will erase all the old idx's but since they only
> exist in the array_elements list (at this point in the compilation),
> the reassignment won't cause any issues, right?

That's the idea. I think it's safe (or alternatively, I've completely
misunderstood something) but more scrutiny wouldn't hurt :)