On Tue, 2015-09-15 at 22:02 -0700, Jason Ekstrand wrote:
> On Tue, Sep 15, 2015 at 9:42 PM, Timothy Arceri <
> t_arc...@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_arc...@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_arc...@yahoo.com.au> wrote:
> > > > > > From: Timothy <t_arc...@yahoo.com.au>
> > > > > >
> > > > > > Cc: Jason Ekstrand <jason.ekstr...@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.ekstr...@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?
Makes sense :)
>
> Thanks for working on this and thanks for your patience as I
> reviewed.
Not a problem thanks for taking the time to review, I don't mind
explaining my patches its a good sanity check.
> 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...
hehe
>
> 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@lists.freedesktop.org
> > > > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
