[Mesa-dev] [PATCH] squash! glsl: Optimize min/max expression trees
Petri Latvala
petri.latvala at intel.com
Mon Aug 18 06:14:55 PDT 2014
On 08/14/2014 07:04 AM, Matt Turner wrote:
> ---
> I'd squash this in at minimum. The changes are
>
> - Whitespace
> - Removal of unnecessary destructor
> - Renaming "one" and "two" to "a" and "b" (one->value.u[c0] < two->value.u[c0]...)
> - continue -> break
> - assert(!...) -> unreachable
> - Not doing assignments in if conditionals
> - Marking swizzle_if_required as static
Thanks, I'll squash this in.
> I also think less_all_components should just return an enum like
> { MIXED, EQUAL, LESS, GREATER }, rather than setting a variable in
> the class. It, as well as smaller/larger_constant, can then be
> static functions outside of the visitor.
Yes, I'll try what it looks like with that.
>
> I think the algorithm itself looks correct.
>
> src/glsl/opt_minmax.cpp | 145 +++++++++++++++++++++---------------------------
> 1 file changed, 63 insertions(+), 82 deletions(-)
>
> diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
> index 5656059..b987386 100644
> --- a/src/glsl/opt_minmax.cpp
> +++ b/src/glsl/opt_minmax.cpp
> @@ -37,12 +37,10 @@
> #include "glsl_types.h"
> #include "main/macros.h"
>
> -namespace
> -{
> -class minmax_range
> -{
> -public:
> +namespace {
>
> +class minmax_range {
> +public:
> minmax_range(ir_constant *low = NULL, ir_constant *high = NULL)
> {
> range[0] = low;
> @@ -60,60 +58,45 @@ public:
> class ir_minmax_visitor : public ir_rvalue_enter_visitor {
> public:
> ir_minmax_visitor()
> - : progress(false)
> - , valid(true)
> - {
> - }
> -
> - virtual ~ir_minmax_visitor()
> + : progress(false), valid(true)
> {
> }
>
> - bool
> - less_all_components(ir_constant *one, ir_constant *two);
> -
> - ir_constant *
> - smaller_constant(ir_constant *one, ir_constant *two);
> -
> - ir_constant *
> - larger_constant(ir_constant *one, ir_constant *two);
> + bool less_all_components(ir_constant *a, ir_constant *b);
> + ir_constant *smaller_constant(ir_constant *a, ir_constant *b);
> + ir_constant *larger_constant(ir_constant *a, ir_constant *b);
>
> - minmax_range
> - combine_range(minmax_range r0, minmax_range r1, bool ismin);
> + minmax_range combine_range(minmax_range r0, minmax_range r1, bool ismin);
>
> - minmax_range
> - range_intersection(minmax_range r0, minmax_range r1);
> + minmax_range range_intersection(minmax_range r0, minmax_range r1);
>
> - minmax_range
> - get_range(ir_rvalue *rval);
> + minmax_range get_range(ir_rvalue *rval);
>
> - ir_rvalue *
> - prune_expression(ir_expression *expr, minmax_range baserange);
> + ir_rvalue *prune_expression(ir_expression *expr, minmax_range baserange);
>
> - void
> - handle_rvalue(ir_rvalue **rvalue);
> + void handle_rvalue(ir_rvalue **rvalue);
>
> bool progress;
> bool valid;
> };
>
> /*
> - * Returns true if all vector components of `one' are less than of `two'.
> + * Returns true if all vector components of `a' are less than of `b'.
> *
> * If there are vector components that are less while others are greater, the
> * visitor is marked invalid and no further changes will be made to the IR.
> */
> bool
> -ir_minmax_visitor::less_all_components(ir_constant *one, ir_constant *two)
> +ir_minmax_visitor::less_all_components(ir_constant *a, ir_constant *b)
> {
> - assert(one != NULL);
> - assert(two != NULL);
> + assert(a != NULL);
> + assert(b != NULL);
>
> - assert(one->type->base_type == two->type->base_type);
> + assert(a->type->base_type == b->type->base_type);
>
> - unsigned oneinc = one->type->is_scalar() ? 0 : 1;
> - unsigned twoinc = two->type->is_scalar() ? 0 : 1;
> - unsigned components = MAX2(one->type->components(), two->type->components());
> + unsigned a_inc = a->type->is_scalar() ? 0 : 1;
> + unsigned b_inc = b->type->is_scalar() ? 0 : 1;
> + unsigned components = MAX2(a->type->components(), b->type->components());
>
> /* No early escape. We need to go through all components and mark the
> * visitor as invalid if comparison yields less for some components and
> @@ -127,34 +110,34 @@ ir_minmax_visitor::less_all_components(ir_constant *one, ir_constant *two)
>
> for (unsigned i = 0, c0 = 0, c1 = 0;
> i < components;
> - c0 += oneinc, c1 += twoinc, ++i) {
> - switch (one->type->base_type) {
> + c0 += a_inc, c1 += b_inc, ++i) {
> + switch (a->type->base_type) {
> case GLSL_TYPE_UINT:
> - if (one->value.u[c0] < two->value.u[c1])
> + if (a->value.u[c0] < b->value.u[c1])
> foundless = true;
> - else if (one->value.u[c0] > two->value.u[c1])
> + else if (a->value.u[c0] > b->value.u[c1])
> foundgreater = true;
> else
> foundequal = true;
> - continue;
> + break;
> case GLSL_TYPE_INT:
> - if (one->value.i[c0] < two->value.i[c1])
> + if (a->value.i[c0] < b->value.i[c1])
> foundless = true;
> - else if (one->value.i[c0] > two->value.i[c1])
> + else if (a->value.i[c0] > b->value.i[c1])
> foundgreater = true;
> else
> foundequal = true;
> - continue;
> + break;
> case GLSL_TYPE_FLOAT:
> - if (one->value.f[c0] < two->value.f[c1])
> + if (a->value.f[c0] < b->value.f[c1])
> foundless = true;
> - else if (one->value.f[c0] > two->value.f[c1])
> + else if (a->value.f[c0] > b->value.f[c1])
> foundgreater = true;
> else
> foundequal = true;
> - continue;
> + break;
> default:
> - assert(!"not reached");
> + unreachable("not reached");
> }
> }
>
> @@ -170,27 +153,27 @@ ir_minmax_visitor::less_all_components(ir_constant *one, ir_constant *two)
> }
>
> ir_constant *
> -ir_minmax_visitor::smaller_constant(ir_constant *one, ir_constant *two)
> +ir_minmax_visitor::smaller_constant(ir_constant *a, ir_constant *b)
> {
> - assert(one != NULL);
> - assert(two != NULL);
> + assert(a != NULL);
> + assert(b != NULL);
>
> - if (less_all_components(one, two))
> - return one;
> + if (less_all_components(a, b))
> + return a;
> else
> - return two;
> + return b;
> }
>
> ir_constant *
> -ir_minmax_visitor::larger_constant(ir_constant *one, ir_constant *two)
> +ir_minmax_visitor::larger_constant(ir_constant *a, ir_constant *b)
> {
> - assert(one != NULL);
> - assert(two != NULL);
> + assert(a != NULL);
> + assert(b != NULL);
>
> - if (less_all_components(one, two))
> - return two;
> + if (less_all_components(a, b))
> + return b;
> else
> - return one;
> + return a;
> }
>
> /* Combines two ranges by doing an element-wise min() / max() depending on the
> @@ -250,17 +233,17 @@ ir_minmax_visitor::range_intersection(minmax_range r0, minmax_range r1)
> minmax_range
> ir_minmax_visitor::get_range(ir_rvalue *rval)
> {
> - if (ir_expression *expr = rval->as_expression()) {
> - if (expr->operation == ir_binop_min ||
> - expr->operation == ir_binop_max) {
> - minmax_range r0 = get_range(expr->operands[0]);
> - minmax_range r1 = get_range(expr->operands[1]);
> + ir_expression *expr = rval->as_expression();
> + if (expr && (expr->operation == ir_binop_min ||
> + expr->operation == ir_binop_max)) {
> + minmax_range r0 = get_range(expr->operands[0]);
> + minmax_range r1 = get_range(expr->operands[1]);
>
> - return combine_range(r0, r1, expr->operation == ir_binop_min);
> - }
> + return combine_range(r0, r1, expr->operation == ir_binop_min);
> }
>
> - if (ir_constant *c = rval->as_constant()) {
> + ir_constant *c = rval->as_constant();
> + if (c) {
> return minmax_range(c, c);
> }
>
> @@ -314,11 +297,10 @@ ir_minmax_visitor::prune_expression(ir_expression *expr, minmax_range baserange)
> progress = true;
>
> /* Recurse if necessary. */
> - if (ir_expression* opexpr = expr->operands[1 - i]->as_expression()) {
> - if (opexpr->operation == ir_binop_min ||
> - opexpr->operation == ir_binop_max) {
> - return prune_expression(opexpr, baserange);
> - }
> + ir_expression *op_expr = expr->operands[1 - i]->as_expression();
> + if (op_expr && (op_expr->operation == ir_binop_min ||
> + op_expr->operation == ir_binop_max)) {
> + return prune_expression(op_expr, baserange);
> }
>
> return expr->operands[1 - i];
> @@ -333,20 +315,19 @@ ir_minmax_visitor::prune_expression(ir_expression *expr, minmax_range baserange)
> unsigned clear = (ismin ? 0 : 1);
>
> for (unsigned i = 0; i < 2; ++i) {
> - if (ir_expression *opexpr = expr->operands[i]->as_expression()) {
> - if (opexpr->operation == ir_binop_min ||
> - opexpr->operation == ir_binop_max) {
> - limits[1 - i].range[clear] = NULL;
> - minmax_range base = range_intersection(limits[1 - i], baserange);
> - expr->operands[i] = prune_expression(opexpr, base);
> - }
> + ir_expression *op_expr = expr->operands[i]->as_expression();
> + if (op_expr && (op_expr->operation == ir_binop_min ||
> + op_expr->operation == ir_binop_max)) {
> + limits[1 - i].range[clear] = NULL;
> + minmax_range base = range_intersection(limits[1 - i], baserange);
> + expr->operands[i] = prune_expression(op_expr, base);
> }
> }
>
> return expr;
> }
>
> -ir_rvalue *
> +static ir_rvalue *
> swizzle_if_required(ir_expression *expr,
> ir_rvalue *rval)
> {
More information about the mesa-dev
mailing list