[Mesa-dev] [PATCH V2 2/2] glsl: clean up and fix bug in varying linking rules

[Kenneth Graunke]

On Tuesday, February 2, 2016 12:20:01 PM PST Timothy Arceri wrote:
> The existing code was very hard to follow and has been the source
> of at least 3 bugs in the past year.
> 
> The existing code also has a bug for SSO where if we have a
> multi-stage SSO for example a tes -> gs program, if we try to use
> transform feedback with gs the existing code would look for the
> transform feedback varyings in the tes stage and fail as it can't
> find them.
> 
> V2: Add more code comments, always try to remove unused inputs
> to the first stage.
> 
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: Chris Forbes <chrisf at ijw.co.nz>
> ---
>  src/compiler/glsl/linker.cpp | 137 ++++++++++++++++++++-----------------------
>  1 file changed, 63 insertions(+), 74 deletions(-)
> 
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index fdfdcaa..09323bb 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -4461,91 +4461,80 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>           goto done;
>     }
>  
> -   /* Linking the stages in the opposite order (from fragment to vertex)
> -    * ensures that inter-shader outputs written to in an earlier stage are
> -    * eliminated if they are (transitively) not used in a later stage.
> +   /* If there is no fragment shader we need to set transform feedback.
> +    *
> +    * For SSO we need also need to assign output locations, we assign them
> +    * here because we need to do it for both single stage programs and multi
> +    * stage programs.
>      */
> -   int next;
> -
> -   if (first < MESA_SHADER_FRAGMENT) {
> -      gl_shader *const sh = prog->_LinkedShaders[last];
> -
> -      if (first != MESA_SHADER_VERTEX) {
> -         /* There was no vertex shader, but we still have to assign varying
> -          * locations for use by tessellation/geometry shader inputs in SSO.
> -          *
> -          * If the shader is not separable (i.e., prog->SeparateShader is
> -          * false), linking will have already failed when first is not
> -          * MESA_SHADER_VERTEX.
> -          */
> -         if (!assign_varying_locations(ctx, mem_ctx, prog,
> -                                       NULL, prog->_LinkedShaders[first],
> -                                       num_tfeedback_decls, tfeedback_decls))
> -            goto done;
> -      }
> -
> -      if (last != MESA_SHADER_FRAGMENT &&
> -         (num_tfeedback_decls != 0 || prog->SeparateShader)) {
> -         /* There was no fragment shader, but we still have to assign varying
> -          * locations for use by transform feedback.
> -          */
> -         if (!assign_varying_locations(ctx, mem_ctx, prog,
> -                                       sh, NULL,
> -                                       num_tfeedback_decls, tfeedback_decls))
> -            goto done;
> -      }
> -
> -      do_dead_builtin_varyings(ctx, sh, NULL,
> -                               num_tfeedback_decls, tfeedback_decls);
> +   if (last < MESA_SHADER_FRAGMENT &&
> +       (num_tfeedback_decls != 0 || prog->SeparateShader)) {
> +      if (!assign_varying_locations(ctx, mem_ctx, prog,
> +                                    prog->_LinkedShaders[last], NULL,
> +                                    num_tfeedback_decls, tfeedback_decls))
> +         goto done;
> +   }
>  
> -      remove_unused_shader_inputs_and_outputs(prog->SeparateShader, sh,
> +   if (last <= MESA_SHADER_FRAGMENT) {

I don't understand the point of last <= MESA_SHADER_FRAGMENT here.
The only other option is MESA_SHADER_COMPUTE, which has no inputs or
outputs, so calling this should be harmless.

> +      /* Remove unused varyings from the first/last stage unless SSO */
> +      remove_unused_shader_inputs_and_outputs(prog->SeparateShader,
> +                                              prog->_LinkedShaders[first],
> +                                              ir_var_shader_in);
> +      remove_unused_shader_inputs_and_outputs(prog->SeparateShader,
> +                                              prog->_LinkedShaders[last],
>                                                ir_var_shader_out);

I see, assign_varying_locations calls this for internal stages, when
there's a producer and consumer - this just calls it for the boundary
shaders in the !SSO case.  Makes sense.

This is much cleaner - nice work!

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160209/6bc4f68b/attachment-0001.sig>