[Mesa-dev] [PATCH] glsl: propagate max_array_access through function calls

[Dominik Behr]

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 <idr at freedesktop.org> 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 <dbehr at chromium.org>
>> ---
>>   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) {
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130827/dacbd5cc/attachment.html>