[Mesa-dev] [PATCH 1/8] glsl: calculate number of operands in an expression once

[Timothy Arceri]

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 <tarceri at 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<thomashelland90 at gmail.com>

Thanks!