[Mesa-dev] Mesa (shader-work): glsl: teach loop analysis that array dereferences are bounds on the index

[Luca Barbieri]

> Clever. :)
>
> Technically speaking only the access to the array has undefined
> behavior, and that undefined behavior does not include program
> termination.

No, my interpretation is correct, because ARB_robustness contains the
following text:
<<
    ... Undefined behavior results from indexing an array with a
    non-constant expression that's greater than or equal to the array's
    size or less than 0. If robust buffer access is enabled via the
    OpenGL API, such indexing must not result in abnormal program
    termination. The results are still undefined, but implementations
    are encouraged to produce zero values for such accesses. Only
    one-dimensional arrays may be declared. ...
>>

Hence, abnormal program termination and undefined results are both
allowed in the normal (non-robustness) case, and thus it's obvious
that loop termination is OK too.

> I'm a bit nervous / suspicious that there may be some app
> out there that relies on the loop running too many times.

However, at least the nVidia Cg compiler seems to refuse to unroll
this, so caution may indeed be advisable.

>  I wonder if
> there should be a flag to disable this particular optimization.
Yes, so that we can support ARB_robustness.
We'll probably want an environment variable to force ARB_robustness on
even if the user did not ask for it.

>> +loop_analysis::visit_leave(ir_dereference_array *ir)
>> +{
>> +   /* If we're not somewhere inside a loop, there's nothing to do.
>> +    */
>> +   if (this->state.is_empty())
>> +      return visit_continue_with_parent;
>
> I fixed this bug in loop_analysis::visit(ir_dereference_variable *)
> earlier today.  See commit 956f049f.

Thanks.

Are the other places in the file that return visit_continue_with_parent correct?

> How about:
>
>   assert(ir->array_index->type->is_integer());
>   ir_constant *const max_index_c =
>      (ir->array_index->type->base_type == GLSL_TYPE_UINT)
>        ? new(ir) ir_constant((unsigned)max_index)
>        : new(ir) ir_constant((int)max_index);
>   ir_constant *const zero_c =
>      ir_constant::zero(ir, ir->array_index->type);

Yes, much better.

>> +   bound_if = new (ir) ir_if(new(ir) ir_expression(ir_binop_greater, ir->array_index->type, ir->array_index, max_index_c));
>
> Shouldn't this be ir_binop_gequal?

No, max_index is array_length - 1.

Not sure whether it's better to set it to array_length and use gequal instead.

>
>> +   bound_if->self_link();
>
> This is weird.  Why not insert the new instructions at the top of the
> loop?

Well, if I actually insert them, then they will end up in the
resulting Mesa IR code, which is a pessimization.

> It seems like it would be better just store the minimum and
> maximum "allowed" value for the variable in its entry in loop_variable.

I tried this at first, but it is much more complex, and also doesn't allow
to automagically support array[2 * i + 1], array[int(f)], and so on.

Instead with this approach, as the general code gains such support, it
will automatically apply to array bounds too.

>  This ought to simplify the follow-on patches as well.  I think the key
> observation is that this trick only works on variable identified as loop
> induction variables.

I don't understand what you mean here.

> Everywhere else in the compiler puts the { and } on the same line as the
> if or the else.

OK, will try to follow that.

Thanks for the comments.