On Tue, Jul 15, 2014 at 6:28 AM, Chris Forbes <chr...@ijw.co.nz> 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.
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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
