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

Reply via email to