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

[Kenneth Graunke]

On Tuesday, July 15, 2014 10:28:07 PM Chris Forbes 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.

Personally, I'd just fold it into the for loop - in the other case, we wanted 
to use 'i' after the loop.  But it doesn't really matter.

> >
> >> +         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 :)

Yeah, this looks fine to me.  It looks like nothing uses this info until the 
pass is completely done, and the pass is just recording what's active in no 
particular order.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140715/3216dac5/attachment.sig>