[Mesa-dev] [PATCH 2/4] glsl: store uniform slot id in var location field

[Jason Ekstrand]

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?

>           process(var);
>        }
>        delete this->record_next_sampler;
>     }
>
> +

Unneeded whitespace change.

>     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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150830/48a17802/attachment.html>