On 09/03/2013 09:41 AM, Paul Berry wrote:
> On 30 August 2013 16:07, Ian Romanick <i...@freedesktop.org
> <mailto:i...@freedesktop.org>> wrote:
>
> From: Ian Romanick <ian.d.roman...@intel.com
> <mailto:ian.d.roman...@intel.com>>
>
> The new function, cross_validate_types_and_qualifiers, will have
> multiple callers from this file in future commits.
>
> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com
> <mailto:ian.d.roman...@intel.com>>
> ---
> src/glsl/link_varyings.cpp | 171
> +++++++++++++++++++++++++--------------------
> 1 file changed, 94 insertions(+), 77 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 4ceb1d3..a1899f7 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -41,6 +41,97 @@
>
>
> /**
> + * Validate the types and qualifiers of an output from one stage
> against the
> + * matching input to another stage.
> + */
> +static void
> +cross_validate_types_and_qualifiers(struct gl_shader_program *prog,
> + const ir_variable *input,
> + const ir_variable *output,
> + GLenum consumer_type,
> + const char *consumer_stage,
> + const char *producer_stage)
>
>
> It seems redundant to pass both consumer_type and consumer_stage as
> arguments, since the latter is just
> _mesa_glsl_shader_target_name(consumer_type). You might want to just
> pass consumer_type and producer_type, and use
> _mesa_glsl_shader_target_name() to convert them to strings in the event
> of an error.
I thought about doing that. I decided against it for a couple
reasons... but now that I've looked at it again, I like it. :) I've
changed the signature to
static void
cross_validate_types_and_qualifiers(struct gl_shader_program *prog,
const ir_variable *input,
const ir_variable *output,
GLenum consumer_type,
GLenum producer_type);
And I just call _mesa_glsl_shader_target_name in the linker_error calls.
I pushed this to master of my repo on fdo. Does that look good to you?
> However, it's extra bookkeeping work to do that, so I'm ambivalent about
> it. Either way,
>
> Reviewed-by: Paul Berry <stereotype...@gmail.com
> <mailto:stereotype...@gmail.com>>
>
>
> +{
> + /* Check that the types match between stages.
> + */
> + const glsl_type *type_to_match = input->type;
> + if (consumer_type == GL_GEOMETRY_SHADER) {
> + assert(type_to_match->is_array()); /* Enforced by ast_to_hir */
> + type_to_match = type_to_match->element_type();
> + }
> + if (type_to_match != output->type) {
> + /* There is a bit of a special case for gl_TexCoord. This
> + * built-in is unsized by default. Applications that variable
> + * access it must redeclare it with a size. There is some
> + * language in the GLSL spec that implies the fragment shader
> + * and vertex shader do not have to agree on this size. Other
> + * driver behave this way, and one or two applications seem to
> + * rely on it.
> + *
> + * Neither declaration needs to be modified here because the
> array
> + * sizes are fixed later when update_array_sizes is called.
> + *
> + * From page 48 (page 54 of the PDF) of the GLSL 1.10 spec:
> + *
> + * "Unlike user-defined varying variables, the built-in
> + * varying variables don't have a strict one-to-one
> + * correspondence between the vertex language and the
> + * fragment language."
> + */
> + if (!output->type->is_array()
> + || (strncmp("gl_", output->name, 3) != 0)) {
> + linker_error(prog,
> + "%s shader output `%s' declared as type `%s', "
> + "but %s shader input declared as type `%s'\n",
> + producer_stage, output->name,
> + output->type->name,
> + consumer_stage, input->type->name);
> + return;
> + }
> + }
> +
> + /* Check that all of the qualifiers match between stages.
> + */
> + if (input->centroid != output->centroid) {
> + linker_error(prog,
> + "%s shader output `%s' %s centroid qualifier, "
> + "but %s shader input %s centroid qualifier\n",
> + producer_stage,
> + output->name,
> + (output->centroid) ? "has" : "lacks",
> + consumer_stage,
> + (input->centroid) ? "has" : "lacks");
> + return;
> + }
> +
> + if (input->invariant != output->invariant) {
> + linker_error(prog,
> + "%s shader output `%s' %s invariant qualifier, "
> + "but %s shader input %s invariant qualifier\n",
> + producer_stage,
> + output->name,
> + (output->invariant) ? "has" : "lacks",
> + consumer_stage,
> + (input->invariant) ? "has" : "lacks");
> + return;
> + }
> +
> + if (input->interpolation != output->interpolation) {
> + linker_error(prog,
> + "%s shader output `%s' specifies %s "
> + "interpolation qualifier, "
> + "but %s shader input specifies %s "
> + "interpolation qualifier\n",
> + producer_stage,
> + output->name,
> + output->interpolation_string(),
> + consumer_stage,
> + input->interpolation_string());
> + return;
> + }
> +}
> +
> +/**
> * Validate that outputs from one stage match inputs of another
> */
> void
> @@ -81,83 +172,9 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
>
> ir_variable *const output = parameters.get_variable(input->name);
> if (output != NULL) {
> - /* Check that the types match between stages.
> - */
> - const glsl_type *type_to_match = input->type;
> - if (consumer->Type == GL_GEOMETRY_SHADER) {
> - assert(type_to_match->is_array()); /* Enforced by
> ast_to_hir */
> - type_to_match = type_to_match->element_type();
> - }
> - if (type_to_match != output->type) {
> - /* There is a bit of a special case for gl_TexCoord. This
> - * built-in is unsized by default. Applications that
> variable
> - * access it must redeclare it with a size. There is some
> - * language in the GLSL spec that implies the fragment
> shader
> - * and vertex shader do not have to agree on this size.
> Other
> - * driver behave this way, and one or two applications
> seem to
> - * rely on it.
> - *
> - * Neither declaration needs to be modified here because
> the array
> - * sizes are fixed later when update_array_sizes is called.
> - *
> - * From page 48 (page 54 of the PDF) of the GLSL 1.10 spec:
> - *
> - * "Unlike user-defined varying variables, the built-in
> - * varying variables don't have a strict one-to-one
> - * correspondence between the vertex language and the
> - * fragment language."
> - */
> - if (!output->type->is_array()
> - || (strncmp("gl_", output->name, 3) != 0)) {
> - linker_error(prog,
> - "%s shader output `%s' declared as type
> `%s', "
> - "but %s shader input declared as type
> `%s'\n",
> - producer_stage, output->name,
> - output->type->name,
> - consumer_stage, input->type->name);
> - return;
> - }
> - }
> -
> - /* Check that all of the qualifiers match between stages.
> - */
> - if (input->centroid != output->centroid) {
> - linker_error(prog,
> - "%s shader output `%s' %s centroid qualifier, "
> - "but %s shader input %s centroid qualifier\n",
> - producer_stage,
> - output->name,
> - (output->centroid) ? "has" : "lacks",
> - consumer_stage,
> - (input->centroid) ? "has" : "lacks");
> - return;
> - }
> -
> - if (input->invariant != output->invariant) {
> - linker_error(prog,
> - "%s shader output `%s' %s invariant
> qualifier, "
> - "but %s shader input %s invariant qualifier\n",
> - producer_stage,
> - output->name,
> - (output->invariant) ? "has" : "lacks",
> - consumer_stage,
> - (input->invariant) ? "has" : "lacks");
> - return;
> - }
> -
> - if (input->interpolation != output->interpolation) {
> - linker_error(prog,
> - "%s shader output `%s' specifies %s "
> - "interpolation qualifier, "
> - "but %s shader input specifies %s "
> - "interpolation qualifier\n",
> - producer_stage,
> - output->name,
> - output->interpolation_string(),
> - consumer_stage,
> - input->interpolation_string());
> - return;
> - }
> + cross_validate_types_and_qualifiers(prog, input, output,
> + consumer->Type,
> consumer_stage,
> + producer_stage);
> }
> }
> }
> --
> 1.8.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
