[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)

[Timothy Arceri]

On Fri, 2015-07-24 at 19:37 +0200, Alejandro Piñeiro wrote:
> 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

I've been looking at this recently as part of my work on arrays of arrays I've
sent a new patch and cc'd you in on it.

I'm working towards getting rid of the UniformHash table to make things easier
for me, and besides it doesn't seem as useful as it once was. I wasn't going
to send this out so early as I wanted to get so other work done to make sure
this was indeed the right path but since you have started looking at it again
I've sent it :)


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