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 (apinhe...@igalia.com) _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev