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

[Timothy Arceri]

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.

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