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

[Ilia Mirkin]

On Tue, Jul 15, 2014 at 6:28 AM, Chris Forbes <chrisf at ijw.co.nz> wrote:
> 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.

Ah right. I got fooled into thinking it was C by the "declare variable
not-in-for loop" thing, which seems to be the style elsewhere in the
file too. This is fine as-is then.

>
>>
>>> +         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 :)
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev