[Mesa-dev] [RFC 3/4] nir: support indirect indexing samplers in struct arrays
Jason Ekstrand
jason at jlekstrand.net
Sun Aug 30 21:21:35 PDT 2015
On Aug 30, 2015 3:44 AM, "Timothy Arceri" <t_arceri at yahoo.com.au> 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
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150830/f518b981/attachment-0001.html>
More information about the mesa-dev
mailing list