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

[Rob Clark]

On Thu, Jun 18, 2015 at 1:23 AM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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.
>

in ir3 my solution is to add sufficient dependencies between
instructions so the array accesses don't get re-ordered and they all
collapse down to a single name per array element/slot

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

because I was in a hurry and didn't actually read your original reply
(and assumed it was continuation of discussion about nuking is_packed
from irc)

So the place where I was using it is actually for phi instructions
(and actually just for an assert at the moment), so those fxns you
mention don't do me much good.

Somehow, I think it is still a good idea to un-open-code the figuring
out of # of src/dst components.  Sure it makes sense to have validate
code ensure that those are same as nir_tex_instr_src_size(),
nir_tex_instr_dest_size(), and nir_ssa_alu_instr_src_components()
give.  But that doesn't preclude adding these two functions.

BR,
-R

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