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

[Rob Clark]

On Wed, Jun 17, 2015 at 2:27 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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.

hmm, fortunately I hadn't pushed my loops branch yet, since still need
to work out how to make variables/arrays work w/ >1 block (since in
nir-land variables are not ssa)..

(really I want phi's for variables too..  the way I turn arrays into
fanin/collect fanout/split works on the backend for dealing with
arrays in ssa form (other than making instruction graph large) but the
way I go from nir to ir3 currently assumes I get nir phi's everywhere
I need an ir3 phi)

Anyways, maybe I'll just move the helpers into ir3 for now until the
is_packed stuff is purged..

BR,
-R


>
> 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
>>