[Mesa-dev] [PATCH] glsl: avoid calling base_alignment when samplers are involved

[Ian Romanick]

On 03/23/2015 05:00 AM, Ilia Mirkin wrote:
> Earlier commit 53bf7c8fd2e changed the logic to always call
> base_alignment on structs. 1ec715ce8b12 hacked the function to return 0
> for sampler fields, but didn't handle sampler arrays. Instead of
> extending the hack, avoid calling base_alignment in the first place on
> non-UBO uniforms.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89726
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> 
> No regressions in a piglit run. This seems like the right thing to do
> rather than further hacking base_alignment. I was in "fix it now!"
> mode when I wrote the original logic, getting pretty frustrated at the
> various UBO rules and the number of different places they were being
> implemented inside mesa.
> 
> However I think that for uniforms, this is the one and only place. As
> a side-benefit, this means that uniforms will be better packed in the
> presence of structs. And this will hopefully fix a lot of
> "glsl_align(0) might happen, waaaaa" errors from static checkers.
> 
>  src/glsl/glsl_types.cpp    | 9 ---------
>  src/glsl/link_uniforms.cpp | 4 ++++
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> index 38b37a6..be87b0a 100644
> --- a/src/glsl/glsl_types.cpp
> +++ b/src/glsl/glsl_types.cpp
> @@ -1077,15 +1077,6 @@ glsl_type::std140_base_alignment(bool row_major) const
>        return base_alignment;
>     }
>  
> -   /* A sampler may never occur in a UBO (without bindless of some sort),
> -    * however it is convenient to use this alignment function even with
> -    * regular uniforms. This allows use of this function on uniform structs
> -    * that contain samplers.
> -    */
> -   if (this->is_sampler()) {
> -      return 0;
> -   }
> -
>     assert(!"not reached");
>     return -1;

I will also accept a follow-up patch that replaces this with
unreachable(). :)

>  }
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 799c74b..59adc29 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -547,6 +547,8 @@ private:
>     virtual void enter_record(const glsl_type *type, const char *name,
>                               bool row_major) {
>        assert(type->is_record());
> +      if (this->ubo_block_index == -1)
> +         return;

Hmm... would deleting the 'else process(var)' block from
parcel_out_uniform_storage::set_and_process achieve the same result?

>        this->ubo_byte_offset = glsl_align(
>              this->ubo_byte_offset, type->std140_base_alignment(row_major));
>     }
> @@ -554,6 +556,8 @@ private:
>     virtual void leave_record(const glsl_type *type, const char *name,
>                               bool row_major) {
>        assert(type->is_record());
> +      if (this->ubo_block_index == -1)
> +         return;
>        this->ubo_byte_offset = glsl_align(
>              this->ubo_byte_offset, type->std140_base_alignment(row_major));
>     }
>