On Fri, 2015-09-11 at 12:55 +1000, Timothy Arceri wrote:
> On Thu, 2015-09-10 at 18:04 -0700, Jason Ekstrand wrote:
> > On Tue, Sep 1, 2015 at 7:44 PM, Timothy Arceri <
> > t_arc...@yahoo.com.au
> > > wrote:
> > > As a bonus we get indirect support for arrays of arrays for free.
> > >
> > > V3: Use nir_instr_rewrite_src() with empty src rather then
> > > clearing
> > > the use_link list directly for the old indirects as suggested by
> > > Jason
> > >
> > > V2: Fixed validation error in debug build
> > > ---
> > > 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..9736e0f 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"
> > > #include "../program.h"
> > > #include "program/hash_table.h"
> > > #include "ir_uniform.h"
> > > @@ -35,89 +36,52 @@ 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,
> > > + 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));
> > > +
> > > + nir_instr_rewrite_src(&instr->instr, &deref_array
> > > ->indirect,
> > > + NIR_SRC_INIT);
> > > +
> > > + 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;
> >
> > I finally got around to looking at this patch again and most of it
> > seems fine except for this line. I don't think you can just
> > increment
> > location by deref_struct->index. I think you need to add up all of
> > the array sizes of all of the previous members. (Yeah, that's
> > gross
> > but I'm not thinking of anything better off-hand).
>
> I think I might have to do this for AoA but single dimension arrays
> are
> all stored at the same location so I think this is ok as is for now,
> unless I'm missing something?
Actually it looks like this is whats causing most of the regressions in
the deqp test. I need to add up all the member elements of previous
nested structs.
>
>
> >
> > > + calc_sampler_offsets(tail->child, instr,
> > > array_elements,
> > > + indirect, b, location,
> > > found_indirect);
> > > break;
> > > }
> > >
> > > @@ -125,15 +89,72 @@ 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)
> > > +{
> > > + 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;
> >
> > I may have made this comment before, but wouldn't it be easier to
> > just
> > have "indirect" be a nir_ssa_def pointer? Then you wouldn't need
> > to
> > also have a boolean (NULL == no indirect) and you could completely
> > side-step any nir_src copying issues.
>
> I think I had to code like this at one point but changed it (no idea
> why). I'll give it another go.
>
> Thanks for looking over all these :) I'm just trying to resolve some
> regressions with the deqp test suite then I'll send out a new version
> updated with your feedback.
>
>
> >
> > --Jason
> >
> > > +
> > > + 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.
> > > + */
> > > + instr->src[instr->num_srcs].src_type =
> > > nir_tex_src_sampler_offset;
> > > + instr->num_srcs++;
> > > + nir_instr_rewrite_src(&instr->instr,
> > > + &instr->src[instr->num_srcs -
> > > 1].src,
> > > + indirect);
> > > +
> > > + 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
> > > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
