[Mesa-dev] [PATCH 06/11] st/mesa: add atomic buffer support

[Ilia Mirkin]

On Sun, Sep 27, 2015 at 3:20 AM, Kai Wasserbäch
<kai at dev.carbon-project.org> wrote:
> Ilia Mirkin wrote on 27.09.2015 08:33:
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>  src/mesa/Makefile.sources                    |   1 +
>>  src/mesa/program/ir_to_mesa.cpp              |   4 +-
>>  src/mesa/state_tracker/st_atom.c             |   5 +
>>  src/mesa/state_tracker/st_atom.h             |   5 +
>>  src/mesa/state_tracker/st_atom_atomicbuf.c   | 151 +++++++++++++++++++++++++++
>>  src/mesa/state_tracker/st_cb_bufferobjects.c |   3 +
>>  src/mesa/state_tracker/st_context.c          |   1 +
>>  src/mesa/state_tracker/st_context.h          |   1 +
>>  src/mesa/state_tracker/st_extensions.c       |  15 +++
>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp   | 133 +++++++++++++++++++++--
>>  10 files changed, 310 insertions(+), 9 deletions(-)
>>  create mode 100644 src/mesa/state_tracker/st_atom_atomicbuf.c
>>
>> [...]
>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> index 633e90f..28c9637 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>> @@ -261,6 +261,8 @@ public:
>> [...]
>>
>>  void
>> +glsl_to_tgsi_visitor::visit_atomic_counter_intrinsic(ir_call *ir)
>> +{
>> +   const char *callee = ir->callee->function_name();
>> +   ir_dereference *deref = static_cast<ir_dereference *>(
>> +      ir->actual_parameters.get_head());
>> +   ir_variable *location = deref->variable_referenced();
>> +
>> +   /* XXX use accept */
>> +   st_src_reg buffer(
>> +         PROGRAM_SAMPLER, location->data.binding /* XXX */, GLSL_TYPE_ATOMIC_UINT);
>> +
>> +   /* Calculate the surface offset */
>> +   st_src_reg offset;
>> +   ir_dereference_array *deref_array = deref->as_dereference_array();
>> +
>> +   if (deref_array) {
>> +      offset = get_temp(glsl_type::uint_type);
>> +
>> +      deref_array->array_index->accept(this);
>> +
>> +      emit_asm(ir, TGSI_OPCODE_MUL, st_dst_reg(offset),
>> +               this->result, st_src_reg_for_int(ATOMIC_COUNTER_SIZE));
>> +      emit_asm(ir, TGSI_OPCODE_ADD, st_dst_reg(offset),
>> +               offset, st_src_reg_for_int(location->data.atomic.offset));
>> +   } else {
>> +      offset = st_src_reg_for_int(location->data.atomic.offset);
>> +   }
>> +
>> +   ir->return_deref->accept(this);
>> +   st_dst_reg dst(this->result);
>> +   dst.writemask = WRITEMASK_X;
>> +
>> +   glsl_to_tgsi_instruction *inst;
>> +
>> +   if (!strcmp("__intrinsic_atomic_read", callee)) {
>> +      inst = emit_asm(ir, TGSI_OPCODE_LOAD, dst, offset);
>> +      inst->buffer = buffer;
>> +   } else if (!strcmp("__intrinsic_atomic_increment", callee)) {
>> +      inst = emit_asm(ir, TGSI_OPCODE_ATOMUADD, dst, offset,
>> +                      st_src_reg_for_int(1));
>> +      inst->buffer = buffer;
>> +   } else if (!strcmp("__intrinsic_atomic_predecrement", callee)) {
>> +      inst = emit_asm(ir, TGSI_OPCODE_ATOMUADD, dst, offset,
>> +                      st_src_reg_for_int(-1));
>> +      inst->buffer = buffer;
>> +      emit_asm(ir, TGSI_OPCODE_ADD, dst, this->result, st_src_reg_for_int(-1));
>> +   }
>> +}
>> +
>> +void
>>  glsl_to_tgsi_visitor::visit(ir_call *ir)
>>  {
>>     glsl_to_tgsi_instruction *call_inst;
>>     ir_function_signature *sig = ir->callee;
>> +   const char *callee = sig->function_name();
>>     function_entry *entry = get_function_signature(sig);
>>     int i;
>>
>> +   /* Filter out intrinsics */
>> +   if (!strcmp("__intrinsic_atomic_read", callee) ||
>> +       !strcmp("__intrinsic_atomic_increment", callee) ||
>> +       !strcmp("__intrinsic_atomic_predecrement", callee)) {
>> +      visit_atomic_counter_intrinsic(ir);
>> +      return;
>> +   }
>
> You're doing the same string comparison two times in a row (if you match here).
> Wouldn't it be cheaper to cache the result und pass it in to
> visit_atomic_counter_intrinsic()?
>
> I was thinking of something like
>
> unsigned atomic_intr_type = 0;
> if(!strcmp("__intrinsic_atomic_read", callee))
>   atomic_intr_type = 1;
> else if(!strcmp("__intrinsic_atomic_increment", callee))
>   atomic_intr_type = 2;
> else if(!strcmp("__intrinsic_atomic_predecrement", callee))
>   atomic_intr_type = 3;
>
> if(atomic_intr_type) {
>   visit_atomic_counter_intrinsic(ir, atomic_intr_type);
>   return;
> }
>
> Obviously visit_atomic_counter_intrinsic() would need to take that parameter and
> then replace the respective if block with a check for the right code.
>
>
> This is just a suggestion and I might be totally missing something here. ;-)

You're quite right. ir_call's are rare though, so I doubt this would
have much impact. Happy to stick a todo in there.

  -ilia