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

[Ian Romanick]

On 04/30/2014 12:40 PM, Eric Anholt wrote:
> 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.

I think you're correct on both counts.  I'm not sure if the
closed-source drivers enforce this either.  More tests.

>> @@ -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.

Yes... I /think/ that's actually a stale FINISHME that should have been
removed.  I'll replace it with your reasoning.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140430/9e77a2bb/attachment.sig>