[Mesa-dev] [PATCH 11/13] glsl: Treat ir_dereference_array of non-var as a constant for lowering

[Ian Romanick]

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

On 07/21/2011 05:41 PM, Eric Anholt wrote:
> On Thu, 21 Jul 2011 12:16:56 -0700, "Ian Romanick" <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Perviously the code would just look at deref->array->type to see if it
>> was a constant.  This isn't good enough because deref->array might be
>> another ir_dereference_array... of a constant.  As a result,
>> deref->array->type wouldn't be a constant, but
>> deref->variable_referenced() would return NULL.  The unchecked NULL
>> pointer would shortly lead to a segfault.
>>
>> Instead just look at the return of deref->variable_referenced().  If
>> it's NULL, assume that either a constant or some other form of
>> anonymous temporary storage is being dereferenced.
>>
>> This is a bit hinkey because most drivers treat constant arrays as
>> uniforms, but the lowering pass treats them as temporaries.  This
>> keeps the behavior of the old code, so this change isn't making things
>> worse.
> 
> I'd emphasize in the message that this commit is about fixing behavior
> when array->variable_referenced() is NULL more than about the special
> case of constants.  I ratholed in the review thinking about crazy deref
> chains and trying to justify the special case of constants to myself,
> when this is just about "what's the default when we don't know what kind
> of variable it is?"
> 
> If we were to talk about the choice of what we want when we see
> constants (uniform vs temp), we'd probably also want to talk about what
> the right answer for this variable_referenced() == NULL is, since
> there's risk of running this pass before copy propagation, and therefore
> choosing the ->lower_temps behavior instead of ->lower_uniforms behavior
> because there was some silly copy introduced somewhere in the user's
> program or in one of our other passes.

Ugh.  That's a good point.  Something like

uniform mat4 mvp;
void main()
{
    mat4 x = mvp;

    ... x[i] ...
}

Copy propagation would eliminate `x' altogether, and thus would change
the lowering necessary for that non-constant index.

In fairness, we have that problem anyway.  We get off a little easy here
because I think pretty much all architectures have lower_temp ==
lower_uniform.  The other case would be shader inputs, and lower_input
may not be the same as lower_temp.  Ugh.

I think this means there are cases where we may not generate correct
code.  All architectures that can do non-constant indexing of things
other temps can also do non-constant index of temps.  We may not decide
to lower a temp, then we could copy propagate an input.  Now we have a
non-constant index of an input that shouldn't exist.

>> diff --git a/src/glsl/lower_variable_index_to_cond_assign.cpp b/src/glsl/lower_variable_index_to_cond_assign.cpp
>> index e08ec13..79fa58e 100644
>> --- a/src/glsl/lower_variable_index_to_cond_assign.cpp
>> +++ b/src/glsl/lower_variable_index_to_cond_assign.cpp
>> @@ -321,10 +321,16 @@ public:
>>  
>>     bool storage_type_needs_lowering(ir_dereference_array *deref) const
>>     {
>> -      if (deref->array->ir_type == ir_type_constant)
>> +      /* If a variable isn't eventually the target of this dereference, then
>> +       * it must be a constant or some sort of anonymous temporary storage.
>> +       *
>> +       * FINISHME: Is this correct?  Most drivers treat arrays of constants as
>> +       * FINISHME: uniforms.  It seems like this should do the same.
>> +       */

How about changing this to:

/* If a variable isn't eventually the target of this dereference, then
 * it must be a constant or some sort of anonymous temporary storage.
 * Since we don't really know what it is (yet), we'll make what is (on
 * most hardware) the least conservative guess.  We'll guess that it
 * needs to be lowered like a temp.
 */

>> +      const ir_variable *const var = deref->array->variable_referenced();
>> +      if (var == NULL)
>>  	 return this->lower_temps;
>>  
>> -      const ir_variable *const var = deref->array->variable_referenced();
>>        switch (var->mode) {
>>        case ir_var_auto:
>>        case ir_var_temporary:
>> -- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4pqVQACgkQX1gOwKyEAw/wEQCggxjAf9XArX9l8+UqJXkoUE+g
tBMAoIgQd+UJfAPJuumz5hjgFWRzEZLV
=sOO+
-----END PGP SIGNATURE-----