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

[Ian Romanick]

On 11/20/2013 02:15 PM, Paul Berry wrote:
> On 20 November 2013 11:35, Ian Romanick <idr at freedesktop.org
> <mailto: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.

Ahh.. okay.  Then with or without the minor change I suggested, this
patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

>     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
>     <mailto: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)
>     >
> 
>