On Mon, Dec 01, 2014 at 02:04:50PM +0100, Eduardo Lima Mitev wrote:
> From: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
>
> GLSL ES 3.00 spec, 4.3.10 (Linking of Vertex Outputs and Fragment Inputs),
> page 45 says the following:
>
> "The type of vertex outputs and fragment input with the same name must match,
> otherwise the link command will fail. The precision does not need to match.
> Only those fragment inputs statically used (i.e. read) in the fragment shader
> must be declared as outputs in the vertex shader; declaring superfluous vertex
> shader outputs is permissible."
> [...]
> "The term static use means that after preprocessing the shader includes at
> least one statement that accesses the input or output, even if that statement
> is never actually executed."
>
> And it includes a table with all the possibilities.
>
> Similar table or content is present in other GLSL specs: GLSL 4.40, GLSL 1.50,
> etc but for more stages (vertex and geometry shaders, etc).
>
> This patch detects that case and returns a link error. It fixes the following
> dEQP test:
>
> dEQP-GLES3.functional.shaders.linkage.varying.rules.illegal_usage_1
>
> However, it adds a new regression in piglit because the test hasn't a
> vertex shader and it checks the link status.
>
> bin/glslparsertest \
> tests/spec/glsl-1.50/compiler/gs-also-uses-smooth-flat-noperspective.geom
> pass \
> 1.50 --check-link
>
> This piglit test is wrong according to the spec wording above, so if this
> patch
> is merged it should be updated.
>
> Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
> ---
> src/glsl/link_varyings.cpp | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 1fe198a..036996f 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -263,6 +263,22 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> if (output != NULL) {
> cross_validate_types_and_qualifiers(prog, input, output,
> consumer->Stage,
> producer->Stage);
> + } else {
> + /* Check for input vars with unmatched output vars in prev stage
> + * taking into account that interface blocks could have a match
> + * output but with different name, so we ignore them.
> + */
> + if (input->data.used && !input->data.assigned &&
> + !(input->is_interface_instance() ||
> + input->get_interface_type() ||
> + input->is_in_uniform_block()) &&
> + input->data.how_declared == ir_var_declared_normally &&
> + input->data.location == -1)
> + linker_error(prog,
> + "%s shader input `%s' "
> + "has no matching output in the previous stage\n",
> + _mesa_shader_stage_to_string(consumer->Stage),
> + input->name);
The current code isn't wrong, but it made things somewhat confusing for me when
reviewing. I don't think there is ever a case where input->data.assigned is
valid. Inputs are supposed to be read only, and so they would never be assigned.
How about instead: assert(!input->data.assigned);
I am also not certain if how_declared is correct. It seems like we shouldn't get
this far if that wasn't true, but I am not an expert (and I can't see it being
wrong).
With the data.assigned change, it's
Reviewed-by: Ben Widawsky <b...@bwidawsk.net>
> }
> }
> }
> --
> 2.1.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
