[Mesa-dev] [PATCH V6 10/27] i965: fix atomic buffer index for bindings other than 0
Ilia Mirkin
imirkin at alum.mit.edu
Mon Sep 28 23:08:35 PDT 2015
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'?
>
>
>>
>> >
>> > /* 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