[Mesa-dev] [PATCH 3/3] glsl: don't always reject shaders with mismatching ifc blocks

Timothy Arceri timothy.arceri at collabora.com
Tue Mar 8 22:00:13 UTC 2016


On Tue, 2016-03-08 at 10:37 -0500, Ilia Mirkin wrote:
> On Tue, Mar 8, 2016 at 8:47 AM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > Since we store some member qualifiers in the interface type
> > we need to be more careful about rejecting shaders just because
> > the pointer doesn't match. Its perfectly valid for some qualifiers
> > such as precision to not match across shader interfaces.
> > ---
> >  src/compiler/glsl/link_interface_blocks.cpp | 78
> > ++++++++++++++++++++++++++---
> >  1 file changed, 72 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_interface_blocks.cpp
> > b/src/compiler/glsl/link_interface_blocks.cpp
> > index 9d36836..f868c99 100644
> > --- a/src/compiler/glsl/link_interface_blocks.cpp
> > +++ b/src/compiler/glsl/link_interface_blocks.cpp
> > @@ -81,6 +81,66 @@ intrastage_match(ir_variable *a,
> >     return true;
> >  }
> > 
> > +/**
> > + * Return true if interface members mismatch and its not allowed
> > by GLSL.
> > + */
> > +static bool
> > +interstage_member_mismatch(struct gl_shader_program *prog,
> > +                           const glsl_type *c, const glsl_type *p)
> > {
> > +
> > +   if (c->length != p->length)
> > +      return true;
> > +
> > +   for (unsigned i = 0; i < c->length; i++) {
> > +      if (c->fields.structure[i].type != p-
> > >fields.structure[i].type)
> > +         return true;
> > +      if (strcmp(c->fields.structure[i].name,
> > +                 p->fields.structure[i].name) != 0)
> > +         return true;
> > +      if (c->fields.structure[i].location
> > +          != p->fields.structure[i].location)
> > +         return true;
> > +      if (c->fields.structure[i].patch
> > +          != p->fields.structure[i].patch)
> > +         return true;
> > +
> > +      /* From Section 4.5 (Interpolation Qualifiers) of the GLSL
> > 4.40 spec:
> > +       *
> > +       *    "It is a link-time error if, within the same stage,
> > the
> > +       *    interpolation qualifiers of variables of the same name
> > do not
> > +       *    match."
> > +       */
> > +      if (prog->IsES || prog->Version < 440)
> > +         if (c->fields.structure[i].interpolation
> > +             != p->fields.structure[i].interpolation)
> > +            return true;
> > +
> > +      /* From Section 4.3.4 (Input Variables) of the GLSL ES 3.0
> > spec:
> > +       *
> > +       *    "The output of the vertex shader and the input of the
> > fragment
> > +       *    shader form an interface.  For this interface, vertex
> > shader
> > +       *    output variables and fragment shader input variables
> > of the same
> > +       *    name must match in type and qualification (other than
> > precision
> > +       *    and out matching to in).
> > +       *
> > +       * The table in Section 9.2.1 Linked Shaders of the GLSL ES
> > 3.1 spec
> > +       * says that centroid no longer needs to match for varyings.
> > +       *
> > +       * The table in Section 9.2.1 Linked Shaders of the GLSL ES
> > 3.2 spec
> > +       * says that sample need not match for varyings.
> > +       */
> > +      if (!prog->IsES || prog->Version < 310)
> > +         if (c->fields.structure[i].centroid
> > +             != p->fields.structure[i].centroid)
> > +            return true;
> > +      if (!prog->IsES || prog->Version < 320)
> > +         if (c->fields.structure[i].sample
> > +             != p->fields.structure[i].sample)
> > +            return true;
> 
> Are you sure? The sample qualifier is added by
> OES_shader_multisample_interpolation, which says "Also, the "sample"
> qualifier may differ". And before that ext / ES 3.2, the sample
> qualifier didn't exist. So I'd change this to just
> 
> if (!prog->IsES)

Thanks for checking the OES_shader_multisample_interpolation spec I was
thinking right after I sent the series that it should probably just
be if (!prog->IsES).

Will fix before pushing. Thanks.

> 
> > +   }
> > +
> > +   return false;
> > +}
> > 
> >  /**
> >   * Check if two interfaces match, according to interstage (in/out)
> > interface
> > @@ -91,9 +151,8 @@ intrastage_match(ir_variable *a,
> >   * This is used for tessellation control and geometry shader
> > consumers.
> >   */
> >  static bool
> > -interstage_match(ir_variable *producer,
> > -                 ir_variable *consumer,
> > -                 bool extra_array_level)
> > +interstage_match(struct gl_shader_program *prog, ir_variable
> > *producer,
> > +                 ir_variable *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.
> > @@ -106,9 +165,16 @@ interstage_match(ir_variable *producer,
> >        /* Exception: if both the interface blocks are implicitly
> > declared,
> >         * don't force their types to match.  They might mismatch
> > due to the two
> >         * shaders using different GLSL versions, and that's ok.
> > +       *
> > +       * Also we store some member information in glsl_type that
> > doesn't
> > +       * always have to match across shaders such as interpolation
> > so we need
> > +       * to make a pass over members glsl_struct_field to make
> > sure we don't
> > +       * reject shaders where fields don't need to match.
> >         */
> > -      if (consumer->data.how_declared !=
> > ir_var_declared_implicitly ||
> > -          producer->data.how_declared !=
> > ir_var_declared_implicitly)
> > +      if ((consumer->data.how_declared !=
> > ir_var_declared_implicitly ||
> > +           producer->data.how_declared !=
> > ir_var_declared_implicitly) &&
> > +          interstage_member_mismatch(prog, consumer-
> > >get_interface_type(),
> > +                                     producer-
> > >get_interface_type()))
> >           return false;
> >     }
> > 
> > @@ -311,7 +377,7 @@ validate_interstage_inout_blocks(struct
> > gl_shader_program *prog,
> >        if (consumer_def == NULL)
> >           continue;
> > 
> > -      if (!interstage_match(var, consumer_def, extra_array_level))
> > {
> > +      if (!interstage_match(prog, var, consumer_def,
> > extra_array_level)) {
> >           linker_error(prog, "definitions of interface block `%s'
> > do not "
> >                        "match\n", var->get_interface_type()->name);
> >           return;
> > --
> > 2.5.0
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list