[Mesa-stable] [PATCH V2] glsl: fix atomic buffer index for bindings other than 0
Alejandro Piñeiro
apinheiro at igalia.com
Sat Jul 25 09:11:47 PDT 2015
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
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);
--
Alejandro Piñeiro (apinheiro at igalia.com)
More information about the mesa-stable
mailing list