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

[Timothy Arceri]

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