[Mesa-dev] [PATCH 1/2] glsl: add variable mode check to build_stageref

[Tapani Pälli]

On 08/04/2015 01:27 AM, Timothy Arceri wrote:
> On Mon, 2015-08-03 at 23:16 +1000, Timothy Arceri wrote:
>> On Mon, 2015-08-03 at 09:02 +0300, Tapani Pälli wrote:
>>> Currently stage reference mask is built using the variable name
>>> only. However it can happen that input of one stage has same name
>>> as output from another stage. Adding check of variable mode makes
>>> sure we do not pick wrong variable.
>>>
>>> Fixes some subcases from
>>>     ES31-CTS.program_interface_query.no-locations
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>   src/glsl/linker.cpp | 17 +++++++++++++----
>>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>> index a16dab4..e2da0af 100644
>>> --- a/src/glsl/linker.cpp
>>> +++ b/src/glsl/linker.cpp
>>> @@ -3110,7 +3110,8 @@ add_program_resource(struct gl_shader_program *prog,
>>>
>>> GLenum type,
>>>    * Function builds a stage reference bitmask from variable name.
>>>    */
>>>   static uint8_t
>>> -build_stageref(struct gl_shader_program *shProg, const char *name)
>>> +build_stageref(struct gl_shader_program *shProg, const char *name,
>>> +               unsigned mode)
>>>   {
>>>      uint8_t stages = 0;
>>>
>>> @@ -3131,6 +3132,13 @@ build_stageref(struct gl_shader_program *shProg,
>>> const char *name)
>>>            ir_variable *var = node->as_variable();
>>>            if (var) {
>>>               unsigned baselen = strlen(var->name);
>>> +
>>> +            /* Type needs to match if specified, otherwise we might
>>> +             * pick a variable with same name but different interface.
>>> +             */
>>> +            if (mode != 0 && var->data.mode != mode)
>>> +               continue;
>>> +
>>>               if (strncmp(var->name, name, baselen) == 0) {
>>>                  /* Check for exact name matches but also check for arrays
>>> and
>>>                   * structs.
>>> @@ -3188,7 +3196,8 @@ add_interface_variables(struct gl_shader_program
>>> *shProg,
>>>         };
>>>
>>>         if (!add_program_resource(shProg, programInterface, var,
>>> -                                build_stageref(shProg, var->name) |
>>> mask))
>>> +                                build_stageref(shProg, var->name,
>>> +                                               var->data.mode) | mask))
>>>            return false;
>>>      }
>>>      return true;
>>> @@ -3241,7 +3250,7 @@ build_program_resource_list(struct gl_context *ctx,
>>>         for (int i = 0; i < shProg->LinkedTransformFeedback.NumVarying;
>>> i++)
>>> {
>>>            uint8_t stageref =
>>>               build_stageref(shProg,
>>> -                           shProg
>>> ->LinkedTransformFeedback.Varyings[i].Name);
>>> +                           shProg
>>> ->LinkedTransformFeedback.Varyings[i].Name, 0);
>
> Looking at this again won't transform feedback varyings potentially have the
> same problem with inputs in other stages. Also wouldn't the current code
> reference the fs if there are outputs with the same name in the fs.
>
> Maybe build_stageref() shouldn't be used here and we should have a
> LinkedTransformFeedback.Varyings[i].stageref that is populated when the
> transformfeedback struct is filled. What do you think?

Yep, this is possible. Would be cool to first have a test case to hit 
this. Would it be ok to do this as follow-up work?

>
>>>            if (!add_program_resource(shProg, GL_TRANSFORM_FEEDBACK_VARYING,
>>>                                      &shProg
>>> ->LinkedTransformFeedback.Varyings[i],
>>>                                      stageref))
>>> @@ -3256,7 +3265,7 @@ build_program_resource_list(struct gl_context *ctx,
>>>            continue;
>>>
>>>         uint8_t stageref =
>>> -         build_stageref(shProg, shProg->UniformStorage[i].name);
>>> +         build_stageref(shProg, shProg->UniformStorage[i].name, 0);
>>
>> You could pass ir_var_uniform here rather than 0 it might save a few string
>> compares, up to you.
>>
>>>
>>>         /* Add stagereferences for uniforms in a uniform block. */
>>>         int block_index = shProg->UniformStorage[i].block_index;
>>
>>
>> It seems like there should be a cleaner way to do this but I've been over it
>> a
>> few times now and can't come up with anything better. I think its just the
>> mode != 0 thats bugging me.
>>
>> Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev