On Tue, 2015-09-29 at 02:33 -0400, Ilia Mirkin wrote: > On Tue, Sep 29, 2015 at 2:26 AM, Timothy Arceri < > t_arc...@yahoo.com.au> wrote: > > On Tue, 2015-09-29 at 02:08 -0400, Ilia Mirkin wrote: > > > On Tue, Sep 29, 2015 at 2:05 AM, Timothy Arceri < > > > t_arc...@yahoo.com.au> wrote: > > > > On Tue, 2015-09-29 at 01:05 -0400, Ilia Mirkin wrote: > > > > > On Mon, Sep 28, 2015 at 10:42 PM, Timothy Arceri < > > > > > t_arc...@yahoo.com.au> wrote: > > > > > > Since commit c0cd5b var->data.binding was being used as a > > > > > > replacement > > > > > > for atomic buffer index, but they don't have to be the same > > > > > > value > > > > > > they > > > > > > just happen to end up the same when binding is 0. > > > > > > > > > > > > Now that we store the atomic uniform location in var > > > > > > ->data.location > > > > > > we can use this to lookup the atomic buffer index in > > > > > > uniform > > > > > > storage. > > > > > > > > > > > > For arrays of arrays the outer arrays have separate uniform > > > > > > locations > > > > > > however they all share the same buffer so we can get the > > > > > > buffer > > > > > > index > > > > > > using the base uniform location. > > > > > > > > > > > > Cc: Alejandro Piñeiro <apinhe...@igalia.com> > > > > > > Bugzilla: > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=90175 > > > > > > --- > > > > > > src/glsl/nir/glsl_to_nir.cpp | 2 -- > > > > > > src/glsl/nir/nir.h | 4 ++-- > > > > > > src/glsl/nir/nir_lower_atomics.c | 18 > > > > > > ++++++++++++-- > > > > > > ---- > > > > > > src/mesa/drivers/dri/i965/brw_nir.c | 6 ++++-- > > > > > > src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +- > > > > > > 5 files changed, 19 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/src/glsl/nir/glsl_to_nir.cpp > > > > > > b/src/glsl/nir/glsl_to_nir.cpp > > > > > > index f03a107..30726be 100644 > > > > > > --- a/src/glsl/nir/glsl_to_nir.cpp > > > > > > +++ b/src/glsl/nir/glsl_to_nir.cpp > > > > > > @@ -330,8 +330,6 @@ 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.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; > > > > > > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h > > > > > > index 4f45770..670e39e 100644 > > > > > > --- a/src/glsl/nir/nir.h > > > > > > +++ b/src/glsl/nir/nir.h > > > > > > @@ -308,7 +308,6 @@ typedef struct { > > > > > > * Location an atomic counter is stored at. > > > > > > */ > > > > > > struct { > > > > > > - unsigned buffer_index; > > > > > > unsigned offset; > > > > > > } atomic; > > > > > > > > > > > > @@ -1880,7 +1879,8 @@ void nir_lower_clip_fs(nir_shader > > > > > > *shader, > > > > > > unsigned ucp_enables); > > > > > > > > > > > > void nir_lower_two_sided_color(nir_shader *shader); > > > > > > > > > > > > -void nir_lower_atomics(nir_shader *shader); > > > > > > +void nir_lower_atomics(nir_shader *shader, > > > > > > + const struct gl_shader_program > > > > > > *shader_program); > > > > > > void nir_lower_to_source_mods(nir_shader *shader); > > > > > > > > > > > > bool nir_lower_gs_intrinsics(nir_shader *shader); > > > > > > diff --git a/src/glsl/nir/nir_lower_atomics.c > > > > > > b/src/glsl/nir/nir_lower_atomics.c > > > > > > index 46e1376..52e7675 100644 > > > > > > --- a/src/glsl/nir/nir_lower_atomics.c > > > > > > +++ b/src/glsl/nir/nir_lower_atomics.c > > > > > > @@ -25,6 +25,7 @@ > > > > > > * > > > > > > */ > > > > > > > > > > > > +#include "ir_uniform.h" > > > > > > #include "nir.h" > > > > > > #include "main/config.h" > > > > > > #include <assert.h> > > > > > > @@ -35,7 +36,8 @@ > > > > > > */ > > > > > > > > > > > > static void > > > > > > -lower_instr(nir_intrinsic_instr *instr, nir_function_impl > > > > > > *impl) > > > > > > +lower_instr(nir_intrinsic_instr *instr, > > > > > > + const struct gl_shader_program > > > > > > *shader_program) > > > > > > { > > > > > > nir_intrinsic_op op; > > > > > > switch (instr->intrinsic) { > > > > > > @@ -60,10 +62,11 @@ lower_instr(nir_intrinsic_instr *instr, > > > > > > nir_function_impl *impl) > > > > > > return; /* atomics passed as function arguments > > > > > > can't be > > > > > > lowered */ > > > > > > > > > > > > void *mem_ctx = ralloc_parent(instr); > > > > > > + unsigned uniform_loc = instr->variables[0]->var > > > > > > ->data.location; > > > > > > > > > > > > nir_intrinsic_instr *new_instr = > > > > > > nir_intrinsic_instr_create(mem_ctx, op); > > > > > > new_instr->const_index[0] = > > > > > > - (int) instr->variables[0]->var > > > > > > ->data.atomic.buffer_index; > > > > > > + shader_program > > > > > > ->UniformStorage[uniform_loc].atomic_buffer_index; > > > > > > > > > > > > nir_load_const_instr *offset_const = > > > > > > nir_load_const_instr_create(mem_ctx, 1); > > > > > > offset_const->value.u[0] = instr->variables[0]->var > > > > > > ->data.atomic.offset; > > > > > > @@ -128,22 +131,25 @@ lower_instr(nir_intrinsic_instr > > > > > > *instr, > > > > > > nir_function_impl *impl) > > > > > > } > > > > > > > > > > > > static bool > > > > > > -lower_block(nir_block *block, void *state) > > > > > > +lower_block(nir_block *block, void *prog) > > > > > > { > > > > > > nir_foreach_instr_safe(block, instr) { > > > > > > if (instr->type == nir_instr_type_intrinsic) > > > > > > - lower_instr(nir_instr_as_intrinsic(instr), > > > > > > state); > > > > > > + lower_instr(nir_instr_as_intrinsic(instr), > > > > > > + (const struct gl_shader_program *) > > > > > > prog); > > > > > > } > > > > > > > > > > > > return true; > > > > > > } > > > > > > > > > > > > void > > > > > > -nir_lower_atomics(nir_shader *shader) > > > > > > +nir_lower_atomics(nir_shader *shader, > > > > > > + const struct gl_shader_program > > > > > > *shader_program) > > > > > > { > > > > > > nir_foreach_overload(shader, overload) { > > > > > > if (overload->impl) { > > > > > > - nir_foreach_block(overload->impl, lower_block, > > > > > > overload > > > > > > ->impl); > > > > > > + nir_foreach_block(overload->impl, lower_block, > > > > > > + (void *) shader_program); > > > > > > nir_metadata_preserve(overload->impl, > > > > > > nir_metadata_block_index | > > > > > > > > > > > > nir_metadata_dominance); > > > > > > } > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_nir.c > > > > > > b/src/mesa/drivers/dri/i965/brw_nir.c > > > > > > index 1d4f6ab..22909a1 100644 > > > > > > --- a/src/mesa/drivers/dri/i965/brw_nir.c > > > > > > +++ b/src/mesa/drivers/dri/i965/brw_nir.c > > > > > > @@ -155,8 +155,10 @@ brw_create_nir(struct brw_context > > > > > > *brw, > > > > > > nir_lower_system_values(nir); > > > > > > nir_validate_shader(nir); > > > > > > > > > > > > - nir_lower_atomics(nir); > > > > > > - nir_validate_shader(nir); > > > > > > + if (shader_prog) { > > > > > > + nir_lower_atomics(nir, shader_prog); > > > > > > + nir_validate_shader(nir); > > > > > > + } > > > > > > > > > > > > nir_optimize(nir, is_scalar); > > > > > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > > > > > > b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > > > > > > index 63c40ba..a74eb86 100644 > > > > > > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > > > > > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > > > > > > @@ -2435,7 +2435,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.binding); > > > > > > + shader_prog->UniformStorage[location > > > > > > ->data.location].atomic_buffer_index); > > > > > > > > > > Shouldn't there be a similar adjustment in > > > > > brw_upload_abo_surfaces? > > > > > (Although I admit to not fully understanding this stuff, so > > > > > very > > > > > much > > > > > an open question.) > > > > > > > > Which part are you talking about? > > > > > > > > I assume you mean this line: > > > > > > > > struct gl_atomic_buffer_binding *binding = > > > > &ctx->AtomicBufferBindings[prog > > > > ->AtomicBuffers[i].Binding] > > > > > > > > It looks fine to me. > > > > > > > > prog->AtomicBuffers[i].Binding gets the binding for each active > > > > buffer > > > > and then uses the binding as the index to AtomicBufferBindings > > > > to > > > > look > > > > up the buffer object which was set in bind_atomic_buffer() > > > > using > > > > the > > > > binding as the index. > > > > > > No. I mean the line: > > > > > > brw->vtbl.emit_buffer_surface_state(brw, &surf_offsets[i], > > > bo, > > > > > > Shouldn't that look up the atomic buffer index in prog->Uniforms > > > instead of using 'i'? > > > > No that looks fine. The problem in the patch was we didn't have the > > atomic buffex index and we used the binding as the offset which > > doesn't > > have to be the same as the buffer index. > > > > In brw_upload_abo_surfaces its the opposite we have the atomic > > buffer > > index which is i and we are looking up the binding to get the > > buffer > > object. > > > > Does that make sense? > > i is the binding index though...
i is the buffer index See find_active_atomic_counters() for the code the counts the active buffers. http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/link_atomics.cpp#n9 9 This count is then used to set NumAtomicBuffers and to create the prog ->AtomicBuffers array so the array is no bigger than it needs to be. This is the reason the binding and atomic buffer index can differ. http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/link_atomics.cpp#n1 73 > i ranges from 0 to NumAtomicBuffers > which goes up to NumAtomicBuffers which can be go up to > MaxCombinedAtomicBuffers. Or so I understand. > -ilia _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
