[Mesa-dev] [PATCH] nir: add helper to get # of src/dest components

[Connor Abbott]

So, as is, this patch isn't quite correct. When I originally wrote
NIR, the idea was that the size of each instruction would be explicit
-- that is, each instruction has it's own size, and the size of
registers/SSA values was merely a hint to say "by the way, you
actually need this many components based on all the things that use
this value/register." In other words, you could smash num_components
to 4 for everything, and it would still work. Then, we could just
shrink num_components on demand as we got rid of uses of things.
That's why we have functions like nir_tex_instr_src_size(),
nir_tex_instr_dest_size(), and nir_ssa_alu_instr_src_components() that
seem like they're doing the same thing that these functions -- in most
cases, you actually want those functions instead of these. Maybe we
were figuring out the register/value # of components a few times, and
perhaps some of times it was broken, but it seems like adding these
functions would only add to the confusion.

Now, in hindsight, it seems like that might not be the best idea. In
other words, I can see how it would make sense to turn
nir_tex_instr_src_size() etc. into assertions in the validator that
check that nir_(src|dest)_num_components() equals what you would
expect, and it's probably a good idea. But I don't want people to use
these functions without knowing what they're doing until we do that
cleanup.

On Mon, Jun 8, 2015 at 12:45 PM, Rob Clark <robdclark at gmail.com> wrote:
> From: Rob Clark <robclark at freedesktop.org>
>
> I need something like this in a couple places.  And didn't see anything
> like it anywhere.
>
> Signed-off-by: Rob Clark <robclark at freedesktop.org>
> ---
> v2: Added similar helper for nir_src, and cleaned up a few places that
> open coded this.  There are a couple left (such as validate_alu_src())
> but that handle is_packed differently so I thought it best to leave
> them as-is.
>
>  src/glsl/nir/nir.h          | 18 ++++++++++++++++++
>  src/glsl/nir/nir_from_ssa.c | 10 ++--------
>  src/glsl/nir/nir_validate.c |  4 +---
>  3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
> index 697d37e..06bbb0c 100644
> --- a/src/glsl/nir/nir.h
> +++ b/src/glsl/nir/nir.h
> @@ -541,6 +541,24 @@ typedef struct {
>  #define nir_foreach_def_safe(reg, dest) \
>     list_for_each_entry_safe(nir_dest, dest, &(reg)->defs, reg.def_link)
>
> +static inline unsigned
> +nir_dest_num_components(nir_dest *dest)
> +{
> +   if (dest->is_ssa)
> +      return dest->ssa.num_components;
> +   else
> +      return dest->reg.reg->num_components;
> +}
> +
> +static inline unsigned
> +nir_src_num_components(nir_src *src)
> +{
> +   if (src->is_ssa)
> +      return src->ssa->num_components;
> +   else
> +      return src->reg.reg->num_components;
> +}
> +
>  static inline nir_src
>  nir_src_for_ssa(nir_ssa_def *def)
>  {
> diff --git a/src/glsl/nir/nir_from_ssa.c b/src/glsl/nir/nir_from_ssa.c
> index 67733e6..23c798d 100644
> --- a/src/glsl/nir/nir_from_ssa.c
> +++ b/src/glsl/nir/nir_from_ssa.c
> @@ -553,10 +553,7 @@ emit_copy(nir_parallel_copy_instr *pcopy, nir_src src, nir_src dest_src,
>            dest_src.reg.indirect == NULL &&
>            dest_src.reg.base_offset == 0);
>
> -   if (src.is_ssa)
> -      assert(src.ssa->num_components >= dest_src.reg.reg->num_components);
> -   else
> -      assert(src.reg.reg->num_components >= dest_src.reg.reg->num_components);
> +   assert(nir_src_num_components(&src) == nir_src_num_components(&dest_src));
>
>     nir_alu_instr *mov = nir_alu_instr_create(mem_ctx, nir_op_imov);
>     nir_src_copy(&mov->src[0].src, &src, mem_ctx);
> @@ -712,10 +709,7 @@ resolve_parallel_copy(nir_parallel_copy_instr *pcopy,
>        nir_register *reg = nir_local_reg_create(state->impl);
>        reg->name = "copy_temp";
>        reg->num_array_elems = 0;
> -      if (values[b].is_ssa)
> -         reg->num_components = values[b].ssa->num_components;
> -      else
> -         reg->num_components = values[b].reg.reg->num_components;
> +      reg->num_components = nir_src_num_components(&values[b]);
>        values[num_vals].is_ssa = false;
>        values[num_vals].reg.reg = reg;
>
> diff --git a/src/glsl/nir/nir_validate.c b/src/glsl/nir/nir_validate.c
> index da92ed9..c781912 100644
> --- a/src/glsl/nir/nir_validate.c
> +++ b/src/glsl/nir/nir_validate.c
> @@ -262,9 +262,7 @@ validate_dest(nir_dest *dest, validate_state *state)
>  static void
>  validate_alu_dest(nir_alu_dest *dest, validate_state *state)
>  {
> -   unsigned dest_size =
> -      dest->dest.is_ssa ? dest->dest.ssa.num_components
> -                        : dest->dest.reg.reg->num_components;
> +   unsigned dest_size = nir_dest_num_components(&dest->dest);
>     bool is_packed = !dest->dest.is_ssa && dest->dest.reg.reg->is_packed;
>     /*
>      * validate that the instruction doesn't write to components not in the
> --
> 2.4.2
>