[Mesa-dev] [PATCH 11/23] mesa: refactor GetAttribLocation

[Tapani Pälli]

On 03/17/2015 12:30 AM, Ilia Mirkin wrote:
> On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>> Use program_resource_location to fetch location.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   src/mesa/main/shader_query.cpp | 33 ++++++++++-----------------------
>>   1 file changed, 10 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>> index 8134d4b..7e8cf9c 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -41,6 +41,10 @@ extern "C" {
>>   #include "shaderapi.h"
>>   }
>>
>> +static GLint
>> +program_resource_location(struct gl_shader_program *shProg,
>> +                          struct gl_program_resource *res, const char *name);
>> +
>>   /**
>>    * Declare convenience functions to return resource data in a given type.
>>    * Warning! this is not type safe so be *very* careful when using these.
>> @@ -266,31 +270,14 @@ _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
>>      if (shProg->_LinkedShaders[MESA_SHADER_VERTEX] == NULL)
>>         return -1;
>>
>> -   exec_list *ir = shProg->_LinkedShaders[MESA_SHADER_VERTEX]->ir;
>> -   foreach_in_list(ir_instruction, node, ir) {
>> -      const ir_variable *const var = node->as_variable();
>> -
>> -      /* The extra check against VERT_ATTRIB_GENERIC0 is because
>> -       * glGetAttribLocation cannot be used on "conventional" attributes.
>> -       *
>> -       * From page 95 of the OpenGL 3.0 spec:
>> -       *
>> -       *     "If name is not an active attribute, if name is a conventional
>> -       *     attribute, or if an error occurs, -1 will be returned."
>> -       */
>> -      if (var == NULL
>> -         || var->data.mode != ir_var_shader_in
>> -         || var->data.location == -1
>> -         || var->data.location < VERT_ATTRIB_GENERIC0)
>> -        continue;
>> -
>> -      int index = get_matching_index(var, (const char *) name);
>> +   struct gl_program_resource *res =
>> +      _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT, name);
>>
>> -      if (index >= 0)
>> -         return var->data.location + index - VERT_ATTRIB_GENERIC0;
>> -   }
>> +   if (!res)
>> +      return -1;
>>
>> -   return -1;
>> +   GLint loc = program_resource_location(shProg, res, name);
>> +   return (loc >= 0) ? loc : -1;
>
> What does program_resource_location return? Can you just do
>
> return program_resource_location(...)?

Argh sorry, I forgot to add some comments to address and discuss this. 
When we assign locations to attributes, VERT_ATTRIB_GENERIC0 offset is 
added so that we don't overlap 'conventional' attributes .. 
'program_resource_location()' subtracts this from the location stored. 
This is how it is specified to work before the extension and OpenGL 4.3.

So right here the location returned by program_resource_location() would 
be < 0 if it is one of the built-in attributes. This means that 
implementation here might be actually wrong. If I read spec correctly, 
everything should be returned, no matter if it is builtin or not;

See issue 24:
https://www.opengl.org/registry/specs/ARB/program_interface_query.txt

So, in my current understanding, I should either just return loc + 
VERT_ATTRIB_GENERIC0 if loc < 0 or get rid of the offset subtraction 
from program_resource_location(), this will make GetActiveAttrib to work 
in different way though than before so some Piglit tests may need to be 
updated as well.

Ian, any comments on this?

// Tapani