On 11/20/2013 02:15 PM, Paul Berry wrote: > On 20 November 2013 11:35, Ian Romanick <i...@freedesktop.org > <mailto:i...@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.roman...@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-sta...@lists.freedesktop.org > <mailto:mesa-sta...@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) > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
