--- 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
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. 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) { -- 1.8.5.5 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev