On Tue, Jan 6, 2015 at 5:05 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>
>
> On Sun, Jan 4, 2015 at 7:52 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
>
>>
>>
>> On Tue, Dec 16, 2014 at 1:11 AM, Jason Ekstrand <ja...@jlekstrand.net>
>> wrote:
>>
>>> ---
>>> src/glsl/Makefile.sources | 1 +
>>> src/glsl/nir/nir.h | 2 +
>>> src/glsl/nir/nir_lower_locals_to_regs.c | 313
>>> ++++++++++++++++++++++++++++++++
>>> 3 files changed, 316 insertions(+)
>>> create mode 100644 src/glsl/nir/nir_lower_locals_to_regs.c
>>>
>>> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
>>> index 1d3b049..6230f49 100644
>>> --- a/src/glsl/Makefile.sources
>>> +++ b/src/glsl/Makefile.sources
>>> @@ -22,6 +22,7 @@ NIR_FILES = \
>>> $(GLSL_SRCDIR)/nir/nir_intrinsics.h \
>>> $(GLSL_SRCDIR)/nir/nir_live_variables.c \
>>> $(GLSL_SRCDIR)/nir/nir_lower_atomics.c \
>>> + $(GLSL_SRCDIR)/nir/nir_lower_locals_to_regs.c \
>>> $(GLSL_SRCDIR)/nir/nir_lower_samplers.cpp \
>>> $(GLSL_SRCDIR)/nir/nir_lower_system_values.c \
>>> $(GLSL_SRCDIR)/nir/nir_lower_variables.c \
>>> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>>> index b3abfb9..7d7aec7 100644
>>> --- a/src/glsl/nir/nir.h
>>> +++ b/src/glsl/nir/nir.h
>>> @@ -1358,6 +1358,8 @@ void nir_dump_cfg(nir_shader *shader, FILE *fp);
>>>
>>> void nir_split_var_copies(nir_shader *shader);
>>>
>>> +void nir_lower_locals_to_regs(nir_shader *shader);
>>> +
>>> void nir_lower_variables(nir_shader *shader);
>>>
>>> void nir_lower_variables_scalar(nir_shader *shader, bool lower_globals,
>>> diff --git a/src/glsl/nir/nir_lower_locals_to_regs.c
>>> b/src/glsl/nir/nir_lower_locals_to_regs.c
>>> new file mode 100644
>>> index 0000000..caf1c29
>>> --- /dev/null
>>> +++ b/src/glsl/nir/nir_lower_locals_to_regs.c
>>> @@ -0,0 +1,313 @@
>>> +/*
>>> + * Copyright © 2014 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> next
>>> + * paragraph) shall be included in all copies or substantial portions
>>> of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>>> SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>> DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + * Authors:
>>> + * Jason Ekstrand (ja...@jlekstrand.net)
>>> + *
>>> + */
>>> +
>>> +#include "nir.h"
>>> +
>>> +struct locals_to_regs_state {
>>> + void *mem_ctx;
>>> + nir_function_impl *impl;
>>> +
>>> + /* A hash table mapping derefs to registers */
>>> + struct hash_table *regs_table;
>>> +};
>>> +
>>> +/* The following two functions implement a hash and equality check for
>>> + * variable dreferences. When the hash or equality function encounters
>>> an
>>> + * array, it ignores the offset and whether it is direct or indirect
>>> + * entirely.
>>> + */
>>> +static uint32_t
>>> +hash_deref(const void *void_deref)
>>> +{
>>> + const nir_deref *deref = void_deref;
>>> +
>>> + uint32_t hash;
>>> + if (deref->child) {
>>> + hash = hash_deref(deref->child);
>>> + } else {
>>> + hash = 2166136261ul;
>>> + }
>>> +
>>> + switch (deref->deref_type) {
>>> + case nir_deref_type_var:
>>> + hash ^= _mesa_hash_pointer(nir_deref_as_var(deref)->var);
>>> + break;
>>> + case nir_deref_type_array: {
>>> + hash ^= 268435183;
>>> + break;
>>> + }
>>> + case nir_deref_type_struct:
>>> + hash ^= nir_deref_as_struct(deref)->index;
>>> + break;
>>> + }
>>> +
>>> + return hash * 0x01000193;
>>> +}
>>>
>>
>> Same comment here about using FNV-1a instead.
>>
>>
>>> +
>>> +static bool
>>> +derefs_equal(const void *void_a, const void *void_b)
>>> +{
>>> + const nir_deref *a = void_a;
>>> + const nir_deref *b = void_b;
>>> +
>>> + if (a->deref_type != b->deref_type)
>>> + return false;
>>> +
>>> + switch (a->deref_type) {
>>> + case nir_deref_type_var:
>>> + if (nir_deref_as_var(a)->var != nir_deref_as_var(b)->var)
>>> + return false;
>>> + break;
>>>
>>
>> Again, we could split this out of the loop since it's only going to be
>> used once.
>>
>>
>>> + case nir_deref_type_array:
>>> + /* Do nothing. All array derefs are the same */
>>> + break;
>>> + case nir_deref_type_struct:
>>> + if (nir_deref_as_struct(a)->index !=
>>> nir_deref_as_struct(b)->index)
>>> + return false;
>>> + break;
>>> + default:
>>> + unreachable("Invalid dreference type");
>>> + }
>>> +
>>> + assert((a->child == NULL) == (b->child == NULL));
>>> + if (a->child)
>>> + return derefs_equal(a->child, b->child);
>>> + else
>>> + return true;
>>> +}
>>>
>>
>> Same comment about using a for loop here.
>>
>
> Hashing and loopifying are done in 150/133
>
>
>>
>>
>>> +
>>> +static nir_register *
>>> +get_reg_for_deref(nir_deref_var *deref, struct locals_to_regs_state
>>> *state)
>>> +{
>>> + uint32_t hash = hash_deref(deref);
>>> +
>>> + struct hash_entry *entry = _mesa_hash_table_search(state->regs_table,
>>> + hash, deref);
>>> + if (entry)
>>> + return entry->data;
>>> +
>>> + unsigned array_size = 1;
>>> + nir_deref *tail = &deref->deref;
>>> + while (tail->child) {
>>> + if (tail->child->deref_type == nir_deref_type_array) {
>>> + /* Multiply by the parent's type. */
>>> + if (glsl_type_is_matrix(tail->type)) {
>>> + array_size *= glsl_get_matrix_columns(tail->type);
>>> + } else {
>>> + assert(glsl_get_length(tail->type) > 0);
>>> + array_size *= glsl_get_length(tail->type);
>>> + }
>>> + }
>>> + tail = tail->child;
>>> + }
>>> +
>>> + assert(glsl_type_is_vector(tail->type) ||
>>> glsl_type_is_scalar(tail->type));
>>> +
>>> + nir_register *reg = nir_local_reg_create(state->impl);
>>> + reg->num_components = glsl_get_vector_elements(tail->type);
>>> + reg->num_array_elems = array_size > 1 ? array_size : 0;
>>> +
>>> + _mesa_hash_table_insert(state->regs_table, hash, deref, reg);
>>> +
>>> + return reg;
>>> +}
>>> +
>>> +static nir_src
>>> +get_deref_reg_src(nir_deref_var *deref, nir_instr *instr,
>>> + struct locals_to_regs_state *state)
>>> +{
>>> + nir_src src;
>>> +
>>> + src.is_ssa = false;
>>> + src.reg.reg = get_reg_for_deref(deref, state);
>>> + src.reg.base_offset = 0;
>>> + src.reg.indirect = NULL;
>>> +
>>> + nir_deref *tail = &deref->deref;
>>> + while (tail->child != NULL) {
>>> + const struct glsl_type *parent_type = tail->type;
>>> + tail = tail->child;
>>> +
>>> + if (tail->deref_type != nir_deref_type_array)
>>> + continue;
>>> +
>>> + nir_deref_array *deref_array = nir_deref_as_array(tail);
>>> +
>>> + src.reg.base_offset *= glsl_get_length(parent_type);
>>> + src.reg.base_offset += deref_array->base_offset;
>>> +
>>> + if (src.reg.indirect) {
>>>
>>
>> It doesn't matter too much, but since it's so easy I'd put "&&
>> glsl_get_length(parent_type) != 1" here to save some CPU cycles.
>>
>
>>
>>> + nir_load_const_instr *load_const =
>>> + nir_load_const_instr_create(state->mem_ctx);
>>> + load_const->num_components = 1;
>>> + load_const->value.u[0] = glsl_get_length(parent_type);
>>> + load_const->dest.is_ssa = true;
>>> + nir_ssa_def_init(&load_const->instr, &load_const->dest.ssa, 1,
>>> NULL);
>>> + nir_instr_insert_before(instr, &load_const->instr);
>>> +
>>> + nir_alu_instr *mul = nir_alu_instr_create(state->mem_ctx,
>>> nir_op_imul);
>>> + mul->src[0].src = *src.reg.indirect;
>>> + mul->src[1].src.is_ssa = true;
>>> + mul->src[1].src.ssa = &load_const->dest.ssa;
>>> + mul->dest.write_mask = 1;
>>> + mul->dest.dest.is_ssa = true;
>>> + nir_ssa_def_init(&mul->instr, &mul->dest.dest.ssa, 1, NULL);
>>> + nir_instr_insert_before(instr, &mul->instr);
>>> +
>>> + src.reg.indirect->is_ssa = true;
>>> + src.reg.indirect->ssa = &mul->dest.dest.ssa;
>>> + }
>>
>> +
>>> + if (deref_array->deref_array_type ==
>>> nir_deref_array_type_indirect) {
>>> + if (src.reg.indirect == NULL) {
>>> + src.reg.indirect = ralloc(state->mem_ctx, nir_src);
>>> + *src.reg.indirect = nir_src_copy(deref_array->indirect,
>>> + state->mem_ctx);
>>> + } else {
>>> + nir_alu_instr *add = nir_alu_instr_create(state->mem_ctx,
>>> + nir_op_iadd);
>>> + add->src[0].src = *src.reg.indirect;
>>> + add->src[1].src = nir_src_copy(deref_array->indirect,
>>> + state->mem_ctx);
>>> + add->dest.write_mask = 1;
>>> + add->dest.dest.is_ssa = true;
>>> + nir_ssa_def_init(&add->instr, &add->dest.dest.ssa, 1, NULL);
>>> + nir_instr_insert_before(instr, &add->instr);
>>> +
>>> + src.reg.indirect->is_ssa = true;
>>> + src.reg.indirect->ssa = &add->dest.dest.ssa;
>>> + }
>>> + }
>>> + }
>>> +
>>> + return src;
>>> +}
>>> +
>>> +static bool
>>> +lower_locals_to_regs_block(nir_block *block, void *void_state)
>>> +{
>>> + struct locals_to_regs_state *state = void_state;
>>> +
>>> + nir_foreach_instr_safe(block, instr) {
>>> + if (instr->type != nir_instr_type_intrinsic)
>>> + continue;
>>> +
>>> + nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
>>> +
>>> + switch (intrin->intrinsic) {
>>> + case nir_intrinsic_load_var_vec1:
>>> + case nir_intrinsic_load_var_vec2:
>>> + case nir_intrinsic_load_var_vec3:
>>> + case nir_intrinsic_load_var_vec4: {
>>> + if (intrin->variables[0]->var->data.mode != nir_var_local)
>>> + continue;
>>> +
>>> + nir_alu_instr *mov = nir_alu_instr_create(state->mem_ctx,
>>> nir_op_imov);
>>> + mov->src[0].src = get_deref_reg_src(intrin->variables[0],
>>> + &intrin->instr, state);
>>> + unsigned num_components =
>>> mov->src[0].src.reg.reg->num_components;
>>> + mov->dest.write_mask = (1 << num_components) - 1;
>>> + if (intrin->dest.is_ssa) {
>>> + mov->dest.dest.is_ssa = true;
>>> + nir_ssa_def_init(&mov->instr, &mov->dest.dest.ssa,
>>> + num_components, NULL);
>>> +
>>> + nir_src new_src = {
>>> + .is_ssa = true,
>>> + .ssa = &mov->dest.dest.ssa,
>>> + };
>>> +
>>> + nir_ssa_def_rewrite_uses(&intrin->dest.ssa, new_src,
>>> + state->mem_ctx);
>>> + } else {
>>> + mov->dest.dest = nir_dest_copy(intrin->dest,
>>> state->mem_ctx);
>>> + }
>>> + nir_instr_insert_before(&intrin->instr, &mov->instr);
>>> +
>>> + nir_instr_remove(&intrin->instr);
>>> + break;
>>> + }
>>> +
>>> + case nir_intrinsic_store_var_vec1:
>>> + case nir_intrinsic_store_var_vec2:
>>> + case nir_intrinsic_store_var_vec3:
>>> + case nir_intrinsic_store_var_vec4: {
>>> + if (intrin->variables[0]->var->data.mode != nir_var_local)
>>> + continue;
>>> +
>>> + nir_src reg_src = get_deref_reg_src(intrin->variables[0],
>>> + &intrin->instr, state);
>>> + unsigned num_components = reg_src.reg.reg->num_components;
>>> +
>>> + nir_alu_instr *mov = nir_alu_instr_create(state->mem_ctx,
>>> nir_op_imov);
>>> + mov->src[0].src = nir_src_copy(intrin->src[0], state->mem_ctx);
>>> + mov->dest.write_mask = (1 << num_components) - 1;
>>> + mov->dest.dest.is_ssa = false;
>>> + mov->dest.dest.reg.reg = reg_src.reg.reg;
>>> + mov->dest.dest.reg.base_offset = reg_src.reg.base_offset;
>>> + mov->dest.dest.reg.indirect = reg_src.reg.indirect;
>>> +
>>> + nir_instr_insert_before(&intrin->instr, &mov->instr);
>>> +
>>> + nir_instr_remove(&intrin->instr);
>>> + break;
>>> + }
>>> +
>>> + case nir_intrinsic_copy_var:
>>> + unreachable("There should be no copies whatsoever at this
>>> point");
>>> + break;
>>>
>>
>> Are you sure about this? My impression is that lower_variables will lower
>> copies involving things that aren't indirectly referenced, but if you have
>> something like:
>>
>> foo[i] = ...
>> bar[*] = foo[*];
>> ... = bar[i];
>>
>> then the copy in the middle won't get lowered, unless there's something
>> else I'm missing that will lower it.
>>
>
> Yeah, there may be something missing there. I have a pass lying around
> somewhere that lowers all copies. Unfortunately, I've never actually seen
> this happen in the wild so It's untested. I'll try and cook something up
> that I think is reliable.
>
Ok, more info. Right now, GLSL IR is lowering all truely indirect accesses
to if-ladders right now so we can never hit this. Once we can handle
indirects in the backends or generate the if-ladders in NIR, we will need
this. Until then, let's leave it as-is to reduce the ammount of untested
code.
--Jason
>
>
>> If we always lowered these copies (ignoring that it hurts packing for
>> vec4 backends), then we wouldn't need wildcards in the first place...
>>
>
> Yes, but I have an evil plot to do copy propagation of variables that
> properly handles arrays. >:-]
>
>
>
>>
>>
>>> +
>>> + default:
>>> + continue;
>>> + }
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static void
>>> +nir_lower_locals_to_regs_impl(nir_function_impl *impl)
>>> +{
>>> + struct locals_to_regs_state state;
>>> +
>>> + state.mem_ctx = ralloc_parent(impl);
>>> + state.impl = impl;
>>> + state.regs_table = _mesa_hash_table_create(NULL, derefs_equal);
>>> +
>>> + nir_foreach_block(impl, lower_locals_to_regs_block, &state);
>>> +
>>> + _mesa_hash_table_destroy(state.regs_table, NULL);
>>> +}
>>> +
>>> +void
>>> +nir_lower_locals_to_regs(nir_shader *shader)
>>> +{
>>> + nir_foreach_overload(shader, overload) {
>>> + if (overload->impl)
>>> + nir_lower_locals_to_regs_impl(overload->impl);
>>> + }
>>> +}
>>> --
>>> 2.2.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
