[Mesa-dev] [PATCH v2] glsl: Prohibit illegal mixing of redeclarations inside/outside gl_PerVertex.

Paul Berry stereotype441 at gmail.com
Wed Nov 20 13:41:21 PST 2013


On 20 November 2013 11:26, Ian Romanick <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>
> >
> > 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.").


>
> >           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;
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131120/b73f43cf/attachment-0001.html>


More information about the mesa-dev mailing list