[Mesa-stable] [Mesa-dev] [PATCH 3/5] mesa: Fix add_index_to_name logic

[Ian Romanick]

On 05/31/2016 02:44 PM, Timothy Arceri wrote:
> On Tue, 2016-05-31 at 11:52 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Our piglit tests for geometry and tessellation shader inputs were
>> incorrect.  Array shader inputs and output should have '[0]' on the
>> end
>> regardless of stage.  In addtion, transform feedback varyings should
>> not.
> 
> Is there a spec quote for this? It doesn't seem right to me since for
> arrays of arrays would that mean we should end up with gs inputs like
> this

Here are all the rules that I think applies:

      * For an active variable declared as an array of basic types, a single
        entry will be generated, with its name string formed by concatenating
        the name of the array and the string "[0]".

      * For an active variable declared as a structure, a separate entry will
        be generated for each active structure member.  The name of each entry
        is formed by concatenating the name of the structure, the "."
        character, and the name of the structure member.  If a structure
        member to enumerate is itself a structure or array, these enumeration
        rules are applied recursively.

      * For an active variable declared as an array of an aggregate data type
        (structures or arrays), a separate entry will be generated for each
        active array element, unless noted immediately below.  The name of
        each entry is formed by concatenating the name of the array, the "["
        character, an integer identifying the element number, and the "]"
        character.  These enumeration rules are applied recursively, treating
        each enumerated array element as a separate active variable.

> input_name[0][0]
> input_name[...][0]
> input_name[num_vertices-1][0]

Yes, this is correct. We don't do this with or without this patch. I
don't know of any tests that exercise this.  Alas.

> otherwise
> 
> in vec4 input1[];
> and
> in vec4 input2[][3];
> 
> Would both end up as:
> input1[0]
> input2[0]
> 
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/mesa/main/shader_query.cpp | 23 ++++++++++-------------
>>  1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp
>> b/src/mesa/main/shader_query.cpp
>> index eec933c..f4b7243 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -696,20 +696,17 @@ _mesa_program_resource_find_index(struct
>> gl_shader_program *shProg,
>>  static bool
>>  add_index_to_name(struct gl_program_resource *res)
>>  {
>> -   bool add_index = !((res->Type == GL_PROGRAM_INPUT &&
>> -                       res->StageReferences & (1 <<
>> MESA_SHADER_GEOMETRY |
>> -                                               1 <<
>> MESA_SHADER_TESS_CTRL |
>> -                                               1 <<
>> MESA_SHADER_TESS_EVAL)) ||
>> -                      (res->Type == GL_PROGRAM_OUTPUT &&
>> -                       res->StageReferences & 1 <<
>> MESA_SHADER_TESS_CTRL));
>> -
>> -   /* Transform feedback varyings have array index already appended
>> -    * in their names.
>> -    */
>> -   if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
>> -      add_index = false;
>> +   if (res->Type != GL_PROGRAM_INPUT && res->Type !=
>> GL_PROGRAM_OUTPUT)
>> +      return res->Type != GL_TRANSFORM_FEEDBACK_VARYING;
> 
> I'm slighlty confused by this. When does this return true? And for
> transform feedback wont this always end up us false?
> 
> So isn't it just 
> 
>    if (res->Type == GL_TRANSFORM_FEEDBACK_VARYING)
>       return false;

I thought I tried that but it regressed some dEQP tests.  I'll double
check.

>> +
>> +   const gl_shader_variable *const var = RESOURCE_VAR(res);
>>  
>> -   return add_index;
>> +   assert(var->type->is_array());
>> +
>> +   if (var->interface_type != NULL && var->interface_type-
>>> is_array())
>> +      return var->type->fields.array->is_array();
>> +
> 
> So I'm assuming your doing this since after lowering block[3].foo we
> end up with foo[3]. However something like block[3].foo[2] will end up
> as foo[3][2] and blocks can also be arrays of arrays so I'm not sure
> this will work.

Correct.  I had forgotten about blocks being arrays-of-arrays.  That
should be easy enough to fix.

>> +   return true;
>>  }
>>  
>>  /* Get name length of a program resource. This consists of