<div dir="ltr">On 20 November 2013 11:35, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 11/19/2013 06:11 PM, Paul Berry wrote:<br>
> Previously, when attempting to link a vertex shader and a geometry<br>
> shader that use different GLSL versions, we would sometimes generate a<br>
> link error due to the implicit declaration of gl_PerVertex being<br>
> different between the two GLSL versions.<br>
><br>
> This patch fixes that problem by only requiring interface block<br>
> definitions to match when they are explicitly declared.<br>
><br>
> Fixes piglit test "shaders/version-mixing vs-gs".<br>
<br>
</div>Which type do you get? What happens when the linked shader runs?<br></blockquote><div><br></div><div>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.<br>
<br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Do we know what other implementations do for mixed VS / GS versions?<br></blockquote><div><br></div><div>My Nvidia system allows arbitrary mixing of VS / GS versions.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im"><br>
> Cc: "10.0" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>><br>
> ---<br>
><br>
> This patch depends on "[PATCH v2] glsl: Prohibit illegal mixing of<br>
> redeclarations inside/outside gl_PerVertex.", which was sent out for<br>
> review earlier today.<br>
><br>
> src/glsl/link_interface_blocks.cpp | 29 +++++++++++++++++++++++++----<br>
> 1 file changed, 25 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/glsl/link_interface_blocks.cpp b/src/glsl/link_interface_blocks.cpp<br>
> index a7fceb9..b4379b0 100644<br>
> --- a/src/glsl/link_interface_blocks.cpp<br>
> +++ b/src/glsl/link_interface_blocks.cpp<br>
> @@ -59,6 +59,9 @@ struct interface_block_definition<br>
> instance_name = var->name;<br>
> if (var->type->is_array())<br>
> array_size = var->type->length;<br>
> + explicitly_declared = (var->how_declared == ir_var_declared_normally);<br>
> + } else {<br>
> + explicitly_declared = (var->how_declared == ir_var_declared_in_block);<br>
<br>
</div>Would it be easier to just have<br>
<br>
explicitly_declared = (var->how_declared != ir_var_declared_implicitly);<br>
<br>
after the existing if-block?<br></blockquote><div><br></div><div>Ok, sure. I don't have a strong feeling about this, so I'm happy to change it.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><div class="h5"><br>
> }<br>
> }<br>
><br>
> @@ -77,6 +80,12 @@ struct interface_block_definition<br>
> * Otherwise -1.<br>
> */<br>
> int array_size;<br>
> +<br>
> + /**<br>
> + * True if this interface block was explicitly declared in the shader;<br>
> + * false if it was an implicitly declared built-in interface block.<br>
> + */<br>
> + bool explicitly_declared;<br>
> };<br>
><br>
><br>
> @@ -91,8 +100,14 @@ intrastage_match(interface_block_definition *a,<br>
> ir_variable_mode mode)<br>
> {<br>
> /* Types must match. */<br>
> - if (a->type != b->type)<br>
> - return false;<br>
> + if (a->type != b->type) {<br>
> + /* Exception: if both the interface blocks are implicitly declared,<br>
> + * don't force their types to match. They might mismatch due to the two<br>
> + * shaders using different GLSL versions, and that's ok.<br>
> + */<br>
> + if (a->explicitly_declared || b->explicitly_declared)<br>
> + return false;<br>
> + }<br>
><br>
> /* Presence/absence of interface names must match. */<br>
> if ((a->instance_name == NULL) != (b->instance_name == NULL))<br>
> @@ -144,8 +159,14 @@ interstage_match(const interface_block_definition *producer,<br>
> assert(producer->array_size != 0);<br>
><br>
> /* Types must match. */<br>
> - if (consumer->type != producer->type)<br>
> - return false;<br>
> + if (consumer->type != producer->type) {<br>
> + /* Exception: if both the interface blocks are implicitly declared,<br>
> + * don't force their types to match. They might mismatch due to the two<br>
> + * shaders using different GLSL versions, and that's ok.<br>
> + */<br>
> + if (consumer->explicitly_declared || producer->explicitly_declared)<br>
> + return false;<br>
> + }<br>
> if (extra_array_level) {<br>
> /* Consumer must be an array, and producer must not. */<br>
> if (consumer->array_size == -1)<br>
><br>
<br>
</div></div></blockquote></div><br></div></div>