[Mesa-dev] [PATCH V4 5/6] glsl: add helper for calculating offsets for struct members
Jason Ekstrand
jason at jlekstrand.net
Tue Sep 15 22:02:28 PDT 2015
On Tue, Sep 15, 2015 at 9:42 PM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> 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.
RIght... That makes more sense. I had missed the fact that the offset
we're building and the location are really disconnected things. In
that case, the record_location_offset function does make sense as a
general thing and probably belongs in glsl_type. Wrapping it all up,
here's what I'd like to see
1) Please add a comment to record_location_offset() saying that it
applies to uniform storage and maybe something about why you handle
structs and arrays.
2) You've convinced me that it makes sense and that it really is a
GLSL IR thing; leave it glsl_type.
3) If you're going to leave it in the glsl_type, you might as well
leave in the same form as it was in the v4 (That fits better with GLSL
IR style).
In other words, V4 of this patch (with the requested comment added) is
Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
You can also have my R-B on V5 of patch 6 assuming you've removed
record_location_offset (since it's getting moved back into glsl_type).
Does that all make sense?
Thanks for working on this and thanks for your patience as I reviewed.
My knowledge of how we do uniforms in GLSL IR isn't that great. Given
my experience reviewing this, I don't know that I really want it to
get any better...
Happy Pushing!
--Jason
> 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