[Mesa-dev] [PATCH 3/4] glsl: use common intrastage array validation
Timothy Arceri
t_arceri at yahoo.com.au
Wed Feb 25 23:19:30 PST 2015
On Thu, 2015-02-26 at 12:06 +1100, Timothy Arceri wrote:
> ---- Mark Janes wrote ----
>
> > Timothy Arceri <t_arceri at yahoo.com.au> writes:
> >
> > > Use common intrastage array validation for interface blocks. This
> change also allows us to support interface blocks that are arrays of
> arrays.
> >
> > Please wrap this message to fit within 80 columns.
> >
> > > ---
> > > src/glsl/link_interface_blocks.cpp | 76
> ++++++++++++++++++--------------------
> > > 1 file changed, 35 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/src/glsl/link_interface_blocks.cpp
> b/src/glsl/link_interface_blocks.cpp
> > > index 0ce502d..9000a3c 100644
> > > --- a/src/glsl/link_interface_blocks.cpp
> > > +++ b/src/glsl/link_interface_blocks.cpp
> > > @@ -50,18 +50,20 @@ struct interface_block_definition
> > > * represents either the interface instance (for named
> interfaces), or a
> > > * member of the interface (for unnamed interfaces).
> > > */
> > > - explicit interface_block_definition(const ir_variable *var)
> > > - : type(var->get_interface_type()),
> > > - instance_name(NULL),
> > > - array_size(-1)
> > > + explicit interface_block_definition(ir_variable *var)
> > > + : var(var),
> > > + type(var->get_interface_type()),
> > > + instance_name(NULL)
> > > {
> > > if (var->is_interface_instance()) {
> > > instance_name = var->name;
> > > - if (var->type->is_array())
> > > - array_size = var->type->length;
> > > }
> > > explicitly_declared = (var->data.how_declared !=
> ir_var_declared_implicitly);
> > > }
> > > + /**
> > > + * Interface block ir_variable
> > > + */
> > > + ir_variable *var;
> > >
> > > /**
> > > * Interface block type
> > > @@ -74,12 +76,6 @@ struct interface_block_definition
> > > const char *instance_name;
> > >
> > > /**
> > > - * For an interface block array, the array size (or 0 if
> unsized).
> > > - * Otherwise -1.
> > > - */
> > > - int array_size;
> > > -
> > > - /**
> > > * True if this interface block was explicitly declared in the
> shader;
> > > * false if it was an implicitly declared built-in interface
> block.
> > > */
> > > @@ -95,7 +91,8 @@ struct interface_block_definition
> > > bool
> > > intrastage_match(interface_block_definition *a,
> > > const interface_block_definition *b,
> > ^^^^^
> > I think the intent of this declaration is that b should not change
> (see below).
> >
> > > - ir_variable_mode mode)
> > > + ir_variable_mode mode,
> > > + struct gl_shader_program *prog)
> > > {
> > > /* Types must match. */
> > > if (a->type != b->type) {
> > > @@ -120,18 +117,13 @@ intrastage_match(interface_block_definition
> *a,
> > > return false;
> > > }
> > >
> > > - /* Array vs. nonarray must be consistent, and sizes must be
> > > - * consistent, with the exception that unsized arrays match
> sized
> > > - * arrays.
> > > + /* If a block is an array then it must match across the
> shader.
> > > + * Unsized arrays are also processed and matched agaist sized
> arrays.
> > > */
> > > - if ((a->array_size == -1) != (b->array_size == -1))
> > > + if (b->var->type != a->var->type &&
> > > + (b->instance_name != NULL || a->instance_name != NULL) &&
> > > + !validate_intrastage_arrays(prog, b->var, a->var))
> >
> > you pass a b's var member into validate_intrastage_arrays, where it
> may
> > be modified (due to your second patch in this series). Is that your
> intention?
> >
> > It doesn't seem to affect any piglit tests.
>
> Patch 2 may have been a result of me passing the a and b values in the
> wrong order when I started development, I will check this.
Ok I didn't need to modify the value in patch 2 at all just add the
return true. In which case it should be squashed into patch 1.
Thanks for looking over this.
I'll send a version 2.
>
> >
> > > return false;
> > > - if (b->array_size != 0) {
> > > - if (a->array_size == 0)
> > > - a->array_size = b->array_size;
> > > - else if (a->array_size != b->array_size)
> > > - return false;
> > > - }
> > >
> > > return true;
> > > }
> > > @@ -150,12 +142,6 @@ interstage_match(const
> interface_block_definition *producer,
> > > const interface_block_definition *consumer,
> > > bool extra_array_level)
> > > {
> > > - /* Unsized arrays should not occur during interstage linking.
> They
> > > - * should have all been assigned a size by
> link_intrastage_shaders.
> > > - */
> > > - assert(consumer->array_size != 0);
> > > - assert(producer->array_size != 0);
> > > -
> > > /* Types must match. */
> > > if (consumer->type != producer->type) {
> > > /* Exception: if both the interface blocks are implicitly
> declared,
> > > @@ -165,20 +151,27 @@ interstage_match(const
> interface_block_definition *producer,
> > > if (consumer->explicitly_declared ||
> producer->explicitly_declared)
> > > return false;
> > > }
> > > +
> > > + /* Ignore outermost array if geom shader */
> > > + const glsl_type *consumer_instance_type;
> > > if (extra_array_level) {
> > > - /* Consumer must be an array, and producer must not. */
> > > - if (consumer->array_size == -1)
> > > - return false;
> > > - if (producer->array_size != -1)
> > > - return false;
> > > + consumer_instance_type = consumer->var->type->fields.array;
> > > } else {
> > > - /* Array vs. nonarray must be consistent, and sizes must be
> consistent.
> > > - * Since unsized arrays have been ruled out, we can check
> this by just
> > > - * making sure the sizes are equal.
> > > - */
> > > - if (consumer->array_size != producer->array_size)
> > > + consumer_instance_type = consumer->var->type;
> > > + }
> > > +
> > > + /* If a block is an array then it must match across shaders.
> > > + * Since unsized arrays have been ruled out, we can check this
> by just
> > > + * making sure the types are equal.
> > > + */
> > > + if ((consumer->instance_name != NULL &&
> > > + consumer_instance_type->is_array()) ||
> > > + (producer->instance_name != NULL &&
> > > + producer->var->type->is_array())) {
> > > + if (consumer_instance_type != producer->var->type)
> > > return false;
> > > }
> > > +
> > > return true;
> > > }
> > >
> > > @@ -298,7 +291,8 @@ validate_intrastage_interface_blocks(struct
> gl_shader_program *prog,
> > > */
> > > definitions->store(def);
> > > } else if (!intrastage_match(prev_def, &def,
> > > - (ir_variable_mode)
> var->data.mode)) {
> > > + (ir_variable_mode)
> var->data.mode,
> > > + prog)) {
> > > linker_error(prog, "definitions of interface block `%
> s' do not"
> > > " match\n", iface_type->name);
> > > return;
> > > @@ -374,7 +368,7 @@ validate_interstage_uniform_blocks(struct
> gl_shader_program *prog,
> > > * uniform matchin rules (for uniforms, it is as
> though all
> > > * shaders are in the same shader stage).
> > > */
> > > - if (!intrastage_match(old_def, &new_def,
> ir_var_uniform)) {
> > > + if (!intrastage_match(old_def, &new_def,
> ir_var_uniform, prog)) {
> > > linker_error(prog, "definitions of interface block
> `%s' do not "
> > > "match\n",
> var->get_interface_type()->name);
> > > return;
> > > --
> > > 2.1.0
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list