[Mesa-dev] [PATCH] glsl: add buffer block number information to atomic counter variables.

[Timothy Arceri]

On Fri, 2015-10-23 at 08:57 +0200, Samuel Iglesias Gonsalvez wrote:
> Atomic counter variables can have a 'binding' layout modifier.
> Unfortunately,
> the atomic counter buffers are not sorted by binding value in
> gl_shader_program's AtomicBuffers.
> 
> Save the atomic counter buffer block index into the variable, so we
> can use it inside the drivers.
> 
> Fixes 298 dEQP-GLES31.functional.atomic_counter.* tests and
> 9 dEQP-GLES31.functional.synchronization.inter_call.
>   without_memory_barrier.ssbo_atomic_counter_mixed_dispatch_*
> tests for i965 driver.

Hi Samuel,

I've sent a number of patches trying to fix the problem with the
binding without reintroducing the buffer_index field.

You can see the latest attempt here [1] based on feedback from curro
and Ilia, also see the bug for a history of the problem.

Tim

[1] http://patchwork.freedesktop.org/patch/60989/



> 
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> ---
>  src/glsl/ir.h                |  1 +
>  src/glsl/link_atomics.cpp    | 32 ++++++++++++++++++++++++++++++++
>  src/glsl/linker.cpp          |  1 +
>  src/glsl/linker.h            |  3 +++
>  src/glsl/nir/glsl_to_nir.cpp |  3 +--
>  5 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index 9c9f22d..d120203 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -851,6 +851,7 @@ public:
>         * Location an atomic counter is stored at.
>         */
>        struct {
> +         unsigned buffer_index;
>           unsigned offset;
>        } atomic;
>  
> diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
> index 70ef0e1..d19cd81 100644
> --- a/src/glsl/link_atomics.cpp
> +++ b/src/glsl/link_atomics.cpp
> @@ -193,6 +193,38 @@ namespace {
>     }
>  }
>  
> +/**
> + * Scan the program for atomic counters and store buffer index
> + * information into the variable data structure.
> + */
> +void
> +link_assign_atomic_counter_variable_buffer_index(struct
> gl_shader_program *prog)
> +{
> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> +      gl_shader *sh = prog->_LinkedShaders[i];
> +
> +      if (sh == NULL)
> +	 continue;
> +
> +      foreach_in_list(ir_instruction, node, sh->ir) {
> +	 ir_variable *var = node->as_variable();
> +
> +         if (var && var->data.mode == ir_var_uniform &&
> +             var->type->contains_atomic()) {
> +            for(unsigned i = 0; i < prog->NumAtomicBuffers; i++) {
> +               if (prog->AtomicBuffers[i].Binding ==  (unsigned) var
> ->data.binding) {
> +                  /* Save the buffer block index that corresponds to
> variable's
> +                   * binding.
> +                   */
> +                  var->data.atomic.buffer_index = i;
> +                  break;
> +               }
> +            }
> +         }
> +      }
> +   }
> +}
> +
>  void
>  link_assign_atomic_counter_resources(struct gl_context *ctx,
>                                       struct gl_shader_program *prog)
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 424b92a..c8334f6 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -4361,6 +4361,7 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
>     update_array_sizes(prog);
>     link_assign_uniform_locations(prog, ctx
> ->Const.UniformBooleanTrue);
>     link_assign_atomic_counter_resources(ctx, prog);
> +   link_assign_atomic_counter_variable_buffer_index(prog);
>     store_fragdepth_layout(prog);
>  
>     link_calculate_subroutine_compat(prog);
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index c80be1c..f16dd64 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -79,6 +79,9 @@ validate_interstage_uniform_blocks(struct
> gl_shader_program *prog,
>                                     gl_shader **stages, int
> num_stages);
>  
>  extern void
> +link_assign_atomic_counter_variable_buffer_index(struct
> gl_shader_program *prog);
> +
> +extern void
>  link_assign_atomic_counter_resources(struct gl_context *ctx,
>                                       struct gl_shader_program
> *prog);
>  
> diff --git a/src/glsl/nir/glsl_to_nir.cpp
> b/src/glsl/nir/glsl_to_nir.cpp
> index 9b50a93..aaeb53b 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -392,8 +392,7 @@ nir_visitor::visit(ir_variable *ir)
>  
>     var->data.index = ir->data.index;
>     var->data.binding = ir->data.binding;
> -   /* XXX Get rid of buffer_index */
> -   var->data.atomic.buffer_index = ir->data.binding;
> +   var->data.atomic.buffer_index = ir->data.atomic.buffer_index;
>     var->data.atomic.offset = ir->data.atomic.offset;
>     var->data.image.read_only = ir->data.image_read_only;
>     var->data.image.write_only = ir->data.image_write_only;