<div dir="ltr"><span style="font-size:13px;font-family:arial,sans-serif">Hi,</span><div style="font-size:13px;font-family:arial,sans-serif">There was a lot of code everywhere that used iterators so it is not obvious for a new person that the iterators are evil. And they do abstract the implementation of the storage.</div>
<div style="font-size:13px;font-family:arial,sans-serif">Also, I'd rather use ir_instruction and as_*() calls instead of exec_node and explicit cast.</div><div style="font-size:13px;font-family:arial,sans-serif">I am not sure about MAX2, is it guaranteed to be converted to branchless conditional assignment?</div>
<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 27, 2013 at 8:26 AM, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 08/21/2013 05:57 PM, Dominik Behr wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Fixes a bug where if an uniform array is passed to a function the accesses<br>
to the array are not propagated so later all but the first vector of the<br>
uniform array are removed in parcel_out_uniform_storage resulting in<br>
broken shaders and out of bounds access to arrays in<br>
brw::vec4_visitor::pack_<u></u>uniform_registers.<br>
<br>
Signed-off-by: Dominik Behr <<a href="mailto:dbehr@chromium.org" target="_blank">dbehr@chromium.org</a>><br>
---<br>
src/glsl/link_functions.cpp | 26 ++++++++++++++++++++++++++<br>
1 file changed, 26 insertions(+)<br>
<br>
diff --git a/src/glsl/link_functions.cpp b/src/glsl/link_functions.cpp<br>
index 6b3e154..32e2012 100644<br>
--- a/src/glsl/link_functions.cpp<br>
+++ b/src/glsl/link_functions.cpp<br>
@@ -173,6 +173,32 @@ public:<br>
return visit_continue;<br>
}<br>
<br>
+ virtual ir_visitor_status visit_leave(ir_call *ir)<br>
+ {<br>
+ /* traverse list of function parameters, and for array parameters<br>
+ propagate max_array_access, do it when leaving the node<br>
+ so the childen would propagate their array accesses first */<br>
</blockquote></div>
children<br>
<br>
I also think this comment should be expanded. On the first read of this code, it wasn't obvious to me why this was necessary. It's worth mentioning that the important case is when the reference to the array at the call site does not reference a specific element. In that case the access range of the from the callee is propagated back to the array passed in.<div class="im">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ exec_list_iterator sig_param_iter = ir->callee->parameters.<u></u>iterator();<br>
+ exec_list_iterator param_iter = ir->actual_parameters.<u></u>iterator();<br>
</blockquote>
<br></div>
While iterators are a fine design pattern, the implementation of them in the compiler front-end is garbage (and I made that implementation). We stopped writing new code to use the iterators years ago. There is other code that does a similar double-walk of the signature parameters and actual parameters lists.<br>
<br>
I think the best example is in lower_clip_distance_visitor::<u></u>visit_leave(ir_call *ir) in lower_clip_distance.cpp.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ while (param_iter.has_next()) {<br>
+ ir_variable *sig_param = (ir_variable *) sig_param_iter.get();<br>
+ ir_rvalue *param = (ir_rvalue *) param_iter.get();<br>
+ assert(sig_param);<br>
+ assert(param);<br>
+ if (sig_param->type->is_array()) {<br>
+ ir_dereference_variable *deref = param->as_dereference_<u></u>variable();<br>
+ if (deref && deref->var && deref->var->type->is_array()) {<br>
+ if (sig_param->max_array_access > deref->var->max_array_access) {<br>
+ deref->var->max_array_access = sig_param->max_array_access;<br>
+ }<br>
</blockquote>
<br></div>
This should end up looking more similar to the code in call_link_visitor::visit(ir_<u></u>dereference_variable *ir) around line 200 (without your patch).<br>
<br>
var->max_array_access =<br>
MAX2(var->max_array_access, ir->var->max_array_access);<div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br>
+ sig_param_iter.next();<br>
+ param_iter.next();<br>
+ }<br>
+ return visit_continue;<br>
+ }<br>
+<br>
virtual ir_visitor_status visit(ir_dereference_variable *ir)<br>
{<br>
if (hash_table_find(locals, ir->var) == NULL) {<br>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br></div>