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

[Paul Berry]

On 23 January 2013 21:19, Ian Romanick <idr at freedesktop.org> wrote:

> 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 <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 <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 <mesa-dev at lists.freedesktop.org>>
>>
>>     http://lists.freedesktop.org/**mailman/listinfo/mesa-dev<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? :)
>

If I've learned anything from working on the Mesa codebase over the last
year and a half, it's that you, Ian, don't need my permission to make
judgement calls about what's best for the project :).  And if I ever start
to feel like giving (or withdrawing) permission, it's a sign that I'm
getting too big for my britches and am probably heading for a smackdown.
 So I'm going to defer to your opinion on whether it's okay to continue
that trend.  A lot of people trust and respect your judgement on what's
best for the project, including me.

Having said that, my personal opinion is that it would be to our benefit to
reverse the trend, and I urge you to consider it.  Here's my rationale:

- It's an unusual practice for C++ projects.  Developers who come to the
Mesa code base from elsewhere in the software industry are likely to be
confused and surprised by it.  I had about 10 years of C++ development
experience before I started working on Mesa, and I definitely found it
confusing and surprising when I first encountered it.

- It doesn't play well with static analysis tools like Coverity.  Coverity
reports a defect for every class member that isn't initialized either in
the constructor or in one of the functions it calls.  Since this practice
is currently so widespread in Mesa, we pretty much have to ignore every
Coverity defect of this sort, and that unfortunately means we lose out on
opportunities for Coverity to help us catch bugs.

- It's not a practice we can safely apply uniformly, because not all of our
classes get allocated using rzalloc.  Some of them get allocated on the
stack (e.g. most of our visitor classes), and for those classes, we have no
choice but to initialize class members.  Some classes get heap-allocated
sometimes and stack-allocated other times (exec_list comes to mind)--we
have to initialize members of those classes too, but if we forget, we might
not discover the bug for a while because the stack-allocated usages might
be rare.  Personally, I'd rather get in the habit of initializing class
members everywhere rather than have to base the decision on how I think the
class is going to get allocated, and run the risk of making the wrong call.

Having said that, obviously ir_variable is allocated with rzalloc, because
it's part of the IR tree, and if I'd spent a little more time thinking
before I spoke up I probably would have realized that and not said
anything.  So if you don't want to take my suggestion in this particular
case, I'm not going to lose any sleep.  Clearly your code works as written.

And in case you're worried, I don't have any fantasy about digging through
the Mesa source and adding NULL initializers where we don't already have
them.  At best it would be a waste of time, and at worst it would run the
risk of creating a performance regression by adding unnecessary
zero-initialization code to a hot path.  I also don't think we should go
around creating constructors for plain old structs that are used in a C
fashion.  The code as it exists is fine and we have way better things to do.

But when we add new classes, or add new fields to existing classes (as we
do in this patch), IMHO it's worth the small amount of additional effort to
add initializers.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130123/9ef13ac0/attachment-0001.html>