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

[Samuel Iglesias Gonsálvez]

On Wed, 2016-03-09 at 00:47 +1100, Timothy Arceri 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)

Just one comment, I usually see the relational operator at the end of
the previous line, although there are several examples in Mesa like
this. I checked the coding style and I have not found any reference of
the preferred way.

In any case, the series is:

Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>

Sam

> +         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;
> +   }
> +
> +   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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160308/ee3ea06c/attachment.sig>