[Mesa-dev] [PATCH] nir: Add a nir_lower_load_const_to_scalar() pass.

[Matt Turner]

On Mon, Aug 3, 2015 at 5:44 PM, Eric Anholt <eric at anholt.net> wrote:
> This is useful to increase the CSE opportunities for a scalar backend.  It
> avoids regressions when dropping vc4's custom CSE implementation.
> ---
>  src/glsl/Makefile.sources                     |   1 +
>  src/glsl/nir/nir.h                            |   1 +
>  src/glsl/nir/nir_lower_load_const_to_scalar.c | 106 ++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
>  create mode 100644 src/glsl/nir/nir_lower_load_const_to_scalar.c
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 93f4e48..a0e85ed 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -33,6 +33,7 @@ NIR_FILES = \
>         nir/nir_lower_alu_to_scalar.c \
>         nir/nir_lower_atomics.c \
>         nir/nir_lower_global_vars_to_local.c \
> +       nir/nir_lower_load_const_to_scalar.c \
>         nir/nir_lower_locals_to_regs.c \
>         nir/nir_lower_idiv.c \
>         nir/nir_lower_io.c \
> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
> index 62cdbd4..a1b6fe7 100644
> --- a/src/glsl/nir/nir.h
> +++ b/src/glsl/nir/nir.h
> @@ -1652,6 +1652,7 @@ void nir_remove_dead_variables(nir_shader *shader);
>
>  void nir_lower_vec_to_movs(nir_shader *shader);
>  void nir_lower_alu_to_scalar(nir_shader *shader);
> +void nir_lower_load_const_to_scalar(nir_shader *shader);
>
>  void nir_lower_phis_to_scalar(nir_shader *shader);
>
> diff --git a/src/glsl/nir/nir_lower_load_const_to_scalar.c b/src/glsl/nir/nir_lower_load_const_to_scalar.c
> new file mode 100644
> index 0000000..ff40983
> --- /dev/null
> +++ b/src/glsl/nir/nir_lower_load_const_to_scalar.c
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright © 2015 Broadcom
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "nir.h"
> +#include "nir_builder.h"
> +
> +/** @file nir_lower_load_const_to_scalar.c
> + *
> + * Replaces vector nir_load_const instructions with a series of loads and a
> + * vec[234] to reconstruct the original vector (on the assumption that
> + * nir_lower_alu_to_scalar() will then be used to split it up).
> + *
> + * This gives NIR a chance to CSE more operations on a scalar shader, when the
> + * same value was used in different vector contant loads.
> + */
> +
> +static void
> +lower_load_const_instr_scalar(nir_load_const_instr *lower)
> +{
> +   unsigned i;

Seeing this declaration here made me think it might be used after the
for loop. It's not, so probably just declare i in the for loop.

> +
> +   if (lower->def.num_components == 1)
> +      return;

You might put this as part of the conditional in
lower_load_const_to_scalar_block(). No real preference from me.

> +
> +   nir_builder b;
> +   nir_builder_init(&b, nir_cf_node_get_function(&lower->instr.block->cf_node));
> +   nir_builder_insert_before_instr(&b, &lower->instr);
> +
> +   /* Emit the individual loads. */
> +   nir_ssa_def *loads[4];
> +   for (i = 0; i < lower->def.num_components; i++) {
> +      nir_load_const_instr *load_comp = nir_load_const_instr_create(b.shader, 1);
> +      load_comp->value.u[0] = lower->value.u[i];
> +      nir_builder_instr_insert(&b, &load_comp->instr);
> +      loads[i] = &load_comp->def;
> +   }
> +
> +   /* Batch things back together into a vector. */
> +   nir_ssa_def *vec;
> +   switch (lower->def.num_components) {
> +   case 2:
> +      vec = nir_vec2(&b, loads[0], loads[1]);
> +      break;
> +   case 3:
> +      vec = nir_vec3(&b, loads[0], loads[1], loads[2]);
> +      break;
> +   case 4:
> +      vec = nir_vec4(&b, loads[0], loads[1], loads[2], loads[3]);
> +      break;
> +   default:
> +      fprintf(stderr, "Unknown load_const component count: %d\n",
> +              lower->def.num_components);
> +      abort();

I'd use unreachable() here.

Other than that, it looks good to me. I'll have to see if it's useful for i965.

Reviewed-by: Matt Turner <mattst88 at gmail.com>