-----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" <i...@freedesktop.org> > wrote: >> From: Ian Romanick <ian.d.roman...@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----- _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev