On Tue, Sep 15, 2015 at 11:52 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Tue, Sep 15, 2015 at 12:51 AM, Timothy Arceri <t_arc...@yahoo.com.au>
> wrote:
>> As a bonus we get indirect support for arrays of arrays for free.
>>
>> V4: fix struct member location caclulation, use nir_ssa_def rather than
>> nir_src for the indirect as suggested by Jason
>>
>> 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
>>
>> Cc: Jason Ekstrand <jason.ekstr...@intel.com>
>> ---
>> src/glsl/nir/nir_lower_samplers.cpp | 160
>> ++++++++++++++++++++----------------
>> 1 file changed, 88 insertions(+), 72 deletions(-)
>>
>> diff --git a/src/glsl/nir/nir_lower_samplers.cpp
>> b/src/glsl/nir/nir_lower_samplers.cpp
>> index 9583b45..623a07c 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,105 +36,120 @@ 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)
>> +/* Calculate the sampler index based on array indicies and also
>> + * calculate the base uniform location for struct members.
>> + */
>> +static void
>> +calc_sampler_offsets(nir_deref *tail, nir_tex_instr *instr,
>> + unsigned *array_elements, nir_ssa_def **indirect,
>> + nir_builder *b, unsigned *location)
>> {
>> - unsigned location;
>> - if (!shader_program->UniformHash->get(location, name)) {
>> - assert(!"failed to find sampler");
>> - return 0;
>> - }
>> + if (tail->child != NULL) {
>> + switch (tail->child->deref_type) {
>> + case nir_deref_type_array: {
>> + nir_deref_array *deref_array = nir_deref_as_array(tail->child);
>>
>> - if (!shader_program->UniformStorage[location].sampler[stage].active) {
>> - assert(!"cannot return a sampler");
>> - return 0;
>> - }
>> + assert(deref_array->deref_array_type !=
>> nir_deref_array_type_wildcard);
>> +
>> + calc_sampler_offsets(tail->child, instr, array_elements,
>> + indirect, b, location);
>> + instr->sampler_index += deref_array->base_offset * *array_elements;
>> +
>> + if (deref_array->deref_array_type ==
>> nir_deref_array_type_indirect) {
>> + 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 (*indirect) {
>> + *indirect = nir_iadd(b, *indirect, mul);
>> + } else {
>> + *indirect = mul;
>> + }
>> + }
>> +
>> + *array_elements *= glsl_get_length(tail->type);
>> + break;
>> + }
>> +
>> + case nir_deref_type_struct: {
>> + nir_deref_struct *deref_struct = nir_deref_as_struct(tail->child);
>> + *location +=
>> tail->type->record_location_offset(deref_struct->index);
>> + calc_sampler_offsets(tail->child, instr, array_elements,
>> + indirect, b, location);
>> + break;
>> + }
>>
>> - return shader_program->UniformStorage[location].sampler[stage].index;
>> + default:
>> + unreachable("Invalid deref type");
>> + break;
>> + }
>> + } else {
>> + return;
>
> Why don't we just do "if (tail->child == NULL) return;" at the top?
> That way you don't have to go all the way down here to figure out what
> we do in the else case.
>
>> + }
>> }
>>
>> static void
>> lower_sampler(nir_tex_instr *instr, const struct gl_shader_program
>> *shader_program,
>> - gl_shader_stage stage, void *mem_ctx)
>> + gl_shader_stage stage, nir_builder *builder)
>> {
>> 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);
>> + unsigned location = instr->sampler->var->data.location;
>>
>> - for (nir_deref *deref = &instr->sampler->deref;
>> - deref->child; deref = deref->child) {
>> - switch (deref->child->deref_type) {
>> - case nir_deref_type_array: {
>> - nir_deref_array *deref_array = nir_deref_as_array(deref->child);
>> + if (instr->sampler->deref.child) {
>
> The calc_sampler_offsets() function does this check for you and
> becomes a no-op if cuild == NULL so there's no real reason to check it
> twice.
>
> Other than my few trivial comments here and the stuff about reworking
> the structure position function, this is looking really good! Don't
> bother and send a whole v5; just resend the stuff for 5 and 6 (which
> may now be one patch).
> --Jason
Two notes:
1) I ran this through our CI system and there were no regressions on
any Intel hardware so as soon as we get the review comments on 5 and 6
addressed, we should be good-to-go.
2) As provided, it won't build thanks to a change Rob Clark made to
nir_builder.h. I've sent out a patch to fix it but that needs to land
first.
>> + unsigned array_elements = 1;
>> + nir_ssa_def *indirect = NULL;
>>
>> - assert(deref_array->deref_array_type !=
>> nir_deref_array_type_wildcard);
>> + builder->cursor = nir_before_instr(&instr->instr);
>> + calc_sampler_offsets(&instr->sampler->deref, instr, &array_elements,
>> + &indirect, builder, &location);
>>
>> - 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;
>> - }
>> + if (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);
>>
>> - /* 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);
>> - }
>> + 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;
>> + 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,
>> + /* 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,
>> - &deref_array->indirect);
>> + nir_src_for_ssa(indirect));
>>
>> - instr->sampler_array_size = glsl_get_length(deref->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);
>> - break;
>> + instr->sampler_array_size = array_elements;
>> }
>> + }
>>
>> - default:
>> - unreachable("Invalid deref type");
>> - break;
>> - }
>> + 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 +163,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 +176,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
