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

[Ian Romanick]

On 05/31/2016 04:45 PM, Ian Romanick wrote:
> 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.

It makes a huge pile of dEQP tests fail because lots of things,
including UBOs and SSBOs, come through this function.  Inputs and
outputs get some special treatment because of the array-of-interface
handing, and xfb variables never get the [0] suffix.  Everything else
gets the [0] suffix.

>>> +
>>> +   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
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>