[Mesa-dev] [PATCH 2/4] glsl: store uniform slot id in var location field

[Timothy Arceri]

On Sun, 2015-08-30 at 20:54 -0700, Jason Ekstrand wrote:
> 
> On Aug 30, 2015 3:44 AM, "Timothy Arceri" <t_arceri at yahoo.com.au> wrote:
> >
> > This will allow us to access the uniform later on without resorting to
> > building a name string and looking it up in UniformHash.
> > ---
> >  src/glsl/ir.h              |  2 ++
> >  src/glsl/link_uniforms.cpp | 21 ++++++++++++++++++---
> >  src/glsl/nir/nir.h         |  2 ++
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> > index ede8caa..9bcaf28 100644
> > --- a/src/glsl/ir.h
> > +++ b/src/glsl/ir.h
> > @@ -819,6 +819,8 @@ 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.
> > +       *   - Opaque Uniforms: explicit binding until linking then reused 
> to
> > +       *     store 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_uniforms.cpp b/src/glsl/link_uniforms.cpp
> > index 8bf5ef9..c28f13d 100644
> > --- a/src/glsl/link_uniforms.cpp
> > +++ b/src/glsl/link_uniforms.cpp
> > @@ -458,6 +458,7 @@ public:
> >     {
> >        current_var = var;
> >        field_counter = 0;
> > +      this->explicit_binding = current_var->data.location;
> >        this->record_next_sampler = new string_to_uint_map;
> >
> >        ubo_block_index = -1;
> > @@ -510,12 +511,18 @@ public:
> >                      var->get_interface_type()->name);
> >           else
> >              process(var);
> > -      } else
> > +      } else {
> > +         /* Reset data location so we can reuse this variable for storing 
> the
> > +          * uniform slot of opaque types.
> > +          */
> > +         current_var->data.location = -1;
> > +
> Why are we doing this in the else case and not where we save off the value? 
> It seems like it will be easier to read if either a) only use the save and 
> use the saved value for opaque things or b) always save and use the saved 
> copy.  In particular, it looks like we wipe this for non-opaque non-UBO 
> uniforms without ever setting it back.  I guess that may be OK because IIRC, 
> you can't set locations on them.  However, it would still be good to make 
> the logic match up better.  Am I making any sense?

I had them together at the top originally but this caused issues for UBOs as
they actually use the location value, so thats just where it ended up.

Would you be happy with something like below:

current_var = var;
field_counter = 0;
this->record_next_sampler = new string_to_uint_map;

this->explicit_binding = current_var->data.location;
if (current_var->type->contains_opaque()) {
   /* Reset data location so we can reuse this variable for storing the
    * uniform slot of opaque types.
    */
   current_var->data.location = -1;
}


> >           process(var);
> >        }
> >        delete this->record_next_sampler;
> >     }
> >
> > +
> Unneeded whitespace change.

Grr. I knew about this I keep forgetting to fix it. Will do for next version
:)

> >     int ubo_block_index;
> >     int ubo_byte_offset;
> >     gl_shader_stage shader_type;
> > @@ -697,16 +704,23 @@ private:
> >           return;
> >        }
> >
> > +      /* For array of arrays or struct arrays the base location may have
> > +       * already been set so dont set it again.
> > +       */
> > +      if (base_type->contains_opaque() && current_var->data.location == 
> -1) {
> > +          current_var->data.location = id;
> > +      }
> > +
> >        /* Assign explicit locations. */
> >        if (current_var->data.explicit_location) {
> >           /* Set sequential locations for struct fields. */
> >           if (record_type != NULL) {
> >              const unsigned entries = MAX2(1, this
> ->uniforms[id].array_elements);
> >              this->uniforms[id].remap_location =
> > -               current_var->data.location + field_counter;
> > +               this->explicit_binding + field_counter;
> >              field_counter += entries;
> >           } else {
> > -            this->uniforms[id].remap_location = current_var
> ->data.location;
> > +            this->uniforms[id].remap_location = this->explicit_binding;
> >           }
> >        } else {
> >           /* Initialize to to indicate that no location is set */
> > @@ -771,6 +785,7 @@ private:
> >     unsigned next_sampler;
> >     unsigned next_image;
> >     unsigned next_subroutine;
> > +   int explicit_binding;
> Please put this after current_var in the private section and document that 
> it applies to the current_var.
> >
> >     /* Stores total struct array elements including nested structs */
> >     unsigned sampler_count;
> > diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
> > index 9703372..a051af1 100644
> > --- a/src/glsl/nir/nir.h
> > +++ b/src/glsl/nir/nir.h
> > @@ -278,6 +278,8 @@ 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.
> > +       *   - Opaque Uniforms: explicit binding until linking then reused 
> to
> > +       *     store uniform slot number.
> No need to mention linking here. We don't do linking in NIR at this point 
> and there's no guarantee that we'll do it the same way if/when we do.
> >         *   - Other: This field is not currently used.
> >         *
> >         * If the variable is a uniform, shader input, or shader output, 
> and the
> > --
> > 2.4.3
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev