[Mesa-dev] [PATCH v2] nir: add new linking opt nir_move_out_const_to_consumer()

[Samuel Pitoiset]

On 11/7/18 9:36 AM, Timothy Arceri wrote:
> This pass moves constant outputs to the consuming shader stage
> where possible.
> 
> V2: limit pass to scalars for now
> ---
> 
>   V2 doesn't change any shader-db/vkpipeline-db results as all 32bit
>   varyings that we don't skip are already scalar. V2 just avoids a
>   potential bug with doubles as we don't currently split those in
>   nir_lower_io_to_scalar_early().
> 
>   src/compiler/nir/nir.h                    |   2 +
>   src/compiler/nir/nir_linking_helpers.c    | 110 ++++++++++++++++++++++
>   src/mesa/state_tracker/st_glsl_to_nir.cpp |   2 +-
>   3 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index a0ae9a4430e..0b8c8578953 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2800,6 +2800,8 @@ bool nir_remove_unused_io_vars(nir_shader *shader, struct exec_list *var_list,
>   void nir_compact_varyings(nir_shader *producer, nir_shader *consumer,
>                             bool default_to_smooth_interp);
>   void nir_link_xfb_varyings(nir_shader *producer, nir_shader *consumer);
> +bool nir_move_out_const_to_consumer(nir_shader *producer,
> +                                    nir_shader *consumer);
>   
>   typedef enum {
>      /* If set, this forces all non-flat fragment shader inputs to be
> diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
> index de6f2481def..a03c327a12f 100644
> --- a/src/compiler/nir/nir_linking_helpers.c
> +++ b/src/compiler/nir/nir_linking_helpers.c
> @@ -22,6 +22,7 @@
>    */
>   
>   #include "nir.h"
> +#include "nir_builder.h"
>   #include "util/set.h"
>   #include "util/hash_table.h"
>   
> @@ -556,3 +557,112 @@ nir_link_xfb_varyings(nir_shader *producer, nir_shader *consumer)
>         }
>      }
>   }
> +
> +static bool
> +try_replace_constant_input(nir_shader *shader,
> +                           nir_intrinsic_instr *store_intr)
> +{
> +   nir_variable *out_var =
> +      nir_deref_instr_get_variable(nir_src_as_deref(store_intr->src[0]));
> +
> +   if (out_var->data.mode != nir_var_shader_out)
> +      return false;
> +
> +   /* Skip types that require more complex handling.
> +    * TODO: add support for these types.
> +    */
> +   if (glsl_type_is_array(out_var->type) ||
> +       glsl_type_is_dual_slot(out_var->type) ||
> +       glsl_type_is_matrix(out_var->type) ||
> +       glsl_type_is_struct(out_var->type))
> +      return false;
> +
> +   /* Limit this pass to scalars for now to keep things simple. Most varyings
> +    * should have been lowered to scalars at this point anyway.
> +    */
> +   if (store_intr->num_components != 1)
> +      return false;
> +
> +   if (out_var->data.location < VARYING_SLOT_VAR0 ||
> +       out_var->data.location - VARYING_SLOT_VAR0 >= MAX_VARYING)
> +      return false;
> +
> +   nir_function_impl *impl = nir_shader_get_entrypoint(shader);
> +
> +   nir_builder b;
> +   nir_builder_init(&b, impl);
> +
> +   bool progress = false;
> +   nir_foreach_block(block, impl) {
> +      nir_foreach_instr(instr, block) {
> +         if (instr->type != nir_instr_type_intrinsic)
> +            continue;
> +
> +         nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
> +         if (intr->intrinsic != nir_intrinsic_load_deref)
> +            continue;
> +
> +         nir_variable *in_var =
> +            nir_deref_instr_get_variable(nir_src_as_deref(intr->src[0]));
> +
> +         if (in_var->data.mode != nir_var_shader_in)
> +            continue;
> +
> +         if (in_var->data.location != out_var->data.location ||
> +             in_var->data.location_frac != out_var->data.location_frac)
> +            continue;
> +
> +         b.cursor = nir_before_instr(instr);
> +
> +         nir_load_const_instr *out_const =
> +            nir_instr_as_load_const(store_intr->src[1].ssa->parent_instr);
> +
> +         /* Add new const to replace the input */
> +         nir_ssa_def *nconst = nir_build_imm(&b, store_intr->num_components,
> +                                             intr->dest.ssa.bit_size,
> +                                             out_const->value);
> +
> +         nir_ssa_def_rewrite_uses(&intr->dest.ssa, nir_src_for_ssa(nconst));
> +
> +         progress = true;
> +      }
> +   }
> +
> +   return progress;
> +}
> +
> +bool
> +nir_move_out_const_to_consumer(nir_shader *producer, nir_shader *consumer)
> +{
> +   /* TODO: Add support for more shader stage combinations */
> +   if (consumer->info.stage != MESA_SHADER_FRAGMENT ||
> +       (producer->info.stage != MESA_SHADER_VERTEX &&
> +        producer->info.stage != MESA_SHADER_TESS_EVAL))
> +      return false;

I would suggest to only enable it for VS->FS for now because I think 
it's the most important point, also because it might be trickier for 
other stages.

> +
> +   bool progress = false;
> +
> +   nir_function_impl *impl = nir_shader_get_entrypoint(producer);
> +
> +   /* If we find a store in the last block of the producer we can be sure this
> +    * is the only possible value for this output.
> +    */
> +   nir_block *last_block = nir_impl_last_block(impl);
> +   nir_foreach_instr_reverse(instr, last_block) {
> +      if (instr->type != nir_instr_type_intrinsic)
> +         continue;
> +
> +      nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
> +
> +      if (intr->intrinsic != nir_intrinsic_store_deref)
> +         continue;
> +
> +      if (intr->src[1].ssa->parent_instr->type != nir_instr_type_load_const) {
> +         continue;
> +      }

useless braces.

> +
> +      progress |= try_replace_constant_input(consumer, intr);
> +   }
> +
> +   return progress;
> +}
> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> index dcf8c2b638e..9141acbc4df 100644
> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> @@ -757,7 +757,7 @@ st_link_nir(struct gl_context *ctx,
>             */
>            if (!prev_shader->sh.LinkedTransformFeedback)
>               nir_compact_varyings(shader_program->_LinkedShaders[prev]->Program->nir,
> -                              nir, ctx->API != API_OPENGL_COMPAT);
> +                                 nir, ctx->API != API_OPENGL_COMPAT);
>         }
>         prev = i;
>      }
>