[Mesa-dev] [PATCH V2] glsl: fix atomic buffer index for bindings other than 0
Timothy Arceri
t_arceri at yahoo.com.au
Sun Jul 26 01:47:22 PDT 2015
On Sat, 2015-07-25 at 18:11 +0200, Alejandro Piñeiro wrote:
> Hi Timothy,
>
> thanks for CCing me. Just to say that it looks good to me. And FWIW,
> with this patch, the piglit subtest included on the second version of my
> patch (second version after the first review of Ian Romanick) here:
>
> http://lists.freedesktop.org/archives/piglit/2015-May/015979.html
Thanks for testing. I tested with your piglit patch too I'll try give your
second version a review soon :)
I've also sent a V3 of my patch as I realised just after sending V2 that I
shouldn't have needed to be messing with the uniform location.
>
> pass properly.
>
> Best regards
>
> On 25/07/15 16:24, Timothy Arceri 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 we store atomic buffer index in the unused var->data.index
> > to avoid the extra memory of putting back the atmoic buffer index field.
> >
> > V2: store buffer index in var->data.index and uniform slot in
> > var->data.location to avoid issues when linking more than 2 shaders.
> > Also some small tidy ups.
> >
> > Cc: Alejandro Piñeiro <apinheiro at igalia.com>
> > Cc: Ian Romanick <idr at freedesktop.org>
> > Cc: 10.4, 10.5 <mesa-stable at lists.freedesktop.org>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
> > ---
> > src/glsl/ir.h | 3 +++
> > src/glsl/link_atomics.cpp | 18 +++++++-----------
> > src/glsl/link_uniforms.cpp | 4 ++++
> > src/glsl/nir/glsl_to_nir.cpp | 2 --
> > src/glsl/nir/nir.h | 6 ++++--
> > src/glsl/nir/nir_lower_atomics.c | 2 +-
> > src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +-
> > 7 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> > index ede8caa..e76b0ec 100644
> > --- a/src/glsl/ir.h
> > +++ b/src/glsl/ir.h
> > @@ -757,6 +757,8 @@ public:
> > * \note
> > * The GLSL spec only allows the values 0 or 1 for the index in \b
> > dual
> > * source blending.
> > + *
> > + * For atomic counters this stores the atomic buffer index.
> > */
> > unsigned index:1;
> >
> > @@ -819,6 +821,7 @@ public:
> > * - Fragment shader output: one of the values from \c
> > gl_frag_result.
> > * - Uniforms: Per-stage uniform slot number for default uniform
> > block.
> > * - Uniforms: Index within the uniform block definition for UBO
> > members.
> > + * - Atomic Counter: Uniform slot number.
> > * - Other: This field is not currently used.
> > *
> > * If the variable is a uniform, shader input, or shader output,
> > and the
> > diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
> > index 100d03c..5d3c40f 100644
> > --- a/src/glsl/link_atomics.cpp
> > +++ b/src/glsl/link_atomics.cpp
> > @@ -33,7 +33,6 @@ namespace {
> > * Atomic counter as seen by the program.
> > */
> > struct active_atomic_counter {
> > - unsigned id;
> > ir_variable *var;
> > };
> >
> > @@ -52,7 +51,7 @@ namespace {
> > free(counters);
> > }
> >
> > - void push_back(unsigned id, ir_variable *var)
> > + void push_back(ir_variable *var)
> > {
> > active_atomic_counter *new_counters;
> >
> > @@ -66,7 +65,6 @@ namespace {
> > }
> >
> > counters = new_counters;
> > - counters[num_counters].id = id;
> > counters[num_counters].var = var;
> > num_counters++;
> > }
> > @@ -114,10 +112,6 @@ namespace {
> > ir_variable *var = node->as_variable();
> >
> > if (var && var->type->contains_atomic()) {
> > - unsigned id = 0;
> > - bool found = prog->UniformHash->get(id, var->name);
> > - assert(found);
> > - (void) found;
> > active_atomic_buffer *buf = &buffers[var->data.binding];
> >
> > /* If this is the first time the buffer is used, increment
> > @@ -126,7 +120,7 @@ namespace {
> > if (buf->size == 0)
> > (*num_buffers)++;
> >
> > - buf->push_back(id, var);
> > + buf->push_back(var);
> >
> > buf->stage_references[i]++;
> > buf->size = MAX2(buf->size, var->data.atomic.offset +
> > @@ -197,13 +191,15 @@ link_assign_atomic_counter_resources(struct
> > gl_context *ctx,
> > /* Assign counter-specific fields. */
> > for (unsigned j = 0; j < ab.num_counters; j++) {
> > ir_variable *const var = ab.counters[j].var;
> > - const unsigned id = ab.counters[j].id;
> > - gl_uniform_storage *const storage = &prog->UniformStorage[id];
> > + gl_uniform_storage *const storage =
> > + &prog->UniformStorage[var->data.location];
> >
> > - mab.Uniforms[j] = id;
> > + mab.Uniforms[j] = var->data.location;
> > if (!var->data.explicit_binding)
> > var->data.binding = i;
> >
> > + var->data.index = i;
> > +
> > storage->atomic_buffer_index = i;
> > storage->offset = var->data.atomic.offset;
> > storage->array_stride = (var->type->is_array() ?
> > diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> > index 61b47c9..874db09 100644
> > --- a/src/glsl/link_uniforms.cpp
> > +++ b/src/glsl/link_uniforms.cpp
> > @@ -620,6 +620,10 @@ private:
> > handle_images(base_type, &this->uniforms[id]);
> > handle_subroutines(base_type, &this->uniforms[id]);
> >
> > + if (base_type->contains_atomic()) {
> > + current_var->data.location = id;
> > + }
> > +
> > /* If there is already storage associated with this uniform or if
> > the
> > * uniform is set as builtin, it means that it was set while
> > processing
> > * an earlier shader stage. For example, we may be processing the
> > diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> > index 66430f3..108222d 100644
> > --- a/src/glsl/nir/glsl_to_nir.cpp
> > +++ b/src/glsl/nir/glsl_to_nir.cpp
> > @@ -326,8 +326,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 62cdbd4..d97db68 100644
> > --- a/src/glsl/nir/nir.h
> > +++ b/src/glsl/nir/nir.h
> > @@ -278,6 +278,7 @@ typedef struct {
> > * - Fragment shader output: one of the values from \c
> > gl_frag_result.
> > * - Uniforms: Per-stage uniform slot number for default uniform
> > block.
> > * - Uniforms: Index within the uniform block definition for UBO
> > members.
> > + * - Atomic Counter: Uniform slot number.
> > * - Other: This field is not currently used.
> > *
> > * If the variable is a uniform, shader input, or shader output,
> > and the
> > @@ -292,7 +293,9 @@ typedef struct {
> > unsigned int driver_location;
> >
> > /**
> > - * output index for dual source blending.
> > + * Output index for dual source blending.
> > + *
> > + * For atomic counters this stores the atomic buffer index.
> > */
> > int index;
> >
> > @@ -307,7 +310,6 @@ typedef struct {
> > * Location an atomic counter is stored at.
> > */
> > struct {
> > - unsigned buffer_index;
> > unsigned offset;
> > } atomic;
> >
> > diff --git a/src/glsl/nir/nir_lower_atomics.c
> > b/src/glsl/nir/nir_lower_atomics.c
> > index ce3615a..93db3a9 100644
> > --- a/src/glsl/nir/nir_lower_atomics.c
> > +++ b/src/glsl/nir/nir_lower_atomics.c
> > @@ -63,7 +63,7 @@ lower_instr(nir_intrinsic_instr *instr,
> > nir_function_impl *impl)
> >
> > 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;
> > + (int) instr->variables[0]->var->data.location;
> >
> > 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;
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > index a6eee47..5844c7c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > @@ -2400,7 +2400,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);
> > + location->data.index);
> >
> > /* Calculate the surface offset */
> > src_reg offset(this, glsl_type::uint_type);
>
More information about the mesa-dev
mailing list