[Mesa-stable] [Mesa-dev] [PATCH v2] glsl: Prohibit illegal mixing of redeclarations inside/outside gl_PerVertex.
Ian Romanick
idr at freedesktop.org
Wed Nov 20 16:01:32 PST 2013
On 11/20/2013 01:41 PM, Paul Berry wrote:
> On 20 November 2013 11:26, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> 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
>
> But it's not a boolean. :) Tri-state?
>
> 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.
>
> > 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" <mesa-stable at lists.freedesktop.org
> <mailto:mesa-stable at lists.freedesktop.org>>
> >
> > 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;
>
> Is this correct? At this point redeclaring_per_vertex is true, so it
> seems like this should be ir_var_declared_in_block.
>
>
> 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.").
Okay. That makes sense.
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> > 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;
> >
>
>
More information about the mesa-stable
mailing list