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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Luca Barbieri wrote:
>> 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 think some of the danger lies in constructs like:

	for (int i = 0; i < limit; i++)
		if (i < gl_TexCoord.length())
			do_something_with(gl_TexCoord[i]);
		else
			do_something_else();

We need a bunch more tests.

>>  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?

I don't think so.

>> 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.

It probably doesn't matter one way or the other.  A comment would help
the next person not think it was a bug, though.

>>> +   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.

I'm a bit nervous about having IR floating around that is expected to be
kept but is not in the instruction stream.  My main concern is that a
round of talloc clean up will destroy these objects before they're
intended to be destroyed.  What is the intended lifetime of these
instructions?  It appears that it's the same as the loop_state object
(or the loop_variable_state objects).  If that's the case, one of those
should probably be used as the memory context instead of ir.

The way that these are flagged as being different is also strange.

>>  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.

I think I was looking at some of the patches out of order.  Never mind.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyH3PMACgkQX1gOwKyEAw+UMwCgkpZmd6zc8eVfdCyq35CYQuvP
AVcAoIiCWB0hQUcjpQjmDZU+8XDsc/rk
=yIw6
-----END PGP SIGNATURE-----