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.pa...@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_arc...@yahoo.com.au>
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
