[Mesa-dev] [PATCH 15/21] linker: Modify cross_validate_outputs_to_inputs to match using explicit locations

[Eric Anholt]

Ian Romanick <idr at freedesktop.org> writes:

> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This will be used for GL_ARB_separate_shader_objects.  That extension
> not only allows separable shaders to rendezvous by location, but it also
> allows traditionally linked shaders to rendezvous by location.  The spec
> says:
>
>     36. How does the behavior of input/output interface matching differ
>         between separable programs and non-separable programs?
>
>         RESOLVED: The rules for matching individual variables or block
>         members between stages are identical for separable and
>         non-separable programs, with one exception -- matching variables
>         of different type with the same location, as discussed in issue
>         34, applies only to separable programs.
>
>         However, the ability to enforce matching requirements differs
>         between program types.  In non-separable programs, both sides of
>         an interface are contained in the same linked program.  In this
>         case, if the linker detects a mismatch, it will generate a link
>         error.


> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index e5c2751..232e230 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -172,6 +172,7 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
>  				 gl_shader *producer, gl_shader *consumer)
>  {
>     glsl_symbol_table parameters;
> +   ir_variable *explicit_locations[MAX_VARYING] = { NULL, };
>  
>     /* Find all shader outputs in the "producer" stage.
>      */
> @@ -181,7 +182,26 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
>        if ((var == NULL) || (var->data.mode != ir_var_shader_out))
>  	 continue;
>  
> -      parameters.add_variable(var);
> +      if (!var->data.explicit_location
> +          || var->data.location < VARYING_SLOT_VAR0)
> +         parameters.add_variable(var);
> +      else {
> +         /* User-defined varyings with explicit locations are handled
> +          * differently because they do not need to have matching names.
> +          */
> +         const unsigned idx = var->data.location - VARYING_SLOT_VAR0;
> +
> +         if (explicit_locations[idx] != NULL) {
> +            linker_error(prog,
> +                         "%s shader has multiple outputs explicitly "
> +                         "assigned to location %d\n",
> +                         _mesa_shader_stage_to_string(producer->Stage),
> +                         idx);
> +            return;

Relevant citation:

    A program will fail to link if any two fragment shader output variables
    are assigned to the same location and index, or if any two output
    variables from the same non-fragment shader stage are assigned to the same
    location.

I think this has a problem of not throwing an error when your explicit
location array overlaps your explicit location scalar, but I don't think
it's that important.

> @@ -1051,8 +1091,13 @@ namespace linker {
>  bool
>  populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
>                               hash_table *consumer_inputs,
> -                             hash_table *consumer_interface_inputs)
> +                             hash_table *consumer_interface_inputs,
> +                             ir_variable *consumer_inputs_with_locations[MAX_VARYING])
>  {
> +   memset(consumer_inputs_with_locations,
> +          0,
> +          sizeof(consumer_inputs_with_locations[0]) * MAX_VARYING);
> +
>     foreach_list(node, ir) {
>        ir_variable *const input_var = ((ir_instruction *) node)->as_variable();
>  
> @@ -1060,7 +1105,13 @@ populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
>           if (input_var->type->is_interface())
>              return false;
>  
> -         if (input_var->get_interface_type() != NULL) {
> +         if (input_var->data.explicit_location) {
> +            /* FINISHME: If the input is an interface, array, or structure, it
> +             * FINISHME: will use multiple slots.
> +             */
> +            consumer_inputs_with_locations[input_var->data.location] =
> +               input_var;

I think this FINISHME shouldn't be there.  It looks like
assign_varying_locations() only cares about finding the ir_variable at
the start of a contiguous location block.  My reasoning:

For !producer, consumer_inputs_with_locations isn't used.

For !consumer, consumer_inputs_with_locations is empty.

For consumer && producer, if you were trying to set some ir_variable to
the middle of a location block on the other side of producer/consumer,
cross_validate_outputs_to_inputs() should be link-erroring due to either
type mismatch or location overlaps.  If the variables do match up, then
they've got a matching data.location and you only looked at
consumer_inputs_with_locations[var->data.location], not any following
entries for the array/structure.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140430/0c8d6439/attachment.sig>