[Mesa-dev] [PATCH V4 5/6] glsl: add helper for calculating offsets for struct members

Timothy Arceri t_arceri at yahoo.com.au
Tue Sep 15 21:42:29 PDT 2015


On Tue, 2015-09-15 at 19:21 -0700, Jason Ekstrand wrote:
> On Tue, Sep 15, 2015 at 5:18 PM, Timothy Arceri <
> t_arceri at yahoo.com.au> wrote:
> > On Tue, 2015-09-15 at 11:51 -0700, Jason Ekstrand wrote:
> > > On Tue, Sep 15, 2015 at 12:51 AM, Timothy Arceri <
> > > t_arceri at yahoo.com.au> wrote:
> > > > From: Timothy <t_arceri at yahoo.com.au>
> > > > 
> > > > Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> > > > ---
> > > >  src/glsl/glsl_types.cpp | 20 ++++++++++++++++++++
> > > >  src/glsl/glsl_types.h   |  7 +++++++
> > > >  2 files changed, 27 insertions(+)
> > > > 
> > > > diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> > > > index 755618a..38b1660 100644
> > > > --- a/src/glsl/glsl_types.cpp
> > > > +++ b/src/glsl/glsl_types.cpp
> > > > @@ -1040,6 +1040,26 @@ glsl_type::component_slots() const
> > > >  }
> > > > 
> > > >  unsigned
> > > > +glsl_type::record_location_offset(unsigned length) const
> > > > +{
> > > > +   unsigned offset = 0;
> > > > +   const glsl_type *t = this->without_array();
> > > > +   if (t->is_record()) {
> > > > +      for (unsigned i = 0; i < length; i++) {
> > > > +         const glsl_type *st = t->fields.structure[i].type;
> > > > +         const glsl_type *wa = st->without_array();
> > > > +         if (wa->is_record()) {
> > > > +            unsigned r_offset = wa->record_location_offset(wa
> > > > ->length);
> > > > +            offset += st->is_array() ? st->length * r_offset :
> > > > r_offset;
> > > > +         } else {
> > > > +            offset += 1;
> > > > +         }
> > > > +      }
> > > > +   }
> > > > +   return offset;
> > > > +}
> > > 
> > > I'm not sure how I feel about having this as a general thin in
> > > glsl_type.  It's really just for samplers, so I think I'd rather
> > > just
> > > add it as a helper in nir_lower_samplers.
> > 
> > Sure I can see what you mean I guess I put it here for non-nir
> > drivers.
> > Also there is also no reason this needs to be sampler specific
> > (although there are obiviously no uses elsewhere currently).
> > 
> > I'll move it.
> > 
> > > 
> > > Also, IMHO, this would be easier to read if it were written more
> > > like
> > > this:
> > > 
> > > unsigned
> > > get_struct_field_offset(glsl_type *type, unsigned length)
> > > {
> > >    switch (type->base_type) {
> > >    case SAMPLER:
> > >       return 1;
> > >    case ARRAY:
> > >       return get_struct_field_offset(type->array_type,
> > > type->array_type->length) * type->array_length;
> > >    case STRUCT: {
> > >       unsigned offset = 0;
> > >       for (stuff) {
> > >          offset += get_struct_field_offset(field_type, field_type
> > > ->length)
> > >       }
> > >    default:
> > >       return 0;
> > >    }
> > > }
> > > 
> > > The way you wrote it is very GLSL IR style which is fine for what
> > > it
> > > is.  I simply find explicitly listing all the cases easier to
> > > follow
> > > than the without_array() stuff.
> > 
> > I see what you mean but the code above won't work the way we want
> > it
> > to. We are getting the offset for the uniform not the sampler, so
> > we do
> > care about and want to count all members not just samplers.
> > 
> > Also we only want to add arrays to the count if its an array of a
> > struct (or array of array but I will add that support later) as
> > arrays
> > of other members only take up a single uniform location, this is
> > why
> > without_array() is important. I'll have a think about a better way
> > to
> > implement this but I'm not sure the alternative will be much
> > better.
> 
> I think one or both of us is confused about something.  The whole
> point of nir_lower_samplers is to take a deref and turn it into an
> index into the flattened array of samplers.  The backed takes the
> resulting index, adds some fixed offset, and that's the index into
> the
> hardware binding table.  The other uniforms in the shader don't count
> into this at all.
> 
> Also, I'm not sure what you mean by "uniform location".  Yes, we have
> a UniformStorage structure and each of the samplers has some data
> there.  However, we should be looking at the .sampler field of that
> which isn't just a generic uniform index.  Could you please explain
> what you mean?


Sure. To get the right .sampler field we first need to lookup the right
uniform in UniformStorage. That's what this offset calculation is
about, this location (or uniform slot id as its called in some
comments) is what was previously looked up in the hash table where we
used to generate a string as the key.

We could have stuck to generating a string but it seems wasteful to
carry around a string of every uniform just to look-up this slot id.
I'm working towards dropping the UniformHash table, I think we could
already free this earlier as its only used at compile time now rather
than runtime since I landed some clean-up patches for
program_interface_query.



> --Jason
> 
> > > 
> > > Also, note the default case in my example.  When we're figuring
> > > out
> > > where we want to put samplers, we don't want to count in struct
> > > members that aren't samplers.
> > 
> > As above we what the uniform index here not the sampler index.
> > 
> > >  Again, this is why this is a
> > > sampler-specific thing and should go in nir_lower_samplers.
> > > --Jason
> > > 
> > > > +
> > > > +unsigned
> > > >  glsl_type::uniform_locations() const
> > > >  {
> > > >     unsigned size = 0;
> > > > diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> > > > index 02a398f..8ee8968 100644
> > > > --- a/src/glsl/glsl_types.h
> > > > +++ b/src/glsl/glsl_types.h
> > > > @@ -292,6 +292,13 @@ struct glsl_type {
> > > >     unsigned component_slots() const;
> > > > 
> > > >     /**
> > > > +    * Calculate offset between the base location and this
> > > > struct
> > > > member.
> > > > +    * For the initial call length is the index of the member
> > > > to
> > > > find the
> > > > +    * offset for.
> > > > +    */
> > > > +   unsigned record_location_offset(unsigned length) const;
> > > > +
> > > > +   /**
> > > >      * Calculate the number of unique values from
> > > > glGetUniformLocation for the
> > > >      * elements of the type.
> > > >      *
> > > > --
> > > > 2.4.3
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list