[Mesa-dev] [PATCH 3/4] glsl: use common intrastage array validation

[Timothy Arceri]

---- 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.

> 
> >        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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150226/4dc712d3/attachment-0001.html>