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

[Timothy Arceri]

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.

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) {
      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;
> > >