[Mesa-stable] [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-stable mailing list