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

[Eric Anholt]

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.

> 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.
> +       */
> +      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:
> -- 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110721/7d8cec46/attachment.pgp>