[Mesa-dev] Atomic counters doesn't work for a binding point different to zero (was Re: [PATCH] glsl: set the binding value regardless explicit_binding)

[Alejandro Piñeiro]

On 14/05/15 20:38, Ian Romanick wrote:

Hi,

today I was able to go back to work on this patch. Sorry for the delay.
I changed the subject to the problem, as previous fix was wrong. I have
some questions (see below).

>>
>>
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> @@ -2294,7 +2294,7 @@
>> vec4_visitor::visit_atomic_counter_intrinsic(ir_call *ir)
>>        ir->actual_parameters.get_head());
>>     ir_variable *location = deref->variable_referenced();
>>     unsigned surf_index = (prog_data->base.binding_table.abo_start +
>> -                          location->data.atomic.buffer_index);
>> +                          location->data.binding);
>>
>>     /* Calculate the surface offset */
>>     src_reg offset(this, glsl_type::uint_type);
>>
>> Maybe try adding a utility function that will search
>> gl_shader_program::UniformStorage for location->name and use the
>> atomic_buffer_index stored there for use in the i965 driver?
>>
>>>>           storage->atomic_buffer_index = i;
>>>>           storage->offset = var->data.atomic.offset;

I was able to do that on brw_vec4_visitor in a really straighforward
way. It is not required to create a utility method, as the code needed
is really sort. The change is something like this:

+   unsigned location_index = 0;
+   const bool found = shader_prog->UniformHash->get(location_index,
+                                                    location->name);
+   assert(found);
    unsigned surf_index = (prog_data->base.binding_table.abo_start +
-                          location->data.binding);
+                         
shader_prog->UniformStorage[location_index].atomic_buffer_index);


But that would not work on brw_fs, as it already switched to NIR (so it
will not be valid in any case on vec4 in the future).

On NIR the variables (that we get on IR with ir_dereference, deref, etc)
are stored on some intrinsics at nir_intrinsic_instr::variables. So, at
first I tried to change the second line to something like this:         

const bool found = shader_prog->UniformHash->get(location_index,
instr->variables[0]->var->name)

The problem is that nir_intrinsic_atomic_counter_read doesn't include
info on variables. The one that include that info there is
nir_intrinsic_atomic_counter_read_var. The latter is lowered to the
former at nir_lower_atomics, that "replace atomic counter intrinsics
that use a variable with intrinsics that directly store the buffer index
and byte offset". On that lowering the content of
instr->variables[0]->var->data.atomic.buffer_index is stored at
new_instr->const_index[0] (see here [1]), but that content is already
wrong, because it is filled like this:
   var->data.atomic.buffer_index = ir->data.binding; (see here [2])

because IR doesn't have a specific buffer index anymore (as commit
c0cd5b assumed that atomic buffer index and binding point was the same).

Additionally, this lowering doesn't import (as far as I see) the uniform
name, that is the one I was using to get the buffer index from the
shader prog at the i965 driver.

Initially I was tempted to do the same trick on the lowering, so adding
shader_program when you call this lowering, as nir_lower_samples do. But
I find that it will be somewhat confusing the lowering assuming that the
value at instr->variables[0]->var->data.atomic.buffer_index is wrong,
and that it should use the shader program instead. Probably is just
enough to add more info on the comment "/* XXX Get rid of buffer_index
*/" here [2], but I preferred to ask first.

Thanks in advance

BR

[1]
https://github.com/Igalia/mesa/blob/master/src/glsl/nir/nir_lower_atomics.c#L65
[2]
https://github.com/Igalia/mesa/blob/master/src/glsl/nir/glsl_to_nir.cpp#L330


-- 
Alejandro Piñeiro (apinheiro at igalia.com)