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.
>
>> + 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
