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 <imir...@alum.mit.edu>
> > Cc: Chris Forbes <chr...@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.
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. Makes sense.
Right.
>
> This is much cleaner - nice work!
>
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
Thanks.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
