On Tuesday, July 15, 2014 10:28:07 PM Chris Forbes wrote:
> Accidentally replied privately only, sorry.
>
> ---------- Forwarded message ----------
> From: Chris Forbes <chr...@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 <imir...@alum.mit.edu>
>
>
> On Tue, Jul 15, 2014 at 10:20 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> > On Sat, Jul 12, 2014 at 9:51 PM, Chris Forbes <chr...@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 <chr...@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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
