On 01/04/15 15:14, Tapani Pälli wrote:
Previously linker did not take in to account case where one would
have only gs and fs (with SSO), patch adds the case by refactoring
code around assign_varying_locations. This makes sure locations for
gs get populated correctly.

This was found with some of the SSO subtests of Martin's upcoming
GetProgramInterfaceiv Piglit test which passes with the patch, no
Piglit regressions.

Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
---
  src/glsl/linker.cpp | 32 +++++++++++++++++++-------------
  1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 85830e6..73432b2 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -2726,10 +2726,19 @@ link_shaders(struct gl_context *ctx, struct 
gl_shader_program *prog)
        goto done;
     }
- unsigned first;
-   for (first = 0; first <= MESA_SHADER_FRAGMENT; first++) {
-      if (prog->_LinkedShaders[first] != NULL)
-        break;
+   unsigned first, last;
+
+   first = MESA_SHADER_STAGES;
+   last = 0;
+
+   /* Determine first and last stage. */
+   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
+      struct gl_shader *sh = prog->_LinkedShaders[i];
Why create this variable?
+      if (!sh)
+         continue;
+      if (first == MESA_SHADER_STAGES)
+         first = i;
+      last = i;
     }
if (num_tfeedback_decls != 0) {
@@ -2758,13 +2767,9 @@ link_shaders(struct gl_context *ctx, struct 
gl_shader_program *prog)
      * ensures that inter-shader outputs written to in an earlier stage are
      * eliminated if they are (transitively) not used in a later stage.
      */
-   int last, next;
-   for (last = MESA_SHADER_FRAGMENT; last >= 0; last--) {
-      if (prog->_LinkedShaders[last] != NULL)
-         break;
-   }
+   int next;

So, the above is a cleanup for finding the first and last shader stage. It is however not necessary.
- if (last >= 0 && last < MESA_SHADER_FRAGMENT) {
+   if (first < MESA_SHADER_FRAGMENT) {
        gl_shader *const sh = prog->_LinkedShaders[last];
if (first == MESA_SHADER_GEOMETRY) {
@@ -2776,13 +2781,14 @@ link_shaders(struct gl_context *ctx, struct 
gl_shader_program *prog)
            * MESA_SHADER_GEOMETRY.
            */
           if (!assign_varying_locations(ctx, mem_ctx, prog,
-                                       NULL, sh,
+                                       NULL, prog->_LinkedShaders[first],
The above change should not change anything because first == last == MESA_SHADER_GEOMETRY. Please get rid of it if I am right.
                                         num_tfeedback_decls, tfeedback_decls,
                                         prog->Geom.VerticesIn))
              goto done;
        }
- if (num_tfeedback_decls != 0 || prog->SeparateShader) {
+      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.
            */
@@ -2804,7 +2810,7 @@ link_shaders(struct gl_context *ctx, struct 
gl_shader_program *prog)
        while (do_dead_code(sh->ir, false))
           ;
     }
-   else if (first == MESA_SHADER_FRAGMENT) {
+   else if (first == MESA_SHADER_FRAGMENT && first == last) {
How could first != last since fragment is the last stage anyway? Why did you add this test?
        /* If the program only contains a fragment shader...
         */
        gl_shader *const sh = prog->_LinkedShaders[first];

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to