[Mesa-dev] [PATCH 1/4] st/mesa: handle indirect samplers in arrays/structs properly (v3)

[Timothy Arceri]

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;
> +}
> +
> +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;
> +      }
> +      if (i < struct_type->length)
> +         tmp = struct_type->record_location_offset(i);
> +
> +      *location += tmp;
> +      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;

This bit doesn't seem right. This is the base location of the array, if
you keep adding it I don't see how you could be getting the right
answer.

Don't you just need to pass this into calc_deref_offsets on the first
call then add to it with each recursive call?