[Mesa-dev] [RFC 2/2] gallium/ttn: add support for temp arrays
Rob Clark
robdclark at gmail.com
Tue Apr 7 09:43:29 PDT 2015
On Tue, Apr 7, 2015 at 12:32 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Tue, Apr 7, 2015 at 8:52 AM, Rob Clark <robdclark at gmail.com> wrote:
>> From: Rob Clark <robclark at freedesktop.org>
>>
>> Since the rest of NIR really would rather have these as variables rather
>> than registers, create a nir_variable per array. But rather than
>> completely re-arrange ttn to be variable based rather than register
>> based, keep the registers. In the cases where there is a matching var
>> for the reg, ttn_emit_instruction will append the appropriate intrinsic
>> to get things back from the shadow reg into the variable.
>>
>> NOTE: this doesn't quite handle TEMP[ADDR[]] when the DCL doesn't give
>> an array id. But those just kinda suck, and should really go away.
>> AFAICT we don't get those from glsl. Might be an issue for some other
>> state tracker.
>>
>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>> ---
>> src/gallium/auxiliary/nir/tgsi_to_nir.c | 116 ++++++++++++++++++++++++++------
>> 1 file changed, 94 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> index da935a4..1c7b313 100644
>> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> @@ -44,6 +44,7 @@
>> struct ttn_reg_info {
>> /** nir register containing this TGSI index. */
>> nir_register *reg;
>> + nir_variable *var;
>> /** Offset (in vec4s) from the start of var for this TGSI index. */
>> int offset;
>> };
>> @@ -121,22 +122,29 @@ ttn_emit_declaration(struct ttn_compile *c)
>>
>> if (file == TGSI_FILE_TEMPORARY) {
>> nir_register *reg;
>> - if (c->scan->indirect_files & (1 << file)) {
>> + for (i = 0; i < array_size; i++) {
>> reg = nir_local_reg_create(b->impl);
>> reg->num_components = 4;
>> - reg->num_array_elems = array_size;
>> + c->temp_regs[decl->Range.First + i].reg = reg;
>> + c->temp_regs[decl->Range.First + i].offset = 0;
>> + }
>> + if (decl->Declaration.Array) {
>> + /* for arrays, the register created just serves as a
>> + * shadow register. We append intrinsic_store_global
>> + * after the tgsi instruction is translated to move
>> + * back from the shadow register to the variable
>> + */
>> + nir_variable *var = rzalloc(b->shader, nir_variable);
>> + var->type = glsl_array_type(glsl_vec4_type(), array_size);
>> + var->data.mode = nir_var_global;
>> + var->name = ralloc_asprintf(var, "arr_%d", decl->Array.ArrayID);
>> +
>> + exec_list_push_tail(&b->shader->globals, &var->node);
>>
>> for (i = 0; i < array_size; i++) {
>> - c->temp_regs[decl->Range.First + i].reg = reg;
>> - c->temp_regs[decl->Range.First + i].offset = i;
>> + c->temp_regs[decl->Range.First + i].var = var;
>> }
>> } else {
>> - for (i = 0; i < array_size; i++) {
>> - reg = nir_local_reg_create(b->impl);
>> - reg->num_components = 4;
>> - c->temp_regs[decl->Range.First + i].reg = reg;
>> - c->temp_regs[decl->Range.First + i].offset = 0;
>> - }
>> }
>> } else if (file == TGSI_FILE_ADDRESS) {
>> c->addr_reg = nir_local_reg_create(b->impl);
>> @@ -256,10 +264,34 @@ ttn_src_for_file_and_index(struct ttn_compile *c, unsigned file, unsigned index,
>>
>> switch (file) {
>> case TGSI_FILE_TEMPORARY:
>> - src.reg.reg = c->temp_regs[index].reg;
>> - src.reg.base_offset = c->temp_regs[index].offset;
>> - if (indirect)
>> - src.reg.indirect = ttn_src_for_indirect(c, indirect);
>> + if (c->temp_regs[index].var) {
>> + nir_intrinsic_instr *load;
>> + nir_alu_src indirect_address;
>> +
>> + assert(indirect);
>> +
>> + load = nir_intrinsic_instr_create(b->shader,
>> + nir_intrinsic_load_global_indirect);
>> + load->num_components = 4;
>> + load->const_index[0] = index;
>> + load->const_index[1] = 1;
>
> Why are we using an intrinsic that has an index and not
> nir_intrinsic_load_var with a deref? A short (2-element) deref chain
> will handle this for you and then the lower_vars_to_ssa pass will pick
> up on things like if all the indirect uses are actually constant and
> lower it to SSA values for you. If you use an index+offset intrinsic
> then it's completely opaque and the rest of NIR doesn't know what to
> do with it.
I am *assuming* here that the index refers to which var you are
load/storing.. at least that is how it seemed to work for
uniforms/inputs/outputs. Ofc I'm mostly just trying to infer about
how things should work from reading code so entirely possible I'm
missing something or haven't read the right parts of the code yet..
I'm starting to think more that I should have added a
nir_intrinsic_{load,store}_var_indirect instead of new intrinsics for
load/store_global(_indirect).. I guess that would fit in better with
how variables already work. Although I couldn't see any obvious way
for {load,store}_var to take an additional addr src, so I think one
way or another we need new intrinsics..
>
>> +
>> + memset(&indirect_address, 0, sizeof(indirect_address));
>> + indirect_address.src = nir_src_for_reg(c->addr_reg);
>> + for (int i = 0; i < 4; i++)
>> + indirect_address.swizzle[i] = indirect->Swizzle;
>> + load->src[0] = nir_src_for_ssa(nir_imov_alu(b, indirect_address, 1));
>> +
>> + nir_ssa_dest_init(&load->instr, &load->dest, 4, NULL);
>> + nir_instr_insert_after_cf_list(b->cf_node_list, &load->instr);
>> +
>> + src = nir_src_for_ssa(&load->dest.ssa);
>> +
>> + } else {
>> + assert(!indirect);
>> + src.reg.reg = c->temp_regs[index].reg;
>> + src.reg.base_offset = c->temp_regs[index].offset;
>> + }
>> break;
>>
>> case TGSI_FILE_ADDRESS:
>> @@ -340,29 +372,45 @@ ttn_get_dest(struct ttn_compile *c, struct tgsi_full_dst_register *tgsi_fdst)
>> {
>> struct tgsi_dst_register *tgsi_dst = &tgsi_fdst->Register;
>> nir_alu_dest dest;
>> + unsigned index = tgsi_dst->Index;
>>
>> memset(&dest, 0, sizeof(dest));
>>
>> + dest.write_mask = tgsi_dst->WriteMask;
>> + dest.saturate = false;
>> +
>> if (tgsi_dst->File == TGSI_FILE_TEMPORARY) {
>> - dest.dest.reg.reg = c->temp_regs[tgsi_dst->Index].reg;
>> - dest.dest.reg.base_offset = c->temp_regs[tgsi_dst->Index].offset;
>> + dest.dest.reg.reg = c->temp_regs[index].reg;
>> + dest.dest.reg.base_offset = c->temp_regs[index].offset;
>> } else if (tgsi_dst->File == TGSI_FILE_OUTPUT) {
>> - dest.dest.reg.reg = c->output_regs[tgsi_dst->Index].reg;
>> - dest.dest.reg.base_offset = c->output_regs[tgsi_dst->Index].offset;
>> + dest.dest.reg.reg = c->output_regs[index].reg;
>> + dest.dest.reg.base_offset = c->output_regs[index].offset;
>> } else if (tgsi_dst->File == TGSI_FILE_ADDRESS) {
>> - assert(tgsi_dst->Index == 0);
>> + assert(index == 0);
>> dest.dest.reg.reg = c->addr_reg;
>> }
>>
>> - dest.write_mask = tgsi_dst->WriteMask;
>> - dest.saturate = false;
>> -
>> if (tgsi_dst->Indirect)
>> dest.dest.reg.indirect = ttn_src_for_indirect(c, &tgsi_fdst->Indirect);
>>
>> return dest;
>> }
>>
>> +static nir_variable *
>> +ttn_get_var(struct ttn_compile *c, struct tgsi_full_dst_register *tgsi_fdst)
>> +{
>> + struct tgsi_dst_register *tgsi_dst = &tgsi_fdst->Register;
>> + unsigned index = tgsi_dst->Index;
>> +
>> + if (tgsi_dst->File == TGSI_FILE_TEMPORARY) {
>> + return c->temp_regs[index].var;
>> + } else if (tgsi_dst->File == TGSI_FILE_OUTPUT) {
>> + return c->output_regs[index].var;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static nir_ssa_def *
>> ttn_get_src(struct ttn_compile *c, struct tgsi_full_src_register *tgsi_fsrc)
>> {
>> @@ -1331,6 +1379,30 @@ ttn_emit_instruction(struct ttn_compile *c)
>> assert(!dest.dest.is_ssa);
>> ttn_move_dest(b, dest, nir_fsat(b, ttn_src_for_dest(b, &dest)));
>> }
>> +
>> + /* if the dst has a matching var, append store_global to move
>> + * output from reg to var
>> + */
>> + nir_variable *var = ttn_get_var(c, &tgsi_inst->Dst[0]);
>> + if (var) {
>
> As far as I can see, the only place you ever use the variables you
> create is here to check if you have one. Why are you creating
> variables at all if you're going to use an index+offset intrinsic
> every time you load/store from/to them?
It is referring to the var via:
store->const_index[0] = tgsi_inst->Dst[0].Register.Index;
ofc, based on your last comment, that may not be the correct way to
refer to a var..
BR,
-R
> --Jason
>
>> + nir_intrinsic_op op = tgsi_inst->Dst[0].Register.Indirect ?
>> + nir_intrinsic_store_global_indirect :
>> + nir_intrinsic_store_global;
>> + nir_intrinsic_instr *store =
>> + nir_intrinsic_instr_create(b->shader, op);
>> +
>> + store->num_components = 4;
>> + store->const_index[0] = tgsi_inst->Dst[0].Register.Index;
>> + store->const_index[1] = 1;
>> + if (tgsi_inst->Dst[0].Register.Indirect) {
>> + store->src[0] = nir_src_for_reg(c->addr_reg);
>> + store->src[1] = nir_src_for_reg(dest.dest.reg.reg);
>> + } else {
>> + store->src[0] = nir_src_for_reg(dest.dest.reg.reg);
>> + }
>> +
>> + nir_instr_insert_after_cf_list(b->cf_node_list, &store->instr);
>> + }
>> }
>>
>> /**
>> --
>> 2.1.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list