[Mesa-dev] [PATCH 8/8] glsl/linker: Ensure the first stage of an SSO pipeline has input locs assigned

[Timothy Arceri]

On Wed, 2016-05-18 at 20:59 +1000, Timothy Arceri wrote:
> On Wed, 2016-05-18 at 20:44 +1000, Timothy Arceri wrote:
> > 
> > On Tue, 2016-05-17 at 20:24 -0700, Ian Romanick wrote:
> > > 
> > > 
> > > On 05/17/2016 04:40 PM, Timothy Arceri wrote:
> > > > 
> > > > 
> > > > 
> > > > On Tue, 2016-05-17 at 15:11 -0700, Ian Romanick wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > From: Ian Romanick <ian.d.romanick at intel.com>
> > > > > 
> > > > > Previously an SSO pipeline containing only a tessellation
> > > > > control
> > > > > shader
> > > > > and a tessellation evaluation shader would not get locations
> > > > > assigned
> > > > > for the TCS inputs.  This would lead to assertion failures in
> > > > > some
> > > > > piglit tests, such as arb_program_interface_query-resource-
> > > > > query.
> > > > > 
> > > > > That piglit test still fails on some tessellation related
> > > > > subtests.
> > > > > Specifically, these subtests fail:
> > > > > 
> > > > > 'GL_PROGRAM_INPUT(tcs) active resources' expected 2 but got 3
> > > > > 'GL_PROGRAM_INPUT(tcs) max length name' expected 12 but got
> > > > > 16
> > > > > 'GL_PROGRAM_INPUT(tcs,tes) active resources' expected 2 but
> > > > > got
> > > > > 3
> > > > > 'GL_PROGRAM_INPUT(tcs,tes) max length name' expected 12 but
> > > > > got
> > > > > 16
> > > > > 'GL_PROGRAM_OUTPUT(tcs) active resources' expected 15 but got
> > > > > 3
> > > > > 'GL_PROGRAM_OUTPUT(tcs) max length name' expected 23 but got
> > > > > 12
> > > > > 
> > > > > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > > > > ---
> > > > >  src/compiler/glsl/linker.cpp | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/src/compiler/glsl/linker.cpp
> > > > > b/src/compiler/glsl/linker.cpp
> > > > > index 34b4a81..d277dd0 100644
> > > > > --- a/src/compiler/glsl/linker.cpp
> > > > > +++ b/src/compiler/glsl/linker.cpp
> > > > > @@ -4790,7 +4790,7 @@ link_shaders(struct gl_context *ctx,
> > > > > struct
> > > > > gl_shader_program *prog)
> > > > >            */
> > > > >           int next = last;
> > > > >           for (int i = next - 1; i >= 0; i--) {
> > > > > -            if (prog->_LinkedShaders[i] == NULL)
> > > > > +            if (prog->_LinkedShaders[i] == NULL && i != 0)
> > > > Up to you but I think i != MESA_SHADER_VERTEX makes it slightly
> > > > easier
> > > > to follow.
> > > I'm a little bit torn.  I thought about using MESA_SHADER_VERTEX,
> > > but
> > > that's not really the information that's important.  What's
> > > important
> > > is
> > > always executing at the loop bound (which is 0) even when there's
> > > no
> > > shader there.
> > In that case I don't think this won't work as intended. The
> > _LinkedShaders array is defined as:
> > 
> >    _LinkedShaders[MESA_SHADER_STAGES]
> > 
> > Here a zero index is the first GL stage not the first stage in the
> > program. So a program containing only geom and frag stages would
> > hit
> > the same problem.

Ignore this I see how it it intended to work now :P maybe I should go
to bed.

Now that I understand it more clearly I'm happy for you to leave it as
zero.

> > 
> > I think we need to do something like what is done with the last
> > stage
> > above this code e.g.
> > 
> >    if (first < MESA_SHADER_VERTEX && prog->SeparateShader) {
> Whoops that should be first > MESA_SHADER_VERTEX
> 
> > 
> >       if (!assign_varying_locations(...))
> >          goto done;
> >    }
> > 
> > I wonder if that code should be
> > calling check_against_output_limit()
> > also. I guess we probably have no SSO tests that exceed the limit.
> > 
> > > 
> > > 
> > > 
> > > I think I'd want to either leave both i comparisons with zero or
> > > change
> > > both to MESA_SHADER_VERTEX.  Do you have an opinion?
> > > 
> > > > 
> > > > 
> > > > 
> > > > Otherwise the series is Reviewed-by: Timothy Arceri
> > > > <timothy.arceri at collabora.com>
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > >                 continue;
> > > > >  
> > > > >              gl_shader *const sh_i = prog->_LinkedShaders[i];
> > > > > @@ -4806,8 +4806,11 @@ link_shaders(struct gl_context *ctx,
> > > > > struct
> > > > > gl_shader_program *prog)
> > > > >                        tfeedback_decls);
> > > > >  
> > > > >              /* This must be done after all dead varyings are
> > > > > eliminated. */
> > > > > -            if (!check_against_output_limit(ctx, prog,
> > > > > sh_i))
> > > > > -               goto done;
> > > > > +            if (sh_i != NULL) {
> > > > > +               if (!check_against_output_limit(ctx, prog,
> > > > > sh_i))
> > > > > {
> > > > > +                  goto done;
> > > > > +               }
> > > > > +            }
> > > > >              if (!check_against_input_limit(ctx, prog,
> > > > > sh_next))
> > > > >                 goto done;
> > > > >  
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev