[Mesa-dev] [PATCH 1/3] glsl: Rebalance expression trees that are reduction operations.

[Ian Romanick]

On 06/09/2014 02:11 PM, Matt Turner wrote:
> +/* Note that this function does not attempt to recognize that reduction trees
> + * are already balanced.
> + */
> +static void
> +is_reduction(ir_instruction *ir, void *data)
> +{
> +   struct is_reduction_data *ird = (struct is_reduction_data *)data;
> +   if (!ird->is_reduction)
> +      return;
> +
> +   /* We don't want to balance a tree that contains multiple constants, since
> +    * we'll be able to constant fold them if they're not in separate subtrees.
> +    */
> +   if (ir->as_constant()) {
> +      if (ird->contains_constant) {
> +         ird->is_reduction = false;
> +      }

Because of the way visit_tree works, I think this would produce a
false-positive for an expression like foo[2] + 3.  Eventually
is_reduction will get called for both the 2 and the 3.

> +      ird->contains_constant = true;
> +      return;
> +   }
> +
> +   ir_expression *expr = ir->as_expression();
> +   if (!expr)
> +      return;
> +
> +   /* Non-constant matrices might still contain constant vec4 that we can
> +    * constant fold once split up. Handling matrices will need some more
> +    * work.
> +    */
> +   if (expr->type->is_matrix()) {
> +      ird->is_reduction = false;
> +      return;
> +   }
> +
> +   if (ird->type != NULL && ird->type != expr->type) {
> +      ird->is_reduction = false;
> +      return;
> +   }
> +   ird->type = expr->type;
> +
> +   ird->num_expr++;
> +   if (is_reduction_operation(expr->operation)) {

This (and, actually, the type check above might get tricked by
expressions like a+foo[b%c]+d+e.

I think you might need something like visit_tree that just doesn't try
to navigate past any sort of ir_dereference (or a version that allows
the predicate function to halt navigation of subtrees).

> +      if (ird->operation != 0 && ird->operation != expr->operation)
> +         ird->is_reduction = false;
> +      ird->operation = expr->operation;
> +   } else {
> +      ird->is_reduction = false;
> +   }
> +}