Re: [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 
> Cc: Chris Forbes 
> ---
>  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 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Timothy Arceri]

On Tue, 2016-02-09 at 01:29 -0800, Kenneth Graunke wrote:
> 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 
> > Cc: Chris Forbes 
> > ---
> >  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.

Since the existing code was so hard to follow I wrote this by seeing
how little code I could get away with and testing until all regressions
went away. Anyway it wasn't harmless when compute reached this code,
although I don't recall exactly what the problem was as it was a couple
of weeks ago.

> 
> > +  /* 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.  

[Mesa-dev] [PATCH V2 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.

V2: Add more code comments, always try to remove unused inputs
to the first stage.

Cc: Ilia Mirkin 
Cc: Chris Forbes 
---
 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) {
+  /* 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);
-   }
-   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 */
+  if (first == 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 {
- remove_unused_shader_inputs_and_outputs(false, sh,
- ir_var_shader_in);
-  }
-   }
+ gl_shader *const sh = prog->_LinkedShaders[last];
+ if (prog->SeparateShader) {
+/* Assign input locations for SSO, output locations are already
+ * assigned.
+ */
+if (!assign_varying_locations(ctx, mem_ctx, prog,
+