<div dir="ltr">On 20 November 2013 11:26, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>On 11/19/2013 05:55 PM, Paul Berry wrote: > From section 7.1 (Built-In Language Variables) of the GLSL 4.10 > spec: > > Also, if a built-in interface block is redeclared, no member of > the built-in declaration can be redeclared outside the block > redeclaration. > > We have been regarding this text as a clarification to the behaviour > established for gl_PerVertex by GLSL 1.50, so we apply it regardless > of GLSL version. > > This patch enforces the rule by adding a boolean to ir_variable to </div>But it's not a boolean. :) Tri-state? </blockquote><div> </div><div>Oops, sorry. This comment was left over from a previous (failed) attempt to implement the correct functionality. I've changed this paragraph to: This patch enforces the rule by adding an enum to ir_variable to track how the variable was declared: implicitly, normally, or in an interface block. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> > track how the variable was declared: implicitly, normally, or in an > interface block. > > Fixes piglit tests: > - gs-redeclares-pervertex-out-after-global-redeclaration.geom > - vs-redeclares-pervertex-out-after-global-redeclaration.vert > - gs-redeclares-pervertex-out-after-other-global-redeclaration.geom > - vs-redeclares-pervertex-out-after-other-global-redeclaration.vert > - gs-redeclares-pervertex-out-before-global-redeclaration > - vs-redeclares-pervertex-out-before-global-redeclaration > > Cc: "10.0" <<a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a>> > > v2: Don't set "how_declared" redundantly in builtin_variables.cpp. > Properly clone "how_declared". > --- > src/glsl/ast_to_hir.cpp | 20 ++++++++++++++++++++ > src/glsl/builtin_variables.cpp | 1 + > src/glsl/ir.cpp | 3 ++- > src/glsl/ir.h | 36 ++++++++++++++++++++++++++++++++++++ > src/glsl/ir_clone.cpp | 1 + > 5 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp > index 76b256c..0128047 100644 > --- a/src/glsl/ast_to_hir.cpp > +++ b/src/glsl/ast_to_hir.cpp > @@ -3355,6 +3355,15 @@ ast_declarator_list::hir(exec_list *instructions, > ir_variable *earlier = > get_variable_being_redeclared(var, decl->get_location(), state, > false /* allow_all_redeclarations */); > + if (earlier != NULL) { > + if (strncmp(var->name, "gl_", 3) == 0 && > + earlier->how_declared == ir_var_declared_in_block) { > + _mesa_glsl_error(&loc, state, > + "`%s' has already been redeclared using " > + "gl_PerVertex", var->name); > + } > + earlier->how_declared = ir_var_declared_normally; > + } > > if (decl->initializer != NULL) { > result = process_initializer((earlier == NULL) ? var : earlier, > @@ -5048,6 +5057,7 @@ ast_interface_block::hir(exec_list *instructions, > _mesa_glsl_error(&loc, state, "`%s' redeclared", > this->instance_name); > } > + earlier->how_declared = ir_var_declared_normally; </div></div>Is this correct? At this point redeclaring_per_vertex is true, so it seems like this should be ir_var_declared_in_block. </blockquote><div> </div><div>It's correct as is. This block of code handles the case where a named interface block (i.e. the gl_PerVertex interface block representing geometry shader inputs) is being redeclared. In this case, the ir_variable represents the entire interface block, so it is ir_var_declared_normally (see the comment below: "Note: an ir_variable that represents a named interface block uses ir_var_declared_normally."). </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> > earlier->type = var->type; > earlier->reinit_interface_type(block_type); > delete var; > @@ -5078,7 +5088,11 @@ ast_interface_block::hir(exec_list *instructions, > _mesa_glsl_error(&loc, state, > "redeclaration of gl_PerVertex can only " > "include built-in variables"); > + } else if (earlier->how_declared == ir_var_declared_normally) { > + _mesa_glsl_error(&loc, state, > + "`%s' has already been redeclared", var->name); > } else { > + earlier->how_declared = ir_var_declared_in_block; > earlier->reinit_interface_type(block_type); > } > continue; > @@ -5125,6 +5139,12 @@ ast_interface_block::hir(exec_list *instructions, > if (var != NULL && > var->get_interface_type() == earlier_per_vertex && > var->mode == var_mode) { > + if (var->how_declared == ir_var_declared_normally) { > + _mesa_glsl_error(&loc, state, > + "redeclaration of gl_PerVertex cannot " > + "follow a redeclaration of `%s'", > + var->name); > + } > state->symbols->disable_variable(var->name); > var->remove(); > } > diff --git a/src/glsl/builtin_variables.cpp b/src/glsl/builtin_variables.cpp > index 4d44104..d57324c 100644 > --- a/src/glsl/builtin_variables.cpp > +++ b/src/glsl/builtin_variables.cpp > @@ -434,6 +434,7 @@ builtin_variable_generator::add_variable(const char *name, > enum ir_variable_mode mode, int slot) > { > ir_variable *var = new(symtab) ir_variable(type, name, mode); > + var->how_declared = ir_var_declared_implicitly; > > switch (var->mode) { > case ir_var_auto: > diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp > index 1b49736..ffff297 100644 > --- a/src/glsl/ir.cpp > +++ b/src/glsl/ir.cpp > @@ -1586,7 +1586,8 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name, > ir_variable_mode mode) > : max_array_access(0), max_ifc_array_access(NULL), > read_only(false), centroid(false), invariant(false), > - mode(mode), interpolation(INTERP_QUALIFIER_NONE), atomic() > + how_declared(ir_var_declared_normally), mode(mode), > + interpolation(INTERP_QUALIFIER_NONE), atomic() > { > this->ir_type = ir_type_variable; > this->type = type; > diff --git a/src/glsl/ir.h b/src/glsl/ir.h > index 7859702..4f775da 100644 > --- a/src/glsl/ir.h > +++ b/src/glsl/ir.h > @@ -294,6 +294,34 @@ enum ir_variable_mode { > }; > > /** > + * Enum keeping track of how a variable was declared. For error checking of > + * the gl_PerVertex redeclaration rules. > + */ > +enum ir_var_declaration_type { > + /** > + * Normal declaration (for most variables, this means an explicit > + * declaration. Exception: temporaries are always implicitly declared, but > + * they still use ir_var_declared_normally). > + * > + * Note: an ir_variable that represents a named interface block uses > + * ir_var_declared_normally. > + */ > + ir_var_declared_normally = 0, > + > + /** > + * Variable was explicitly declared (or re-declared) in an unnamed > + * interface block. > + */ > + ir_var_declared_in_block, > + > + /** > + * Variable is an implicitly declared built-in that has not been explicitly > + * re-declared by the shader. > + */ > + ir_var_declared_implicitly, > +}; > + > +/** > * \brief Layout qualifiers for gl_FragDepth. > * > * The AMD/ARB_conservative_depth extensions allow gl_FragDepth to be redeclared > @@ -526,6 +554,14 @@ public: > unsigned assigned:1; > > /** > + * Enum indicating how the variable was declared. See > + * ir_var_declaration_type. > + * > + * This is used to detect certain kinds of illegal variable redeclarations. > + */ > + unsigned how_declared:2; > + > + /** > * Storage class of the variable. > * > * \sa ir_variable_mode > diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp > index b0f173a..40ed33a 100644 > --- a/src/glsl/ir_clone.cpp > +++ b/src/glsl/ir_clone.cpp > @@ -68,6 +68,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht) const > var->has_initializer = this->has_initializer; > var->depth_layout = this->depth_layout; > var->assigned = this->assigned; > + var->how_declared = this->how_declared; > var->used = this->used; > > var->num_state_slots = this->num_state_slots; > </div></div></blockquote></div> </div></div>