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

[Timothy Arceri]

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.
---
 src/glsl/linker.cpp | 133 +++++++++++++++++++++++-----------------------------
 1 file changed, 59 insertions(+), 74 deletions(-)

diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 927e2f3..efde224 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -4472,91 +4472,76 @@ 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 varying locations.
     */
-   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) {
+      remove_unused_shader_inputs_and_outputs(prog->SeparateShader,
+                                              prog->_LinkedShaders[last],
                                               ir_var_shader_out);
-   }
-   else if (first == MESA_SHADER_FRAGMENT) {
-      /* If the program only contains a fragment shader...
-       */
-      gl_shader *const sh = prog->_LinkedShaders[first];
 
-      do_dead_builtin_varyings(ctx, NULL, sh,
-                               num_tfeedback_decls, tfeedback_decls);
+      /* If the program is made up of only a single stage assign locations if
+       * its a SSO or remove unused inputs for a fragment only program.
+       */
+      if (first == last) {
+         gl_shader *const sh = prog->_LinkedShaders[last];
+
+         if (prog->SeparateShader) {
+            if (!assign_varying_locations(ctx, mem_ctx, prog,
+                                          NULL /* producer */,
+                                          sh /* consumer */,
+                                          0 /* num_tfeedback_decls */,
+                                          NULL /* tfeedback_decls */))
+               goto done;
+         } else {
+            if (first == MESA_SHADER_FRAGMENT)
+               remove_unused_shader_inputs_and_outputs(false, sh,
+                                                       ir_var_shader_in);
+         }
 
-      if (prog->SeparateShader) {
-         if (!assign_varying_locations(ctx, mem_ctx, prog,
-                                       NULL /* producer */,
-                                       sh /* consumer */,
-                                       0 /* num_tfeedback_decls */,
-                                       NULL /* tfeedback_decls */))
-            goto done;
+         do_dead_builtin_varyings(ctx, NULL, sh, 0, NULL);
+         do_dead_builtin_varyings(ctx, sh, NULL, num_tfeedback_decls,
+                                  tfeedback_decls);
       } else {
-         remove_unused_shader_inputs_and_outputs(false, sh,
-                                                 ir_var_shader_in);
-      }
-   }
-
-   next = last;
-   for (int i = next - 1; i >= 0; i--) {
-      if (prog->_LinkedShaders[i] == NULL)
-         continue;
+         /* 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.
+          */
+         int next = last;
+         for (int i = next - 1; i >= 0; i--) {
+            if (prog->_LinkedShaders[i] == NULL)
+               continue;
 
-      gl_shader *const sh_i = prog->_LinkedShaders[i];
-      gl_shader *const sh_next = prog->_LinkedShaders[next];
+            gl_shader *const sh_i = prog->_LinkedShaders[i];
+            gl_shader *const sh_next = prog->_LinkedShaders[next];
 
-      if (!assign_varying_locations(ctx, mem_ctx, prog, sh_i, sh_next,
-                next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
-                tfeedback_decls))
-         goto done;
+            if (!assign_varying_locations(ctx, mem_ctx, prog, sh_i, sh_next,
+                      next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
+                      tfeedback_decls))
+               goto done;
 
-      do_dead_builtin_varyings(ctx, sh_i, sh_next,
-                next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
-                tfeedback_decls);
+            do_dead_builtin_varyings(ctx, sh_i, sh_next,
+                      next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
+                      tfeedback_decls);
 
-      /* This must be done after all dead varyings are eliminated. */
-      if (!check_against_output_limit(ctx, prog, sh_i))
-         goto done;
-      if (!check_against_input_limit(ctx, prog, sh_next))
-         goto done;
+            /* This must be done after all dead varyings are eliminated. */
+            if (!check_against_output_limit(ctx, prog, sh_i))
+               goto done;
+            if (!check_against_input_limit(ctx, prog, sh_next))
+               goto done;
 
-      next = i;
+            next = i;
+         }
+      }
    }
 
    if (!store_tfeedback_info(ctx, prog, num_tfeedback_decls, tfeedback_decls))
-- 
2.4.3