[Mesa-dev] [PATCH V6 10/27] i965: fix atomic buffer index for bindings other than 0
Timothy Arceri
t_arceri at yahoo.com.au
Mon Sep 28 23:26:34 PDT 2015
On Tue, 2015-09-29 at 02:08 -0400, Ilia Mirkin wrote:
> On Tue, Sep 29, 2015 at 2:05 AM, Timothy Arceri <
> t_arceri at 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_arceri at 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 <apinheiro at 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?
>
> >
> >
> > >
> > > >
> > > > /* Calculate the surface offset */
> > > > src_reg offset(this, glsl_type::uint_type);
> > > > --
> > > > 2.4.3
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list