[Mesa-dev] [PATCH v5 08/29] glsl: fix uniform linking logic in the presence of structs
Ilia Mirkin
imirkin at alum.mit.edu
Mon Feb 16 12:01:34 PST 2015
On Tue, Feb 10, 2015 at 6:58 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> Add a enter/leave record callback so that the offset may be aligned to
> the proper value. Otherwise only leaf fields are called, and the first
> field needs to be aligned to the outer struct's base alignment while the
> last field needs to be aligned to the inner struct's base alignment.
>
> This removes most usage of the last field/record type values passed into
> visit_field.
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> I kept this separate from the other changes since it seemed self-contained.
> I can fold somewhere else if desired.
>
> src/glsl/link_uniform_blocks.cpp | 45 ++++++++++++++++++----------------------
> src/glsl/link_uniforms.cpp | 37 +++++++++++++++++++++++++++------
> src/glsl/linker.h | 6 ++++++
> 3 files changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/src/glsl/link_uniform_blocks.cpp b/src/glsl/link_uniform_blocks.cpp
> index f5fc502..1c175ec 100644
> --- a/src/glsl/link_uniform_blocks.cpp
> +++ b/src/glsl/link_uniform_blocks.cpp
> @@ -67,6 +67,25 @@ private:
> assert(!"Should not get here.");
> }
>
> + virtual void enter_record(const glsl_type *type, const char *name,
> + bool row_major) {
> + this->offset = glsl_align(
> + this->offset, type->std140_base_alignment(row_major));
> + }
> +
> + virtual void leave_record(const glsl_type *type, const char *name,
> + bool row_major) {
> + /* If this is the last field of a structure, apply rule #9. The
> + * GL_ARB_uniform_buffer_object spec says:
> + *
> + * "The structure may have padding at the end; the base offset of
> + * the member following the sub-structure is rounded up to the next
> + * multiple of the base alignment of the structure."
> + */
> + this->offset = glsl_align(
> + this->offset, type->std140_base_alignment(row_major));
> + }
> +
> virtual void visit_field(const glsl_type *type, const char *name,
> bool row_major, const glsl_type *record_type,
> bool last_field)
> @@ -97,27 +116,13 @@ private:
> v->IndexName = v->Name;
> }
>
> - const unsigned alignment = record_type
> - ? record_type->std140_base_alignment(v->RowMajor)
> - : type->std140_base_alignment(v->RowMajor);
> + const unsigned alignment = type->std140_base_alignment(v->RowMajor);
> unsigned size = type->std140_size(v->RowMajor);
>
> this->offset = glsl_align(this->offset, alignment);
> v->Offset = this->offset;
>
> - /* If this is the last field of a structure, apply rule #9. The
> - * GL_ARB_uniform_buffer_object spec says:
> - *
> - * "The structure may have padding at the end; the base offset of
> - * the member following the sub-structure is rounded up to the next
> - * multiple of the base alignment of the structure."
> - *
> - * last_field won't be set if this is the last field of a UBO that is
> - * not a named instance.
> - */
> this->offset += size;
> - if (last_field)
> - this->offset = glsl_align(this->offset, 16);
>
> /* From the GL_ARB_uniform_buffer_object spec:
> *
> @@ -131,16 +136,6 @@ private:
> */
> this->buffer_size = glsl_align(this->offset, 16);
> }
> -
> - virtual void visit_field(const glsl_struct_field *field)
> - {
> - /* FINISHME: When support for doubles (dvec4, etc.) is added to the
> - * FINISHME: compiler, this may be incorrect for a structure in a UBO
> - * FINISHME: like struct s { struct { float f } s1; dvec4 v; };.
> - */
> - this->offset = glsl_align(this->offset,
> - field->type->std140_base_alignment(false));
> - }
> };
>
> class count_block_size : public program_resource_visitor {
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 3aa6e0a..347f079 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -169,6 +169,9 @@ program_resource_visitor::recursion(const glsl_type *t, char **name,
> if (record_type == NULL && t->is_record())
> record_type = t;
>
> + if (t->is_record())
> + this->enter_record(t, *name, row_major);
> +
> for (unsigned i = 0; i < t->length; i++) {
> const char *field = t->fields.structure[i].name;
> size_t new_length = name_length;
> @@ -208,6 +211,11 @@ program_resource_visitor::recursion(const glsl_type *t, char **name,
> */
> record_type = NULL;
> }
> +
> + if (t->is_record()) {
> + (*name)[name_length] = '\0';
> + this->leave_record(t, *name, row_major);
> + }
> } else if (t->is_array() && (t->fields.array->is_record()
> || t->fields.array->is_interface())) {
> if (record_type == NULL && t->fields.array->is_record())
> @@ -249,6 +257,16 @@ program_resource_visitor::visit_field(const glsl_struct_field *field)
> /* empty */
> }
>
> +void
> +program_resource_visitor::enter_record(const glsl_type *, const char *, bool)
> +{
> +}
> +
> +void
> +program_resource_visitor::leave_record(const glsl_type *, const char *, bool)
> +{
> +}
> +
> namespace {
>
> /**
> @@ -526,6 +544,18 @@ private:
> assert(!"Should not get here.");
> }
>
> + virtual void enter_record(const glsl_type *type, const char *name,
> + bool row_major) {
> + this->ubo_byte_offset = glsl_align(
> + this->ubo_byte_offset, type->std140_base_alignment(row_major));
> + }
> +
> + virtual void leave_record(const glsl_type *type, const char *name,
> + bool row_major) {
> + this->ubo_byte_offset = glsl_align(
> + this->ubo_byte_offset, type->std140_base_alignment(row_major));
> + }
> +
> virtual void visit_field(const glsl_type *type, const char *name,
> bool row_major, const glsl_type *record_type,
> bool last_field)
> @@ -590,16 +620,11 @@ private:
> if (this->ubo_block_index != -1) {
> this->uniforms[id].block_index = this->ubo_block_index;
>
> - const unsigned alignment = record_type
> - ? record_type->std140_base_alignment(row_major)
> - : type->std140_base_alignment(row_major);
> + const unsigned alignment = type->std140_base_alignment(row_major);
> this->ubo_byte_offset = glsl_align(this->ubo_byte_offset, alignment);
> this->uniforms[id].offset = this->ubo_byte_offset;
> this->ubo_byte_offset += type->std140_size(row_major);
>
> - if (last_field)
> - this->ubo_byte_offset = glsl_align(this->ubo_byte_offset, 16);
> -
> if (type->is_array()) {
> this->uniforms[id].array_stride =
> glsl_align(type->fields.array->std140_size(row_major), 16);
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index 6ee5858..be4da5e 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -170,6 +170,12 @@ protected:
> */
> virtual void visit_field(const glsl_struct_field *field);
>
> + virtual void enter_record(const glsl_type *type, const char *name,
> + bool row_major);
> +
> + virtual void leave_record(const glsl_type *type, const char *name,
> + bool row_major);
> +
> private:
> /**
> * \param name_length Length of the current name \b not including the
> --
> 2.0.5
>
I realized that we now also need this hunk, since
std140_base_alignment is being called on regular uniform structs too
now, which may contain samplers. I've added this as a separate patch
in my tree. Not sure if it should be this->is_sampler() ||
this->is_image() || atomic uint instead. (Perhaps a is_opaque() helper
is in order.)
--- a/src/glsl/glsl_types.cpp
+++ b/src/glsl/glsl_types.cpp
@@ -1077,6 +1077,15 @@ 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;
}
More information about the mesa-dev
mailing list