[Mesa-dev] [PATCH 12/32] glsl: Add ir_variable::interface_type field

[Ian Romanick]

On 01/23/2013 10:07 PM, Paul Berry wrote:
> On 22 January 2013 00:52, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
>     From: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>
>
>     For variables that are in an interface block or are an instance of an
>     interface block, this is the GLSL_TYPE_INTERFACE type for that block.
>
>     Convert the ir_variable::is_in_uniform_block method added in the
>     previous commit to use this field instead of ir_variable::uniform_block.
>
>     Signed-off-by: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>
>     ---
>       src/glsl/ast_to_hir.cpp | 2 ++
>       src/glsl/ir.h           | 9 ++++++++-
>       src/glsl/ir_clone.cpp   | 2 ++
>       3 files changed, 12 insertions(+), 1 deletion(-)
>
>     diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>     index a740a3c..47a1ae0 100644
>     --- a/src/glsl/ast_to_hir.cpp
>     +++ b/src/glsl/ast_to_hir.cpp
>     @@ -4254,6 +4254,7 @@ ast_uniform_block::hir(exec_list *instructions,
>                                                       this->instance_name,
>                                                       ir_var_uniform);
>
>     +      var->interface_type = block_type;
>             state->symbols->add_variable(var);
>             instructions->push_tail(var);
>          } else {
>     @@ -4263,6 +4264,7 @@ ast_uniform_block::hir(exec_list *instructions,
>                                          ralloc_strdup(state,
>     fields[i].name),
>                                          ir_var_uniform);
>                var->uniform_block = ubo - state->uniform_blocks;
>     +         var->interface_type = block_type;
>
>                state->symbols->add_variable(var);
>                instructions->push_tail(var);
>     diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>     index a7eb9c1..49c5f8d 100644
>     --- a/src/glsl/ir.h
>     +++ b/src/glsl/ir.h
>     @@ -352,7 +352,7 @@ public:
>           */
>          inline bool is_in_uniform_block() const
>          {
>     -      return this->mode == ir_var_uniform && this->uniform_block != -1;
>     +      return this->mode == ir_var_uniform && this->interface_type
>     != NULL;
>          }
>
>          /**
>     @@ -538,6 +538,13 @@ public:
>           * objects.
>           */
>          ir_constant *constant_initializer;
>     +
>     +   /**
>     +    * interface
>     +    *
>     +    * \sa ir_variable::location
>     +    */
>     +   const glsl_type *interface_type;
>
>
> This comment doesn't really help me understand what this field means.
>
> How about putting in this text (from the commit message):
>
> For variables that are in an interface block or are an instance of an
> interface block, this is the GLSL_TYPE_INTERFACE type for that block.
>
>       };
>
>
>     diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
>     index 3e22f2d..c221a96 100644
>     --- a/src/glsl/ir_clone.cpp
>     +++ b/src/glsl/ir_clone.cpp
>     @@ -77,6 +77,8 @@ ir_variable::clone(void *mem_ctx, struct
>     hash_table *ht) const
>             var->constant_initializer =
>               this->constant_initializer->clone(mem_ctx, ht);
>
>     +   var->interface_type = this->interface_type;
>     +
>          if (ht) {
>             hash_table_insert(ht, var, (void *)const_cast<ir_variable
>     *>(this));
>          }
>     --
>     1.7.11.7
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> I'm not certain whether it's strictly necessary, but I would feel much
> more confident in this patch if we also initialized interface_type to
> NULL in the ir_variable constructor.

We're (somewhat intentionally) missing many NULL initializers because 
our placement new uses rzalloc.  Is it okay for me to continue that 
trend? :)

> With those two changes, this patch is:
>
> Reviewed-by: Paul Berry <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>>