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

[Samuel Iglesias Gonsálvez]

On 23/10/15 12:11, Timothy Arceri wrote:
> 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.
> 

Great!

> 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.
> 

I will take a look at it later.

Thanks!

Sam

> 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;
>