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

[Jason Ekstrand]

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?
--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