On Tue, 2015-09-08 at 08:09 +1000, Timothy Arceri wrote:
> On Mon, 2015-09-07 at 11:24 -0700, Jason Ekstrand wrote:
> > On Tue, Sep 1, 2015 at 7:44 PM, Timothy Arceri <
> > t_arc...@yahoo.com.au>
> > wrote:
> > > This allows the correct offset to be easily calculated for
> > > indirect
> > > indexing when a struct array contains multiple samplers, or any
> > > crazy
> > > nesting.
> > >
> > > The indices for the folling struct will now look like this:
> > > Sampler index: 0 Name: s[0].tex
> > > Sampler index: 1 Name: s[1].tex
> > > Sampler index: 2 Name: s[0].si.tex
> > > Sampler index: 3 Name: s[1].si.tex
> > > Sampler index: 4 Name: s[0].si.tex2
> > > Sampler index: 5 Name: s[1].si.tex2
> > >
> > > Before this change it looked like this:
> > > Sampler index: 0 Name: s[0].tex
> > > Sampler index: 3 Name: s[1].tex
> > > Sampler index: 1 Name: s[0].si.tex
> > > Sampler index: 4 Name: s[1].si.tex
> > > Sampler index: 2 Name: s[0].si.tex2
> > > Sampler index: 5 Name: s[1].si.tex2
> > >
> > > struct S_inner {
> > > sampler2D tex;
> > > sampler2D tex2;
> > > };
> > >
> > > struct S {
> > > sampler2D tex;
> > > S_inner si;
> > > };
> > >
> > > uniform S s[2];
> > >
> > > V2: rename struct array counter to have better name
> > > ---
> > > src/glsl/link_uniforms.cpp | 112
> > > ++++++++++++++++++++++++++++++++++++++--
> > > -----
> > > src/glsl/linker.h | 4 +-
> > > 2 files changed, 98 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/glsl/link_uniforms.cpp
> > > b/src/glsl/link_uniforms.cpp
> > > index 254086d..5402c99 100644
> > > --- a/src/glsl/link_uniforms.cpp
> > > +++ b/src/glsl/link_uniforms.cpp
> > > @@ -28,6 +28,7 @@
> > > #include "glsl_symbol_table.h"
> > > #include "program/hash_table.h"
> > > #include "program.h"
> > > +#include "util/hash_table.h"
> > >
> > > /**
> > > * \file link_uniforms.cpp
> > > @@ -63,14 +64,17 @@ program_resource_visitor::process(const
> > > glsl_type
> > > *type, const char *name)
> > > assert(type->without_array()->is_record()
> > > || type->without_array()->is_interface());
> > >
> > > + unsigned record_array_count = 1;
> > > char *name_copy = ralloc_strdup(NULL, name);
> > > - recursion(type, &name_copy, strlen(name), false, NULL,
> > > false);
> > > + recursion(type, &name_copy, strlen(name), false, NULL, false,
> > > + record_array_count);
> > > ralloc_free(name_copy);
> > > }
> > >
> > > void
> > > program_resource_visitor::process(ir_variable *var)
> > > {
> > > + unsigned record_array_count = 1;
> > > const glsl_type *t = var->type;
> > > const bool row_major =
> > > var->data.matrix_layout == GLSL_MATRIX_LAYOUT_ROW_MAJOR;
> > > @@ -111,7 +115,8 @@ program_resource_visitor::process(ir_variable
> > > *var)
> > > * lowering is only applied to non-uniform interface
> > > blocks, so
> > > we
> > > * can safely pass false for row_major.
> > > */
> > > - recursion(var->type, &name, new_length, row_major,
> > > NULL, false);
> > > + recursion(var->type, &name, new_length, row_major,
> > > NULL, false,
> > > + record_array_count);
> > > }
> > > ralloc_free(name);
> > > } else if (var->data.from_named_ifc_block_nonarray) {
> > > @@ -135,19 +140,23 @@
> > > program_resource_visitor::process(ir_variable *var)
> > > * is only applied to non-uniform interface blocks, so we
> > > can
> > > safely
> > > * pass false for row_major.
> > > */
> > > - recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false);
> > > + recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false,
> > > + record_array_count);
> > > ralloc_free(name);
> > > } else if (t->without_array()->is_record()) {
> > > char *name = ralloc_strdup(NULL, var->name);
> > > - recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false);
> > > + recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false,
> > > + record_array_count);
> > > ralloc_free(name);
> > > } else if (t->is_interface()) {
> > > char *name = ralloc_strdup(NULL, var->type->name);
> > > - recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false);
> > > + recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false,
> > > + record_array_count);
> > > ralloc_free(name);
> > > } else if (t->is_array() && t->fields.array->is_interface())
> > > {
> > > char *name = ralloc_strdup(NULL, var->type->fields.array
> > > ->name);
> > > - recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false);
> > > + recursion(var->type, &name, strlen(name), row_major, NULL,
> > > false,
> > > + record_array_count);
> > > ralloc_free(name);
> > > } else {
> > > this->visit_field(t, var->name, row_major, NULL, false);
> > > @@ -158,7 +167,8 @@ void
> > > program_resource_visitor::recursion(const glsl_type *t, char
> > > **name,
> > > size_t name_length, bool
> > > row_major,
> > > const glsl_type
> > > *record_type,
> > > - bool last_field)
> > > + bool last_field,
> > > + unsigned record_array_count)
> > > {
> > > /* Records need to have each field processed individually.
> > > *
> > > @@ -205,7 +215,7 @@ program_resource_visitor::recursion(const
> > > glsl_type
> > > *t, char **name,
> > > recursion(t->fields.structure[i].type, name,
> > > new_length,
> > > field_row_major,
> > > record_type,
> > > - (i + 1) == t->length);
> > > + (i + 1) == t->length, record_array_count);
> > >
> > > /* Only the first leaf-field of the record gets called
> > > with the
> > > * record type pointer.
> > > @@ -222,6 +232,8 @@ program_resource_visitor::recursion(const
> > > glsl_type
> > > *t, char **name,
> > > if (record_type == NULL && t->fields.array->is_record())
> > > record_type = t->fields.array;
> > >
> > > + record_array_count *= t->length;
> >
> > It's possible that I'm not understanding something, but I don't
> > think
> > this recursion does what you think it does. The only time you ever
> > manipulate record_array_count is to multiply it by an array length
> > so
> > the final record_array_count will be the product of the lengths of
> > all
> > arrays encountered. If you have the following structure:
> >
> > struct S {
> > sampler2D A[5];
> > sampler2D B[3];
> > sampler3D C[2];
> > };
> >
> > Then, by the time you get done with B and on to C, the
> > record_array_count will be 15, not 8 like is what I think you want.
> > Also, nowhere in here do you predicate that on is_sampler() so if I
> > replaced B with an array of floats, you'd still get 15 but I think
> > you
> > would want 5. Am I missing something?
>
> Yes you are ;)
>
> This happens within
>
> if (t->without_array()->is_record() ||
> t->without_array()->is_interface() ||
> (t->is_array() && t->fields.array->is_array()))
Actually in master its currently still:
if (t->is_array() && (t->fields.array->is_record()
|| t->fields.array->is_interface()))
but that same applies just not the stuff I said about arrays of arrays
(well not yet anyway).
I've pushed the piglit tests referenced in the cover letter so feel
free to test out the logic.
>
> So record_array_count (maybe could have a better name) only gets
> increased if
> the struct is an array, or AoA or the member is an AoA.
>
> In your example above record_array_count would remain at the default
> of 1.
>
> For something a little more complex it would end up like this:
>
> struct S [2] {
> sampler2D A[5]; // record_array_count == 2
> sampler2D B[2][3]; // record_array_count == 4
> sampler3D C[2]; // record_array_count == 2
> };
>
> It ends up back at 2 because the recursion is doing its job as we
> pass the
> value of record_array_count rather than a pointer to it in the next
> recusive
> call.
>
> There is no need to predicate on is_sampler() as record_array_count
> is
> essentially an individualised outter array dimension count for each
> struct
> member. Does that make sense?
>
> As a result AoA will also use this count to allocate there space
> later on
> although they don't really need to as we don't need to worry about
> these kind
> of offsets but its just extra code to stop it for no real gain.
>
> We are doing the count for interface too but we just don't use it for
> those.
>
> Does that all make sense?
>
> > --Jason
> >
> > > +
> > > for (unsigned i = 0; i < t->length; i++) {
> > > size_t new_length = name_length;
> > >
> > > @@ -230,7 +242,7 @@ program_resource_visitor::recursion(const
> > > glsl_type
> > > *t, char **name,
> > >
> > > recursion(t->fields.array, name, new_length, row_major,
> > > record_type,
> > > - (i + 1) == t->length);
> > > + (i + 1) == t->length, record_array_count);
> > >
> > > /* Only the first leaf-field of the record gets called
> > > with the
> > > * record type pointer.
> > > @@ -238,6 +250,7 @@ program_resource_visitor::recursion(const
> > > glsl_type
> > > *t, char **name,
> > > record_type = NULL;
> > > }
> > > } else {
> > > + this->set_record_array_count(record_array_count);
> > > this->visit_field(t, *name, row_major, record_type,
> > > last_field);
> > > }
> > > }
> > > @@ -268,6 +281,11 @@ program_resource_visitor::leave_record(const
> > > glsl_type *, const char *, bool)
> > > {
> > > }
> > >
> > > +void
> > > +program_resource_visitor::set_record_array_count(unsigned)
> > > +{
> > > +}
> > > +
> > > namespace {
> > >
> > > /**
> > > @@ -432,6 +450,7 @@ public:
> > > this->next_sampler = 0;
> > > this->next_image = 0;
> > > this->next_subroutine = 0;
> > > + this->record_array_count = 1;
> > > memset(this->targets, 0, sizeof(this->targets));
> > > }
> > >
> > > @@ -440,6 +459,7 @@ public:
> > > {
> > > current_var = var;
> > > field_counter = 0;
> > > + this->record_next_sampler = new string_to_uint_map;
> > >
> > > ubo_block_index = -1;
> > > if (var->is_in_buffer_block()) {
> > > @@ -493,6 +513,8 @@ public:
> > > process(var);
> > > } else
> > > process(var);
> > > + }
> > > + delete this->record_next_sampler;
> > > }
> > >
> > > int ubo_block_index;
> > > @@ -501,17 +523,60 @@ public:
> > >
> > > private:
> > > void handle_samplers(const glsl_type *base_type,
> > > - struct gl_uniform_storage *uniform)
> > > + struct gl_uniform_storage *uniform,
> > > const char
> > > *name)
> > > {
> > > if (base_type->is_sampler()) {
> > > - uniform->sampler[shader_type].index = this
> > > ->next_sampler;
> > > uniform->sampler[shader_type].active = true;
> > >
> > > - /* Increment the sampler by 1 for non-arrays and by the
> > > number
> > > of
> > > - * array elements for arrays.
> > > - */
> > > - this->next_sampler +=
> > > - MAX2(1, uniform->array_elements);
> > > + /* Handle multiple samplers inside struct arrays */
> > > + if (this->record_array_count > 1) {
> > > + unsigned inner_array_size = MAX2(1, uniform
> > > ->array_elements);
> > > + char *name_copy = ralloc_strdup(NULL, name);
> > > +
> > > + /* Remove all array subscripts from the sampler name
> > > */
> > > + char *str_start;
> > > + const char *str_end;
> > > + while((str_start = strchr(name_copy, '[')) &&
> > > + (str_end = strchr(name_copy, ']'))) {
> > > + memmove(str_start, str_end + 1, 1 +
> > > strlen(str_end));
> > > + }
> > > +
> > > + unsigned index = 0;
> > > + if (this->record_next_sampler->get(index,
> > > name_copy)) {
> > > + /* Set the base sampler index for the uniform,
> > > increment
> > > the
> > > + * next index and return as everything else has
> > > already
> > > been
> > > + * initialised.
> > > + */
> > > + uniform->sampler[shader_type].index = index;
> > > + index = inner_array_size + uniform
> > > ->sampler[shader_type].index;
> > > + this->record_next_sampler->put(index, name_copy);
> > > +
> > > + ralloc_free(name_copy);
> > > + return;
> > > + } else {
> > > + /* Nested struct arrays behave like arrays of
> > > arrays so we
> > > need
> > > + * to increase the index by the total number of
> > > elements
> > > of the
> > > + * sampler in case there is more than one sampler
> > > inside
> > > the
> > > + * structs. This allows the offset to be easily
> > > calculated
> > > for
> > > + * indirect indexing.
> > > + */
> > > + uniform->sampler[shader_type].index = this
> > > ->next_sampler;
> > > + this->next_sampler +=
> > > + inner_array_size * this->record_array_count;
> > > +
> > > + /* Store the next index for future passes over
> > > the struct
> > > array
> > > + */
> > > + index = uniform->sampler[shader_type].index +
> > > inner_array_size;
> > > + this->record_next_sampler->put(index, name_copy);
> > > + ralloc_free(name_copy);
> > > + }
> > > + } else {
> > > + /* Increment the sampler by 1 for non-arrays and by
> > > the
> > > number of
> > > + * array elements for arrays.
> > > + */
> > > + uniform->sampler[shader_type].index = this
> > > ->next_sampler;
> > > + this->next_sampler += MAX2(1, uniform
> > > ->array_elements);
> > > + }
> > >
> > > const gl_texture_index target = base_type
> > > ->sampler_index();
> > > const unsigned shadow = base_type->sampler_shadow;
> > > @@ -564,6 +629,11 @@ private:
> > > }
> > > }
> > >
> > > + virtual void set_record_array_count(unsigned
> > > record_array_count)
> > > + {
> > > + this->record_array_count = record_array_count;
> > > + }
> > > +
> > > virtual void visit_field(const glsl_type *type, const char
> > > *name,
> > > bool row_major)
> > > {
> > > @@ -615,7 +685,7 @@ private:
> > > }
> > >
> > > /* This assigns uniform indices to sampler and image
> > > uniforms. */
> > > - handle_samplers(base_type, &this->uniforms[id]);
> > > + handle_samplers(base_type, &this->uniforms[id], name);
> > > handle_images(base_type, &this->uniforms[id]);
> > > handle_subroutines(base_type, &this->uniforms[id]);
> > >
> > > @@ -704,6 +774,14 @@ private:
> > > unsigned next_image;
> > > unsigned next_subroutine;
> > >
> > > + /* Stores total struct array elements including nested
> > > structs */
> > > + unsigned record_array_count;
> > > +
> > > + /* Map for temporarily storing next sampler index when
> > > handling
> > > samplers in
> > > + * struct arrays.
> > > + */
> > > + struct string_to_uint_map *record_next_sampler;
> > > +
> > > public:
> > > union gl_constant_value *values;
> > >
> > > diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> > > index ce3dc32..40e6c73 100644
> > > --- a/src/glsl/linker.h
> > > +++ b/src/glsl/linker.h
> > > @@ -181,6 +181,8 @@ protected:
> > > virtual void leave_record(const glsl_type *type, const char
> > > *name,
> > > bool row_major);
> > >
> > > + virtual void set_record_array_count(unsigned
> > > record_array_count);
> > > +
> > > private:
> > > /**
> > > * \param name_length Length of the current name \b not
> > > including the
> > > @@ -191,7 +193,7 @@ private:
> > > */
> > > void recursion(const glsl_type *t, char **name, size_t
> > > name_length,
> > > bool row_major, const glsl_type *record_type,
> > > - bool last_field);
> > > + bool last_field, unsigned record_array_count);
> > > };
> > >
> > > void
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
