<div dir="ltr">On 23 January 2013 21:19, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On 01/23/2013 10:07 PM, Paul Berry wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
On 22 January 2013 00:52, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a><br></div><div class="im">
<mailto:<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>>> wrote:<br>
<br>
From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a><br></div>
<mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.<u></u>com</a>>><div class="im"><br>
<br>
For variables that are in an interface block or are an instance of an<br>
interface block, this is the GLSL_TYPE_INTERFACE type for that block.<br>
<br>
Convert the ir_variable::is_in_uniform_<u></u>block method added in the<br>
previous commit to use this field instead of ir_variable::uniform_block.<br>
<br>
Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a><br></div>
<mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.<u></u>com</a>>><div><div class="h5"><br>
---<br>
src/glsl/ast_to_hir.cpp | 2 ++<br>
src/glsl/ir.h | 9 ++++++++-<br>
src/glsl/ir_clone.cpp | 2 ++<br>
3 files changed, 12 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp<br>
index a740a3c..47a1ae0 100644<br>
--- a/src/glsl/ast_to_hir.cpp<br>
+++ b/src/glsl/ast_to_hir.cpp<br>
@@ -4254,6 +4254,7 @@ ast_uniform_block::hir(exec_<u></u>list *instructions,<br>
this->instance_name,<br>
ir_var_uniform);<br>
<br>
+ var->interface_type = block_type;<br>
state->symbols->add_variable(<u></u>var);<br>
instructions->push_tail(var);<br>
} else {<br>
@@ -4263,6 +4264,7 @@ ast_uniform_block::hir(exec_<u></u>list *instructions,<br>
ralloc_strdup(state,<br>
fields[i].name),<br>
ir_var_uniform);<br>
var->uniform_block = ubo - state->uniform_blocks;<br>
+ var->interface_type = block_type;<br>
<br>
state->symbols->add_variable(<u></u>var);<br>
instructions->push_tail(var);<br>
diff --git a/src/glsl/ir.h b/src/glsl/ir.h<br>
index a7eb9c1..49c5f8d 100644<br>
--- a/src/glsl/ir.h<br>
+++ b/src/glsl/ir.h<br>
@@ -352,7 +352,7 @@ public:<br>
*/<br>
inline bool is_in_uniform_block() const<br>
{<br>
- return this->mode == ir_var_uniform && this->uniform_block != -1;<br>
+ return this->mode == ir_var_uniform && this->interface_type<br>
!= NULL;<br>
}<br>
<br>
/**<br>
@@ -538,6 +538,13 @@ public:<br>
* objects.<br>
*/<br>
ir_constant *constant_initializer;<br>
+<br>
+ /**<br>
+ * interface<br>
+ *<br>
+ * \sa ir_variable::location<br>
+ */<br>
+ const glsl_type *interface_type;<br>
<br>
<br>
This comment doesn't really help me understand what this field means.<br>
<br>
How about putting in this text (from the commit message):<br>
<br>
For variables that are in an interface block or are an instance of an<br>
interface block, this is the GLSL_TYPE_INTERFACE type for that block.<br>
<br>
};<br>
<br>
<br>
diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp<br>
index 3e22f2d..c221a96 100644<br>
--- a/src/glsl/ir_clone.cpp<br>
+++ b/src/glsl/ir_clone.cpp<br>
@@ -77,6 +77,8 @@ ir_variable::clone(void *mem_ctx, struct<br>
hash_table *ht) const<br>
var->constant_initializer =<br>
this->constant_initializer-><u></u>clone(mem_ctx, ht);<br>
<br>
+ var->interface_type = this->interface_type;<br>
+<br>
if (ht) {<br>
hash_table_insert(ht, var, (void *)const_cast<ir_variable<br>
*>(this));<br>
}<br>
--<br>
1.7.11.7<br>
<br>
______________________________<u></u>_________________<br>
mesa-dev mailing list<br></div></div>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.<u></u>freedesktop.org</a>><div class="im">
<br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/<u></u>mailman/listinfo/mesa-dev</a><br>
<br>
<br>
I'm not certain whether it's strictly necessary, but I would feel much<br>
more confident in this patch if we also initialized interface_type to<br>
NULL in the ir_variable constructor.<br>
</div></blockquote>
<br>
We're (somewhat intentionally) missing many NULL initializers because our placement new uses rzalloc. Is it okay for me to continue that trend? :)<br></blockquote><div><br></div><div style>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.</div>
<div style><br></div><div style>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:</div><div style><br></div><div style>
- 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.</div>
<div style><br></div><div style>- 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.<br>
</div><div style><br></div><div style>- 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.</div>
<div style><br></div><div style>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.</div>
<div style><br></div><div style>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.</div>
<div style><br></div><div style>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.</div></div></div>
</div>