On 31 October 2011 18:07, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> 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 <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=34687" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=34687</a> </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> --- 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>