On Aug 30, 2015 3:44 AM, "Timothy Arceri" <<a href="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</a>> wrote: > > As a bonus we get indirect support for arrays of arrays for free. > --- > I've marked this as request for comment as although it works enough for to > pass all the piglit tests there are issues as I'm still trying to figure out > nir. > > The issues are around the second call to nir_instr_move_src() this call > expects the use_link list not to be empty just so it can then remove all > entries so I add the list_addtail() call above it to work around this. > While this is enough to pass the piglit tests on a non debug build it fails > validation for debug builds. > > I'm clearly missing a step and would be greatful to anyone who can point me > in the right direction. > > The validation error: > nir/nir_validate.c:873: postvalidate_ssa_def: Assertion `entry' failed. > > src/glsl/nir/nir_lower_samplers.cpp | 169 ++++++++++++++++++++---------------- > 1 file changed, 95 insertions(+), 74 deletions(-) > > diff --git a/src/glsl/nir/nir_lower_samplers.cpp b/src/glsl/nir/nir_lower_samplers.cpp > index 9583b45..dbe9e95 100644 > --- a/src/glsl/nir/nir_lower_samplers.cpp > +++ b/src/glsl/nir/nir_lower_samplers.cpp > @@ -24,6 +24,7 @@ > */ > > #include "nir.h" > +#include "nir_builder.h" Thank you for using the builder! > #include "../program.h" > #include "program/hash_table.h" > #include "ir_uniform.h" > @@ -35,89 +36,49 @@ extern "C" { > #include "program/program.h" > } > > -static unsigned > -get_sampler_index(const struct gl_shader_program *shader_program, > - gl_shader_stage stage, const char *name) > -{ > - unsigned location; > - if (!shader_program->UniformHash->get(location, name)) { > - assert(!"failed to find sampler"); > - return 0; > - } > - > - if (!shader_program->UniformStorage[location].sampler[stage].active) { > - assert(!"cannot return a sampler"); > - return 0; > - } > - > - return shader_program->UniformStorage[location].sampler[stage].index; > -} > - > +/* Calculate the sampler index based on array indicies and also > + * calculate the base uniform location for struct members. > + */ > static void > -lower_sampler(nir_tex_instr *instr, const struct gl_shader_program *shader_program, > - gl_shader_stage stage, void *mem_ctx) > +calc_sampler_offsets(nir_deref *tail, nir_tex_instr *instr, > + unsigned *array_elements, nir_src *indirect, What we have done in most other builder-like things in NIR is to pass around a nir_ssa_def pointer instead of a nir_src. This keeps things less ambiguous because you always know exactly what kind of thing you're working with and you don't have the "should I copy this or move it or use rewrite or what?" issues that you have with a nir_src. Also, I'd be kind of inclined to make an "offset" struct containing the base, the indirect SSA value, and the size. Then use that for the return vale to keep the recursion cleaner. Tracking by-reverence in/out arguments through a recursive algorithm is a pain. > + nir_builder *b, unsigned *location, > + bool *found_indirect) > { > - if (instr->sampler == NULL) > - return; > - > - /* Get the name and the offset */ > - instr->sampler_index = 0; > - char *name = ralloc_strdup(mem_ctx, instr->sampler->var->name); > - > - for (nir_deref *deref = &instr->sampler->deref; > - deref->child; deref = deref->child) { > - switch (deref->child->deref_type) { > + if (tail->child != NULL) { > + switch (tail->child->deref_type) { > case nir_deref_type_array: { > - nir_deref_array *deref_array = nir_deref_as_array(deref->child); > + nir_deref_array *deref_array = nir_deref_as_array(tail->child); > > assert(deref_array->deref_array_type != nir_deref_array_type_wildcard); > > - if (deref_array->deref.child) { > - ralloc_asprintf_append(&name, "[%u]", > - deref_array->deref_array_type == nir_deref_array_type_direct ? > - deref_array->base_offset : 0); > - } else { > - assert(deref->child->type->base_type == GLSL_TYPE_SAMPLER); > - instr->sampler_index = deref_array->base_offset; > - } > + calc_sampler_offsets(tail->child, instr, array_elements, > + indirect, b, location, found_indirect); > + instr->sampler_index += deref_array->base_offset * *array_elements; > > - /* XXX: We're assuming here that the indirect is the last array > - * thing we have. This should be ok for now as we don't support > - * arrays_of_arrays yet. > - */ > if (deref_array->deref_array_type == nir_deref_array_type_indirect) { > - /* First, we have to resize the array of texture sources */ > - nir_tex_src *new_srcs = rzalloc_array(instr, nir_tex_src, > - instr->num_srcs + 1); > - > - for (unsigned i = 0; i < instr->num_srcs; i++) { > - new_srcs[i].src_type = instr->src[i].src_type; > - nir_instr_move_src(&instr->instr, &new_srcs[i].src, > - &instr->src[i].src); > + nir_ssa_def *mul = > + nir_imul(b, nir_imm_int(b, *array_elements), > + nir_ssa_for_src(b, deref_array->indirect, 1)); > + > + if (*found_indirect) { > + indirect->ssa = > + nir_iadd(b, nir_ssa_for_src(b, *indirect, 1), mul); > + } else { > + *indirect = nir_src_for_ssa(mul); > + *found_indirect = true; > } > - > - ralloc_free(instr->src); > - instr->src = new_srcs; > - > - /* Now we can go ahead and move the source over to being a > - * first-class texture source. > - */ > - instr->src[instr->num_srcs].src_type = nir_tex_src_sampler_offset; > - instr->num_srcs++; > - nir_instr_move_src(&instr->instr, > - &instr->src[instr->num_srcs - 1].src, > - &deref_array->indirect); > - > - instr->sampler_array_size = glsl_get_length(deref->type); > } > - break; > + > + *array_elements *= glsl_get_length(tail->type); > + break; > } > > case nir_deref_type_struct: { > - nir_deref_struct *deref_struct = nir_deref_as_struct(deref->child); > - const char *field = glsl_get_struct_elem_name(deref->type, > - deref_struct->index); > - ralloc_asprintf_append(&name, ".%s", field); > + nir_deref_struct *deref_struct = nir_deref_as_struct(tail->child); > + *location += deref_struct->index; This doesn't seem right... What if the struct contains multiple arrays of samplers? What happens if the struct contains 4 floats and then a sampler? This line seems to imply that each element in the structure is a single sampler. > + calc_sampler_offsets(tail->child, instr, array_elements, > + indirect, b, location, found_indirect); > break; > } > > @@ -125,15 +86,75 @@ lower_sampler(nir_tex_instr *instr, const struct gl_shader_program *shader_progr > unreachable("Invalid deref type"); > break; > } > + } else { > + return; > + } > +} > + > +static void > +lower_sampler(nir_tex_instr *instr, const struct gl_shader_program *shader_program, > + gl_shader_stage stage, nir_builder *builder) Now that the calc_sampler_offsets stuff is split out we might as well pull this into lower_block_cb. However that's probably better as a follow-on to keep review easy. While we're at it, we should probably rename lower_block_cb to lower_samplers_block. The nir_lower_samplers pass is old and predates the rent naming conventions. OK, tangent over. > +{ > + if (instr->sampler == NULL) > + return; > + > + instr->sampler_index = 0; > + unsigned location = instr->sampler->var->data.location; > + > + if (instr->sampler->deref.child) { > + unsigned array_elements = 1; > + nir_src indirect; > + bool found_indirect = false; > + > + builder->cursor = nir_before_instr(&instr->instr); > + calc_sampler_offsets(&instr->sampler->deref, instr, > + &array_elements, &indirect, builder, > + &location, &found_indirect); > + > + if (found_indirect) { > + /* First, we have to resize the array of texture sources */ > + nir_tex_src *new_srcs = rzalloc_array(instr, nir_tex_src, > + instr->num_srcs + 1); > + > + for (unsigned i = 0; i < instr->num_srcs; i++) { > + new_srcs[i].src_type = instr->src[i].src_type; > + nir_instr_move_src(&instr->instr, &new_srcs[i].src, > + &instr->src[i].src); > + } > + > + ralloc_free(instr->src); > + instr->src = new_srcs; > + > + /* Now we can go ahead and move the source over to being a > + * first-class texture source. > + */ > + nir_src indirect_nir_src = nir_src_for_ssa(indirect.ssa); > + list_addtail(&indirect_nir_src.use_link, &indirect_nir_src.ssa->uses); > + > + instr->src[instr->num_srcs].src_type = nir_tex_src_sampler_offset; > + instr->num_srcs++; > + nir_instr_move_src(&instr->instr, > + &instr->src[instr->num_srcs - 1].src, > + &indirect_nir_src); > + > + instr->sampler_array_size = array_elements; > + } > + } > + > + if (location > shader_program->NumUniformStorage - 1 || > + !shader_program->UniformStorage[location].sampler[stage].active) { > + assert(!"cannot return a sampler"); > + return; > } > > - instr->sampler_index += get_sampler_index(shader_program, stage, name); > + instr->sampler_index += > + shader_program->UniformStorage[location].sampler[stage].index; > > instr->sampler = NULL; > } > > typedef struct { > - void *mem_ctx; > + nir_builder builder; > const struct gl_shader_program *shader_program; > gl_shader_stage stage; > } lower_state; > @@ -147,7 +168,7 @@ lower_block_cb(nir_block *block, void *_state) > if (instr->type == nir_instr_type_tex) { > nir_tex_instr *tex_instr = nir_instr_as_tex(instr); > lower_sampler(tex_instr, state->shader_program, state->stage, > - state->mem_ctx); > + &state->builder); > } > } > > @@ -160,7 +181,7 @@ lower_impl(nir_function_impl *impl, const struct gl_shader_program *shader_progr > { > lower_state state; > > - state.mem_ctx = ralloc_parent(impl); > + nir_builder_init(&state.builder, impl); > state.shader_program = shader_program; > state.stage = stage; > > -- > 2.4.3 > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>