Hi, 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. Also, I'd rather use ir_instruction and as_*() calls instead of exec_node and explicit cast. I am not sure about MAX2, is it guaranteed to be converted to branchless conditional assignment?
On Tue, Aug 27, 2013 at 8:26 AM, Ian Romanick <[email protected]> wrote: > On 08/21/2013 05:57 PM, Dominik Behr wrote: > >> Fixes a bug where if an uniform array is passed to a function the accesses >> to the array are not propagated so later all but the first vector of the >> uniform array are removed in parcel_out_uniform_storage resulting in >> broken shaders and out of bounds access to arrays in >> brw::vec4_visitor::pack_**uniform_registers. >> >> Signed-off-by: Dominik Behr <[email protected]> >> --- >> src/glsl/link_functions.cpp | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/src/glsl/link_functions.cpp b/src/glsl/link_functions.cpp >> index 6b3e154..32e2012 100644 >> --- a/src/glsl/link_functions.cpp >> +++ b/src/glsl/link_functions.cpp >> @@ -173,6 +173,32 @@ public: >> return visit_continue; >> } >> >> + virtual ir_visitor_status visit_leave(ir_call *ir) >> + { >> + /* traverse list of function parameters, and for array parameters >> + propagate max_array_access, do it when leaving the node >> + so the childen would propagate their array accesses first */ >> > children > > 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. > > > + exec_list_iterator sig_param_iter = ir->callee->parameters.** >> iterator(); >> + exec_list_iterator param_iter = ir->actual_parameters.** >> iterator(); >> > > 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. > > I think the best example is in > lower_clip_distance_visitor::**visit_leave(ir_call > *ir) in lower_clip_distance.cpp. > > > + while (param_iter.has_next()) { >> + ir_variable *sig_param = (ir_variable *) sig_param_iter.get(); >> + ir_rvalue *param = (ir_rvalue *) param_iter.get(); >> + assert(sig_param); >> + assert(param); >> + if (sig_param->type->is_array()) { >> + ir_dereference_variable *deref = param->as_dereference_** >> variable(); >> + if (deref && deref->var && deref->var->type->is_array()) { >> + if (sig_param->max_array_access > >> deref->var->max_array_access) { >> + deref->var->max_array_access = >> sig_param->max_array_access; >> + } >> > > This should end up looking more similar to the code in > call_link_visitor::visit(ir_**dereference_variable *ir) around line 200 > (without your patch). > > var->max_array_access = > MAX2(var->max_array_access, ir->var->max_array_access); > > > + } >> + } >> + sig_param_iter.next(); >> + param_iter.next(); >> + } >> + return visit_continue; >> + } >> + >> virtual ir_visitor_status visit(ir_dereference_variable *ir) >> { >> if (hash_table_find(locals, ir->var) == NULL) { >> >> >
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
