On Wed, Jun 17, 2015 at 2:27 PM, Connor Abbott <cwabbo...@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 <robdcl...@gmail.com> wrote:
>> From: Rob Clark <robcl...@freedesktop.org>
>>
>> I need something like this in a couple places. And didn't see anything
>> like it anywhere.
>>
>> Signed-off-by: Rob Clark <robcl...@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
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
