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

[Tapani Pälli]

On 08/05/2015 09:01 AM, Timothy Arceri wrote:
> On Tue, 2015-08-04 at 21:20 +1000, Timothy Arceri wrote:
>> On Tue, 2015-08-04 at 08:24 +0300, Tapani Pälli wrote:
>>> 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?
>> Actually we don't need to generate a stageref for
>>   GL_TRANSFORM_FEEDBACK_VARYING
>>
>> The build_stageref() call should be removed and 0 passed to
>>   add_program_resource()
>>
>> Then you could just pass ir_var_uniform when generating stageref for
>> uniforms
>> then you can just do.
>>
>>     if (var->data.mode != mode)
>>        continue;
>>
>> I think it would be nice to get this right first time round, since it should
>> be a small change.
> Did you see my last reply??

Sorry I missed it. Anyways, this can live as separate change as it is 
cleaning up and not fixing anything that would be broken.


// Tapani