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

[Timothy Arceri]

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?


> >           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