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

[Connor Abbott]

On Wed, Jun 17, 2015 at 12:02 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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)

Right... we explicitly decided not to support SSA form for arrays in
NIR, since it seemed like a pretty bad idea. SSA form assumes that
inserting copies for the things you're SSA-ifying is relatively
inexpensive, and both SSA-based register allocation and algorithms for
converting out of SSA make no guarantees about not inserting
potentially unnecessary copies. This is a good compromise for smaller
things, but not for your array of (say) 64 things where inserting an
extra copy is rather disastrous. You can do it if you want and shoot
yourself in the foot, but NIR isn't going to help you there -- we
won't write a to-SSA pass for something which doesn't make much sense
in the first place.

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

is_packed? what does that have to do with it? I suspect that there
might be some places where you're using this function instead of the
others I mentioned, and you actually want to use the latter, although
I haven't seen the code so I can't be sure. But of course, there's
always the option of actually going and fixing it :)

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