On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri
<timothy.arc...@collabora.com> wrote:
> Previously we would pack varyings before trying to remove them, this
> relied on the packing pass not packing varyings with a location of -1
> to avoid packing varyings that should be removed.
> However this meant unused varyings with an explicit location would been
> packed before they could be removed when we enable packing of them in a
> later patch.
>
> V2: fix regression in V1 removing unused varyings in multi-stage SSO,
> fix regression with single stage programs.
> ---
> src/glsl/link_varyings.cpp | 45 +++++++++++++++++++++++++++++++++++++++
> src/glsl/link_varyings.h | 5 +++++
> src/glsl/linker.cpp | 52
> ++++------------------------------------------
> 3 files changed, 54 insertions(+), 48 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 961c3d7..2ff4552 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -309,6 +309,41 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> }
> }
>
> +/**
> + * Demote shader inputs and outputs that are not used in other stages, and
> + * remove them via dead code elimination.
> + */
> +void
> +remove_unused_shader_inputs_and_outputs(bool is_separate_shader_object,
> + gl_shader *sh,
> + enum ir_variable_mode mode)
> +{
> + if (is_separate_shader_object)
> + return;
> +
> + foreach_in_list(ir_instruction, node, sh->ir) {
> + ir_variable *const var = node->as_variable();
> +
> + if ((var == NULL) || (var->data.mode != int(mode)))
> + continue;
> +
> + /* A shader 'in' or 'out' variable is only really an input or output if
> + * its value is used by other shader stages. This will cause the
> + * variable to have a location assigned.
> + */
> + if (var->data.is_unmatched_generic_inout) {
> + assert(var->data.mode != ir_var_temporary);
> + var->data.mode = ir_var_auto;
> + }
> + }
> +
> + /* Eliminate code that is now dead due to unused inputs/outputs being
> + * demoted.
> + */
> + while (do_dead_code(sh->ir, false))
> + ;
> +
> +}
>
> /**
> * Initialize this object based on a string that was passed to
> @@ -1682,6 +1717,16 @@ assign_varying_locations(struct gl_context *ctx,
> }
> }
> }
> +
> + /* Now that validation is done its safe to remove unused varyings. As
> + * we have both a producer and consumer its safe to remove unused
> + * varyings even if the program is a SSO because the stages are being
> + * linked together i.e. we have a multi-stage SSO.
> + */
> + remove_unused_shader_inputs_and_outputs(false, producer,
> + ir_var_shader_out);
> + remove_unused_shader_inputs_and_outputs(false, consumer,
> + ir_var_shader_in);
> }
>
> if (!disable_varying_packing) {
> diff --git a/src/glsl/link_varyings.h b/src/glsl/link_varyings.h
> index 1d12978..b2812614 100644
> --- a/src/glsl/link_varyings.h
> +++ b/src/glsl/link_varyings.h
> @@ -268,6 +268,11 @@ parse_tfeedback_decls(struct gl_context *ctx, struct
> gl_shader_program *prog,
> const void *mem_ctx, unsigned num_names,
> char **varying_names, tfeedback_decl *decls);
>
> +void
> +remove_unused_shader_inputs_and_outputs(bool is_separate_shader_object,
> + gl_shader *sh,
> + enum ir_variable_mode mode);
> +
> bool
> store_tfeedback_info(struct gl_context *ctx, struct gl_shader_program *prog,
> unsigned num_tfeedback_decls,
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index d11c404..31d55e3 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -2723,30 +2723,6 @@ match_explicit_outputs_to_inputs(struct
> gl_shader_program *prog,
> }
>
> /**
> - * Demote shader inputs and outputs that are not used in other stages
> - */
> -void
> -demote_shader_inputs_and_outputs(gl_shader *sh, enum ir_variable_mode mode)
> -{
> - foreach_in_list(ir_instruction, node, sh->ir) {
> - ir_variable *const var = node->as_variable();
> -
> - if ((var == NULL) || (var->data.mode != int(mode)))
> - continue;
> -
> - /* A shader 'in' or 'out' variable is only really an input or output if
> - * its value is used by other shader stages. This will cause the
> variable
> - * to have a location assigned.
> - */
> - if (var->data.is_unmatched_generic_inout) {
> - assert(var->data.mode != ir_var_temporary);
> - var->data.mode = ir_var_auto;
> - }
> - }
> -}
> -
> -
> -/**
> * Store the gl_FragDepth layout in the gl_shader_program struct.
> */
> static void
> @@ -4446,14 +4422,8 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> do_dead_builtin_varyings(ctx, sh, NULL,
> num_tfeedback_decls, tfeedback_decls);
>
> - if (!prog->SeparateShader) {
> - demote_shader_inputs_and_outputs(sh, ir_var_shader_out);
> - /* Eliminate code that is now dead due to unused outputs being
> - * demoted.
> - */
> - while (do_dead_code(sh->ir, false))
> - ;
> - }
> + remove_unused_shader_inputs_and_outputs(prog->SeparateShader, sh,
> + ir_var_shader_out);
> }
> else if (first == MESA_SHADER_FRAGMENT) {
> /* If the program only contains a fragment shader...
> @@ -4471,12 +4441,8 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> NULL /* tfeedback_decls */))
> goto done;
> } else {
> - demote_shader_inputs_and_outputs(sh, ir_var_shader_in);
> - /* Eliminate code that is now dead due to unused inputs being
> - * demoted.
> - */
> - while (do_dead_code(sh->ir, false))
> - ;
> + remove_unused_shader_inputs_and_outputs(false, sh,
> + ir_var_shader_in);
> }
> }
>
> @@ -4497,16 +4463,6 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
> tfeedback_decls);
>
> - demote_shader_inputs_and_outputs(sh_i, ir_var_shader_out);
> - demote_shader_inputs_and_outputs(sh_next, ir_var_shader_in);
> -
> - /* Eliminate code that is now dead due to unused outputs being demoted.
> - */
> - while (do_dead_code(sh_i->ir, false))
> - ;
> - while (do_dead_code(sh_next->ir, false))
> - ;
> -
> /* This must be done after all dead varyings are eliminated. */
> if (!check_against_output_limit(ctx, prog, sh_i))
> goto done;
> --
> 2.4.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Looks like the right thing to do.
Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
