[Mesa-dev] [PATCH 2/4] glsl: store uniform slot id in var location field
Timothy Arceri
t_arceri at yahoo.com.au
Sun Aug 30 22:45:47 PDT 2015
On Mon, 2015-08-31 at 15:21 +1000, Timothy Arceri wrote:
> 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;
> }
Although this is still a little confusing because it means the location is
only reset/set for structs when they contain an opaque type.
Maybe option b) would be better, leaving this code as is and reset the
location for all non-UBO uniforms later on.
if (!current_var->is_in_buffer_block() &&
current_var->data.location == -1) {
current_var->data.location = id;
}
I think I like this, it seems more consitent. What do you think?
>
>
> > > 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
> _______________________________________________
> 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