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 <apinhe...@igalia.com> > > Cc: Ian Romanick <i...@freedesktop.org> > > Cc: 10.4, 10.5 <mesa-sta...@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); > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
