[Mesa-dev] [PATCH 1/4] st/mesa: handle indirect samplers in arrays/structs properly (v3)
Timothy Arceri
t_arceri at yahoo.com.au
Fri Feb 5 23:31:39 UTC 2016
On Fri, 2016-02-05 at 13:40 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> The state tracker never handled this properly, and it finally
> annoyed me for the second time so I decided to fix it properly.
>
> This is inspired by the NIR sampler lowering code and I only realised
> NIR seems to do its deref ordering different to GLSL at the last
> minute, once I got that things got much easier.
>
> it fixes a bunch of tests in
> tests/spec/arb_gpu_shader5/execution/sampler_array_indexing/
>
> v2: fix AoA tests when forced on.
> I was right I didn't need all that code, fixing the AoA code
> meant cleaning up a chunk of code I didn't like in the array
> handling.
>
> v3: start generalising the code a bit more for atomics.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 145
> +++++++++++++++++++++++++----
> 1 file changed, 127 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index b8182de..c36edc9 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -40,7 +40,6 @@
> #include "main/uniforms.h"
> #include "main/shaderapi.h"
> #include "program/prog_instruction.h"
> -#include "program/sampler.h"
>
> #include "pipe/p_context.h"
> #include "pipe/p_screen.h"
> @@ -257,6 +256,7 @@ public:
> GLboolean cond_update;
> bool saturate;
> st_src_reg sampler; /**< sampler register */
> + int sampler_base;
> int sampler_array_size; /**< 1-based size of sampler array, 1 if
> not array */
> int tex_target; /**< One of TEXTURE_*_INDEX */
> glsl_base_type tex_type;
> @@ -502,6 +502,18 @@ public:
>
> void emit_arl(ir_instruction *ir, st_dst_reg dst, st_src_reg
> src0);
>
> + void get_deref_offsets(ir_dereference *ir,
> + unsigned *array_size,
> + unsigned *base,
> + unsigned *index,
> + st_src_reg *reladdr);
> + void calc_deref_offsets(ir_dereference *head,
> + ir_dereference *tail,
> + unsigned *array_elements,
> + unsigned *index,
> + st_src_reg *indirect,
> + unsigned *location);
> +
> bool try_emit_mad(ir_expression *ir,
> int mul_operand);
> bool try_emit_mad_for_and_not(ir_expression *ir,
> @@ -3436,18 +3448,118 @@ glsl_to_tgsi_visitor::visit(ir_call *ir)
> this->result = entry->return_reg;
> }
>
> +static unsigned
> +glsl_get_length(const struct glsl_type *type)
> +{
> + return type->is_matrix() ? type->matrix_columns : type->length;
> +}
This isn't needed the type should always be an array unless something
has gone horribly wrong. Nir uses this because it needs a wrapper for
cpp. I don't think the matrix stuff was even there when I added nir
support originally.
> +
> +void
> +glsl_to_tgsi_visitor::calc_deref_offsets(ir_dereference *head,
> + ir_dereference *tail,
> + unsigned *array_elements,
> + unsigned *index,
> + st_src_reg *indirect,
> + unsigned *location)
> +{
> + switch (tail->ir_type) {
> + case ir_type_dereference_record: {
> + ir_dereference_record *deref_record = tail-
> >as_dereference_record();
> + const glsl_type *struct_type = deref_record->record->type;
> + int tmp = 0;
> + unsigned i;
> +
> + calc_deref_offsets(head, deref_record->record-
> >as_dereference(), array_elements, index, indirect, location);
> +
> + for (i = 0; i < struct_type->length; i++) {
> + if (strcmp(struct_type->fields.structure[i].name,
> deref_record->field) == 0)
> + break;
> + }
Rather than doing this you can used do:
i = deref_record->record->type->field_index(deref_record->field);
> + if (i < struct_type->length)
> + tmp = struct_type->record_location_offset(i);
> +
> + *location += tmp;
As pointed out on irc location needs to be initialised to the base
struct location currently this is always starting at 0. Solution
further down.
> + break;
> + }
> +
> + case ir_type_dereference_array: {
> + ir_dereference_array *deref_arr = tail-
> >as_dereference_array();
> + ir_constant *array_index = deref_arr->array_index-
> >constant_expression_value();
> +
> + if (deref_arr->array->ir_type == ir_type_dereference_variable
> && head == tail) {
> + ir_dereference_variable *deref_var = deref_arr->array-
> >as_dereference_variable();
> + *location += deref_var->var->data.location;
> + }
> +
> + if (!array_index) {
> + st_src_reg temp_reg;
> + st_dst_reg temp_dst;
> +
> + temp_reg = get_temp(glsl_type::uint_type);
> + temp_dst = st_dst_reg(temp_reg);
> + temp_dst.writemask = 1;
> +
> + deref_arr->array_index->accept(this);
> + emit_asm(NULL, TGSI_OPCODE_MUL, temp_dst, this->result,
> st_src_reg_for_int(*array_elements));
> +
> + if (indirect->file == PROGRAM_UNDEFINED)
> + *indirect = temp_reg;
> + else {
> + temp_dst = st_dst_reg(*indirect);
> + temp_dst.writemask = 1;
> + emit_asm(NULL, TGSI_OPCODE_ADD, temp_dst, *indirect,
> temp_reg);
> + }
> + } else
> + *index += array_index->value.u[0] * *array_elements;
> +
> + *array_elements *= glsl_get_length(deref_arr->array->type);
Just deref_arr->array->type->length will do as pointed out above.
> +
> + calc_deref_offsets(head, deref_arr->array->as_dereference(),
> array_elements, index, indirect, location);
> + break;
> + }
> + case ir_type_dereference_variable: {
> + if (head == tail) {
> + ir_dereference_variable *deref_var = tail-
> >as_dereference_variable();
> + *location = deref_var->var->data.location;
> + }
You don't really need this or any of the other head==tail stuff since
as pointed out above you need to initalise location with the base
location so you can do this outside the first calc_deref_offsets() then
you can just make this return or add:
if (deref->ir_type == ir_type_dereference_variable)
return;
I can't say I've taken time to fully understand the tgsi specifics here but I think I've looked at it enough to give:
Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com> to patches 1, 2 and 4 if you make the suggested changes (and it still works of course).
I'll leave you and Ilia to figure out patch 3 :)
I'm also about to send a couple of follow up patches to tidy up the remaining ir_to_mesa stuff. I based it off this patch :P I think I did it once before and failed. Now that I think of it I was probably trying to do the deref in the NIR order rather than GLSL IR order.
> + break;
> + }
> + default:
> + break;
> + }
> +}
> +
> +void
> +glsl_to_tgsi_visitor::get_deref_offsets(ir_dereference *ir,
> + unsigned *array_size,
> + unsigned *base,
> + unsigned *index,
> + st_src_reg *reladdr)
> +{
> + GLuint shader = _mesa_program_enum_to_shader_stage(this->prog-
> >Target);
> + unsigned location = 0;
> +
> + memset(reladdr, 0, sizeof(*reladdr));
> + reladdr->file = PROGRAM_UNDEFINED;
> +
> + *array_size = 1;
> + calc_deref_offsets(ir, ir, array_size, index, reladdr,
> &location);
> +
> + *base = this->shader_program-
> >UniformStorage[location].opaque[shader].index;
> + *index += *base;
> +}
> +
> void
> glsl_to_tgsi_visitor::visit(ir_texture *ir)
> {
> st_src_reg result_src, coord, cube_sc, lod_info, projector, dx,
> dy;
> st_src_reg offset[MAX_GLSL_TEXTURE_OFFSET], sample_index,
> component;
> - st_src_reg levels_src;
> + st_src_reg levels_src, reladdr;
> st_dst_reg result_dst, coord_dst, cube_sc_dst;
> glsl_to_tgsi_instruction *inst = NULL;
> unsigned opcode = TGSI_OPCODE_NOP;
> const glsl_type *sampler_type = ir->sampler->type;
> - ir_rvalue *sampler_index =
> - _mesa_get_sampler_array_nonconst_index(ir->sampler);
> + unsigned sampler_array_size = 1, sampler_index = 0, sampler_base
> = 0;
> bool is_cube_array = false;
> unsigned i;
>
> @@ -3669,10 +3781,10 @@ glsl_to_tgsi_visitor::visit(ir_texture *ir)
> coord_dst.writemask = WRITEMASK_XYZW;
> }
>
> - if (sampler_index) {
> - sampler_index->accept(this);
> - emit_arl(ir, sampler_reladdr, this->result);
> - }
> + get_deref_offsets(ir->sampler, &sampler_array_size,
> &sampler_base,
> + &sampler_index, &reladdr);
> + if (reladdr.file != PROGRAM_UNDEFINED)
> + emit_arl(ir, sampler_reladdr, reladdr);
>
> if (opcode == TGSI_OPCODE_TXD)
> inst = emit_asm(ir, opcode, result_dst, coord, dx, dy);
> @@ -3705,16 +3817,13 @@ glsl_to_tgsi_visitor::visit(ir_texture *ir)
> if (ir->shadow_comparitor)
> inst->tex_shadow = GL_TRUE;
>
> - inst->sampler.index = _mesa_get_sampler_uniform_value(ir-
> >sampler,
> - this-
> >shader_program,
> - this-
> >prog);
> - if (sampler_index) {
> + inst->sampler.index = sampler_index;
> + inst->sampler_array_size = sampler_array_size;
> + inst->sampler_base = sampler_base;
> +
> + if (reladdr.file != PROGRAM_UNDEFINED) {
> inst->sampler.reladdr = ralloc(mem_ctx, st_src_reg);
> - memcpy(inst->sampler.reladdr, &sampler_reladdr,
> sizeof(sampler_reladdr));
> - inst->sampler_array_size =
> - ir->sampler->as_dereference_array()->array->type-
> >array_size();
> - } else {
> - inst->sampler_array_size = 1;
> + memcpy(inst->sampler.reladdr, &reladdr, sizeof(reladdr));
> }
>
> if (ir->offset) {
> @@ -3915,7 +4024,7 @@ count_resources(glsl_to_tgsi_visitor *v,
> gl_program *prog)
> foreach_in_list(glsl_to_tgsi_instruction, inst, &v->instructions)
> {
> if (inst->info->is_tex) {
> for (int i = 0; i < inst->sampler_array_size; i++) {
> - unsigned idx = inst->sampler.index + i;
> + unsigned idx = inst->sampler_base + i;
> v->samplers_used |= 1 << idx;
>
> debug_assert(idx < (int)ARRAY_SIZE(v->sampler_types));
More information about the mesa-dev
mailing list