On Mon, Aug 3, 2015 at 5:44 PM, Eric Anholt <e...@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 <matts...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
