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

[Ilia Mirkin]

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?

> +         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.

> +         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?

> +      }
>     }
>
>     return visit_continue_with_parent;
> --
> 2.0.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev