[Mesa-dev] [PATCH v2 01/24] linker: fix varying linking if SSO program has only gs and fs

[Martin Peres]

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.palli at 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];