[Mesa-dev] [PATCH 4/4] linker: Check that initializers for global variables match
Paul Berry
stereotype441 at gmail.com
Tue Nov 1 12:10:38 PDT 2011
On 31 October 2011 18:07, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This requires tracking a couple extra fields in ir_variable:
>
> * A flag to indicate that a variable had an initializer.
>
> * For non-const variables, a field to track the constant value of the
> variable's initializer.
>
> For variables non-constant initalizers, ir_variable::has_initializer
> will be true, but ir_variable::constant_initializer will be NULL. The
> linker can use the values of these fields to check adherence to the
> GLSL 4.20 rules for shared global variables:
>
> "If a shared global has multiple initializers, the initializers
> must all be constant expressions, and they must all have the same
> value. Otherwise, a link error will result. (A shared global
> having only one initializer does not require that initializer to
> be a constant expression.)"
>
> Previous to 4.20 the GLSL spec simply said that initializers must have
> the same value. In this case of non-constant initializers, this was
> impossible to determine. As a result, no vendor actually implemented
> that behavior. The 4.20 behavior matches the behavior of NVIDIA's
> shipping implementations.
>
> NOTE: This is candidate for the 7.11 branch. This patch also needs
> the preceding patch "glsl: Refactor generate_ARB_draw_buffers_variables
> to use add_builtin_constant"
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34687
>
I have a few concerns about this patch, which we talked about in person
today. To summarize for the list:
- There's a "finishme" comment just above the last hunk that I think we
should really address. The test "glsl-link-initializers-03" is trying to
demonstrate that problem, but it has bugs. I'll submit an improved
"glsl-link-initializers-03" to the list.
- I think these changes regress propagation of uniform initializers. In
other words, if one shader initializes a uniform and the other doesn't,
then depending on the order in which the shaders are linked, the
initializer might be lost.
That said, this is clearly an improvement over the previous state of the
code, and I would be ok with accepting the patches and then doing some
follow-up fixes.
> ---
> src/glsl/ast_to_hir.cpp | 3 ++
> src/glsl/ir.cpp | 5 ++++
> src/glsl/ir.h | 18 +++++++++++++++++
> src/glsl/ir_clone.cpp | 5 ++++
> src/glsl/ir_validate.cpp | 7 ++++++
> src/glsl/ir_variable.cpp | 2 +
> src/glsl/linker.cpp | 48
> +++++++++++++++++++++++++++++++++++++++------
> 7 files changed, 81 insertions(+), 7 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 7584fdf..bd9c33d 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2351,6 +2351,9 @@ process_initializer(ir_variable *var,
> ast_declaration *decl,
> } else
> initializer_type = rhs->type;
>
> + var->constant_initializer = rhs->constant_expression_value();
> + var->has_initializer = true;
> +
> /* If the declared variable is an unsized array, it must inherrit
> * its full type from the initializer. A declaration such as
> *
> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
> index ef7300e..a5eca5a 100644
> --- a/src/glsl/ir.cpp
> +++ b/src/glsl/ir.cpp
> @@ -1326,9 +1326,11 @@ ir_variable::ir_variable(const struct glsl_type
> *type, const char *name,
> this->type = type;
> this->name = ralloc_strdup(this, name);
> this->explicit_location = false;
> + this->has_initializer = false;
> this->location = -1;
> this->warn_extension = NULL;
> this->constant_value = NULL;
> + this->constant_initializer = NULL;
> this->origin_upper_left = false;
> this->pixel_center_integer = false;
> this->depth_layout = ir_depth_layout_none;
> @@ -1489,6 +1491,9 @@ steal_memory(ir_instruction *ir, void *new_ctx)
> if (var != NULL && var->constant_value != NULL)
> steal_memory(var->constant_value, ir);
>
> + if (var != NULL && var->constant_initializer != NULL)
> + steal_memory(var->constant_initializer, ir);
> +
> /* The components of aggregate constants are not visited by the normal
> * visitor, so steal their values by hand.
> */
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index abbf455..5878c05 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -365,6 +365,14 @@ public:
> unsigned explicit_location:1;
>
> /**
> + * Does this variable have an initializer?
> + *
> + * This is used by the linker to cross-validiate initializers of global
> + * variables.
> + */
> + unsigned has_initializer:1;
> +
> + /**
> * \brief Layout qualifier for gl_FragDepth.
> *
> * This is not equal to \c ir_depth_layout_none if and only if this
> @@ -414,6 +422,16 @@ public:
> * Value assigned in the initializer of a variable declared "const"
> */
> ir_constant *constant_value;
> +
> + /**
> + * Constant expression assigned in the initializer of the variable
> + *
> + * \warning
> + * This field and \c ::constant_value are distinct. Even if the two
> fields
> + * refer to constants with the same value, they must point to separate
> + * objects.
> + */
> + ir_constant *constant_initializer;
> };
>
>
> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
> index 9adf470..e8ac9fb 100644
> --- a/src/glsl/ir_clone.cpp
> +++ b/src/glsl/ir_clone.cpp
> @@ -50,6 +50,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht)
> const
> var->origin_upper_left = this->origin_upper_left;
> var->pixel_center_integer = this->pixel_center_integer;
> var->explicit_location = this->explicit_location;
> + var->has_initializer = this->has_initializer;
>
> var->num_state_slots = this->num_state_slots;
> if (this->state_slots) {
> @@ -68,6 +69,10 @@ ir_variable::clone(void *mem_ctx, struct hash_table
> *ht) const
> if (this->constant_value)
> var->constant_value = this->constant_value->clone(mem_ctx, ht);
>
> + if (this->constant_initializer)
> + var->constant_initializer =
> + this->constant_initializer->clone(mem_ctx, ht);
> +
> if (ht) {
> hash_table_insert(ht, var, (void *)const_cast<ir_variable *>(this));
> }
> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
> index c387ecb..a352012 100644
> --- a/src/glsl/ir_validate.cpp
> +++ b/src/glsl/ir_validate.cpp
> @@ -496,6 +496,13 @@ ir_validate::visit(ir_variable *ir)
> }
> }
>
> + if (ir->constant_initializer != NULL && !ir->has_initializer) {
> + printf("ir_variable didn't have an initializer, but has a constant "
> + "initializer value.\n");
> + ir->print();
> + abort();
> + }
> +
> return visit_continue;
> }
>
> diff --git a/src/glsl/ir_variable.cpp b/src/glsl/ir_variable.cpp
> index bea0b2b..e719abe 100644
> --- a/src/glsl/ir_variable.cpp
> +++ b/src/glsl/ir_variable.cpp
> @@ -408,6 +408,8 @@ add_builtin_constant(exec_list *instructions,
> glsl_symbol_table *symtab,
> name, glsl_type::int_type,
> ir_var_auto, -1);
> var->constant_value = new(var) ir_constant(value);
> + var->constant_initializer = new(var) ir_constant(value);
> + var->has_initializer = true;
> return var;
> }
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index a595c9c..915d5bb 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -448,17 +448,30 @@ cross_validate_globals(struct gl_shader_program
> *prog,
> }
> }
>
> - /* FINISHME: Handle non-constant initializers.
> + /* Page 35 (page 41 of the PDF) of the GLSL 4.20 spec says:
> + *
> + * "If a shared global has multiple initializers, the
> + * initializers must all be constant expressions, and they
> + * must all have the same value. Otherwise, a link error
> will
> + * result. (A shared global having only one initializer
> does
> + * not require that initializer to be a constant
> expression.)"
> + *
> + * Previous to 4.20 the GLSL spec simply said that initializers
> + * must have the same value. In this case of non-constant
> + * initializers, this was impossible to determine. As a
> result,
> + * no vendor actually implemented that behavior. The 4.20
> + * behavior matches the implemented behavior of at least one
> other
> + * vendor, so we'll implement that for all GLSL versions.
> */
> - if (var->constant_value != NULL) {
> - if (existing->constant_value != NULL) {
> - if
> (!var->constant_value->has_value(existing->constant_value)) {
> + if (var->constant_initializer != NULL) {
> + if (existing->constant_initializer != NULL) {
> + if
> (!var->constant_initializer->has_value(existing->constant_initializer)) {
> linker_error(prog, "initializers for %s "
> "`%s' have differing values\n",
> mode_string(var), var->name);
> return false;
> }
> - } else
> + } else {
> /* If the first-seen instance of a particular uniform did
> not
> * have an initializer but a later instance does, copy the
> * initializer to the version stored in the symbol table.
> @@ -471,8 +484,29 @@ cross_validate_globals(struct gl_shader_program *prog,
> * FINISHME: modify the shader, and linking with the
> second
> * FINISHME: will fail.
> */
> - existing->constant_value =
> - var->constant_value->clone(ralloc_parent(existing),
> NULL);
> + existing->constant_initializer =
> +
> var->constant_initializer->clone(ralloc_parent(existing),
> + NULL);
> + }
> + }
> +
> + if (var->has_initializer) {
> + if (existing->has_initializer
> + && (var->constant_initializer == NULL
> + || existing->constant_initializer == NULL)) {
> + linker_error(prog,
> + "shared global variable `%s' has multiple "
> + "non-constant initializers.\n",
> + var->name);
> + return false;
> + }
> +
> + /* Some instance had an initializer, so keep track of that.
> In
> + * this location, all sorts of initializers (constant or
> + * otherwise) will propagate the existence to the variable
> + * stored in the symbol table.
> + */
> + existing->has_initializer = true;
> }
>
> if (existing->invariant != var->invariant) {
> --
> 1.7.6.4
>
> _______________________________________________
> 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/20111101/757f3b3d/attachment.htm>
More information about the mesa-dev
mailing list