On 09/08/17 05:59, Thomas Helland wrote:
I'm not quite sure if the increase in size of the base ir class
versus the reduced overhead and added code is worth it?
There is more code yes, but most of that is asserts and validation code.
This should actually make the code more robust than it is currently.
Some numbers would be nice.
Yeah sorry should have included something.
The reduction in time is so small that it is not really measurable.
However callgrind was reporting this function as being called just under
34 million times while compiling the Deus Ex shaders (just pre-linking
was profiled) with 0.20% spent in this function.
IMO considering what the function does that it too high. The other IRs
avoid recalculating this type of thing by keeping this stuff in an array
of structs which is created only once and shared by all expressions.
We could do this here also which would remove the extra member as we
would have a pointer for ir_expression_operation and make it a struct
rather than an enum. However on a 64-bit system the pointer then negates
that saving so it wasn't worth it.
If we are worried about size here we should do something about the
operands array always having 4 elements. However I can at least change
num_operands to a uint8_t.
A comment below
2017-08-07 2:18 GMT+00:00 Timothy Arceri <tarc...@itsqueeze.com>:
Extra validation is added to ir_validate to make sure this is
always updated to the correct numer of operands, as passes like
lower_instructions modify the instructions directly rather then
generating a new one.
---
src/compiler/glsl/glsl_to_nir.cpp | 4 +--
src/compiler/glsl/ir.cpp | 20 +++++++++++++-
src/compiler/glsl/ir.h | 13 +++++++++
src/compiler/glsl/ir_builder_print_visitor.cpp | 8 +++---
src/compiler/glsl/ir_clone.cpp | 2 +-
src/compiler/glsl/ir_constant_expression.cpp | 2 +-
src/compiler/glsl/ir_equals.cpp | 2 +-
src/compiler/glsl/ir_hv_accept.cpp | 2 +-
src/compiler/glsl/ir_print_visitor.cpp | 2 +-
src/compiler/glsl/ir_rvalue_visitor.cpp | 2 +-
src/compiler/glsl/ir_validate.cpp | 8 ++++++
src/compiler/glsl/lower_instructions.cpp | 32 ++++++++++++++++++++++
src/compiler/glsl/lower_int64.cpp | 4 +--
src/compiler/glsl/lower_mat_op_to_vec.cpp | 8 +++---
src/compiler/glsl/lower_ubo_reference.cpp | 2 +-
.../glsl/lower_vec_index_to_cond_assign.cpp | 2 +-
src/compiler/glsl/lower_vector.cpp | 2 +-
src/compiler/glsl/opt_algebraic.cpp | 4 +--
src/compiler/glsl/opt_constant_folding.cpp | 2 +-
src/compiler/glsl/opt_tree_grafting.cpp | 2 +-
src/mesa/program/ir_to_mesa.cpp | 4 +--
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 6 ++--
22 files changed, 102 insertions(+), 31 deletions(-)
diff --git a/src/compiler/glsl/glsl_to_nir.cpp
b/src/compiler/glsl/glsl_to_nir.cpp
index 331438a183..e5166855e8 100644
--- a/src/compiler/glsl/glsl_to_nir.cpp
+++ b/src/compiler/glsl/glsl_to_nir.cpp
@@ -1487,25 +1487,25 @@ nir_visitor::visit(ir_expression *ir)
}
return;
}
default:
break;
}
nir_ssa_def *srcs[4];
- for (unsigned i = 0; i < ir->get_num_operands(); i++)
+ for (unsigned i = 0; i < ir->num_operands; i++)
srcs[i] = evaluate_rvalue(ir->operands[i]);
glsl_base_type types[4];
- for (unsigned i = 0; i < ir->get_num_operands(); i++)
+ for (unsigned i = 0; i < ir->num_operands; i++)
if (supports_ints)
types[i] = ir->operands[i]->type->base_type;
else
types[i] = GLSL_TYPE_FLOAT;
glsl_base_type out_type;
if (supports_ints)
out_type = ir->type->base_type;
else
out_type = GLSL_TYPE_FLOAT;
diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp
index 78889bd6d3..d501e19c01 100644
--- a/src/compiler/glsl/ir.cpp
+++ b/src/compiler/glsl/ir.cpp
@@ -196,38 +196,46 @@ ir_expression::ir_expression(int op, const struct
glsl_type *type,
ir_rvalue *op0, ir_rvalue *op1,
ir_rvalue *op2, ir_rvalue *op3)
: ir_rvalue(ir_type_expression)
{
this->type = type;
this->operation = ir_expression_operation(op);
this->operands[0] = op0;
this->operands[1] = op1;
this->operands[2] = op2;
this->operands[3] = op3;
+ init_num_operands();
+
#ifndef NDEBUG
- int num_operands = get_num_operands(this->operation);
for (int i = num_operands; i < 4; i++) {
assert(this->operands[i] == NULL);
}
+
+ for (unsigned i = 0; i < num_operands; i++) {
+ assert(this->operands[i] != NULL);
+ }
#endif
}
It would be nice if the upper for loop iterator used unsigned
for consistency with the variable and the other loop.
The same accounts for the other occurence further down.
But otherwise things looks ok and the patch is:
Reviewed-by: Thomas Helland<thomashellan...@gmail.com>
Thanks!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
