[Mesa-dev] [PATCH] glsl: Fix cross-version linking between VS and GS.

[Paul Berry]

On 20 November 2013 11:35, Ian Romanick <idr at freedesktop.org> wrote:

> On 11/19/2013 06:11 PM, Paul Berry wrote:
> > Previously, when attempting to link a vertex shader and a geometry
> > shader that use different GLSL versions, we would sometimes generate a
> > link error due to the implicit declaration of gl_PerVertex being
> > different between the two GLSL versions.
> >
> > This patch fixes that problem by only requiring interface block
> > definitions to match when they are explicitly declared.
> >
> > Fixes piglit test "shaders/version-mixing vs-gs".
>
> Which type do you get?  What happens when the linked shader runs?
>

The interface blocks get lowered to individual variable declarations by
lower_named_interface_blocks.cpp, and then since these are built-ins, the
linker isn't actually responsible for matching them up--they get matched up
automatically by virtue of the ir_variable->location values that were
preassigned by builtin_variables.cpp.

So what happens when the shader runs is that the mismatch really has no
consequence.  For example, if we link a 1.10 VS with a 1.50 GS, and the GS
tries to access gl_in[...].gl_ClipDistance, it gets undefined results,
because the VS didn't write to gl_ClipDistance (it couldn't, because
gl_ClipDistance didn't exist in 1.10).  But that's no different from what
would have happened if we had linked a 1.50 VS that didn't write
gl_ClipDistance to a 1.50 GS that tried to read from gl_ClipDistance.


>
> Do we know what other implementations do for mixed VS / GS versions?
>

My Nvidia system allows arbitrary mixing of VS / GS versions.


>
> > Cc: "10.0" <mesa-stable at lists.freedesktop.org>
> > ---
> >
> > This patch depends on "[PATCH v2] glsl: Prohibit illegal mixing of
> > redeclarations inside/outside gl_PerVertex.", which was sent out for
> > review earlier today.
> >
> >  src/glsl/link_interface_blocks.cpp | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/glsl/link_interface_blocks.cpp
> b/src/glsl/link_interface_blocks.cpp
> > index a7fceb9..b4379b0 100644
> > --- a/src/glsl/link_interface_blocks.cpp
> > +++ b/src/glsl/link_interface_blocks.cpp
> > @@ -59,6 +59,9 @@ struct interface_block_definition
> >           instance_name = var->name;
> >           if (var->type->is_array())
> >              array_size = var->type->length;
> > +         explicitly_declared = (var->how_declared ==
> ir_var_declared_normally);
> > +      } else {
> > +         explicitly_declared = (var->how_declared ==
> ir_var_declared_in_block);
>
> Would it be easier to just have
>
>    explicitly_declared = (var->how_declared != ir_var_declared_implicitly);
>
> after the existing if-block?
>

Ok, sure.  I don't have a strong feeling about this, so I'm happy to change
it.


> >        }
> >     }
> >
> > @@ -77,6 +80,12 @@ struct interface_block_definition
> >      * 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.
> > +    */
> > +   bool explicitly_declared;
> >  };
> >
> >
> > @@ -91,8 +100,14 @@ intrastage_match(interface_block_definition *a,
> >                   ir_variable_mode mode)
> >  {
> >     /* Types must match. */
> > -   if (a->type != b->type)
> > -      return false;
> > +   if (a->type != b->type) {
> > +      /* 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.
> > +       */
> > +      if (a->explicitly_declared || b->explicitly_declared)
> > +         return false;
> > +   }
> >
> >     /* Presence/absence of interface names must match. */
> >     if ((a->instance_name == NULL) != (b->instance_name == NULL))
> > @@ -144,8 +159,14 @@ interstage_match(const interface_block_definition
> *producer,
> >     assert(producer->array_size != 0);
> >
> >     /* Types must match. */
> > -   if (consumer->type != producer->type)
> > -      return false;
> > +   if (consumer->type != producer->type) {
> > +      /* 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.
> > +       */
> > +      if (consumer->explicitly_declared ||
> producer->explicitly_declared)
> > +         return false;
> > +   }
> >     if (extra_array_level) {
> >        /* Consumer must be an array, and producer must not. */
> >        if (consumer->array_size == -1)
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131120/9c77482f/attachment.html>