[Mesa-dev] [PATCH 01/12] glsl: Add ir_binop_vector_extract

[Ian Romanick]

On 04/27/2013 08:43 AM, Kenneth Graunke wrote:
> On 04/08/2013 03:24 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> The new opcode is used to get a single field from a vector.  The field
>> index may not be constant.  This will eventually replace
>> ir_dereference_array of vectors.  This is similar to the extractelement
>> instruction in LLVM IR.
>>
>> http://llvm.org/docs/LangRef.html#extractelement-instruction
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>   src/glsl/ir.cpp                     |  5 +++++
>>   src/glsl/ir.h                       | 10 +++++++++-
>>   src/glsl/ir_constant_expression.cpp | 35
>> ++++++++++++++++++++++++++++++++---
>>   src/glsl/ir_validate.cpp            |  6 ++++++
>>   src/mesa/program/ir_to_mesa.cpp     |  1 +
>>   5 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
>> index 05b77da..f4596db 100644
>> --- a/src/glsl/ir.cpp
>> +++ b/src/glsl/ir.cpp
>> @@ -399,6 +399,10 @@ ir_expression::ir_expression(int op, ir_rvalue
>> *op0, ir_rvalue *op1)
>>         this->type = op0->type;
>>         break;
>>
>> +   case ir_binop_vector_extract:
>> +      this->type = op0->type->get_scalar_type();
>> +      break;
>> +
>>      default:
>>         assert(!"not reached: missing automatic type setup for
>> ir_expression");
>>         this->type = glsl_type::float_type;
>> @@ -505,6 +509,7 @@ static const char *const operator_strs[] = {
>>      "pow",
>>      "packHalf2x16_split",
>>      "ubo_load",
>> +   "vector_extract",
>>      "lrp",
>>      "vector",
>>   };
>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>> index 0c3e399..4da54fc 100644
>> --- a/src/glsl/ir.h
>> +++ b/src/glsl/ir.h
>> @@ -1115,9 +1115,17 @@ enum ir_expression_operation {
>>      ir_binop_ubo_load,
>>
>>      /**
>> +    * Extract a scalar from a vector
>> +    *
>> +    * operand0 is the vector
>> +    * operand1 is the index of the field to read from operand0
>> +    */
>> +   ir_binop_vector_extract,
>> +
>> +   /**
>>       * A sentinel marking the last of the binary operations.
>>       */
>> -   ir_last_binop = ir_binop_ubo_load,
>> +   ir_last_binop = ir_binop_vector_extract,
>>
>>      ir_triop_lrp,
>>
>> diff --git a/src/glsl/ir_constant_expression.cpp
>> b/src/glsl/ir_constant_expression.cpp
>> index c09e56a..e802e6c 100644
>> --- a/src/glsl/ir_constant_expression.cpp
>> +++ b/src/glsl/ir_constant_expression.cpp
>> @@ -391,9 +391,16 @@ ir_expression::constant_expression_value(struct
>> hash_table *variable_context)
>>      }
>>
>>      if (op[1] != NULL)
>> -      assert(op[0]->type->base_type == op[1]->type->base_type ||
>> -         this->operation == ir_binop_lshift ||
>> -         this->operation == ir_binop_rshift);
>> +      switch (this->operation) {
>> +      case ir_binop_lshift:
>> +      case ir_binop_rshift:
>> +      case ir_binop_vector_extract:
>> +     break;
>> +
>> +      default:
>> +     assert(op[0]->type->base_type == op[1]->type->base_type);
>> +     break;
>> +      }
>>
>>      bool op0_scalar = op[0]->type->is_scalar();
>>      bool op1_scalar = op[1] != NULL && op[1]->type->is_scalar();
>> @@ -1230,6 +1237,28 @@ ir_expression::constant_expression_value(struct
>> hash_table *variable_context)
>>         }
>>         break;
>>
>> +   case ir_binop_vector_extract: {
>> +      const int c = op[1]->value.i[0];
>
> Shouldn't we bounds check 'c' here?  Otherwise you could walk off the
> end of the array and crash.  Undefined behavior is fine (pick some
> random element), but crashing is bad.
>
> Otherwise patch 1 looks good...
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

This series has some issues.  It regresses the (recently fixed) matrix 
inversion tests, and I haven't had a chance to investigate why.

>> +      switch (op[0]->type->base_type) {
>> +      case GLSL_TYPE_UINT:
>> +     data.u[0] = op[0]->value.u[c];
>> +     break;
>> +      case GLSL_TYPE_INT:
>> +     data.i[0] = op[0]->value.i[c];
>> +     break;
>> +      case GLSL_TYPE_FLOAT:
>> +     data.f[0] = op[0]->value.f[c];
>> +     break;
>> +      case GLSL_TYPE_BOOL:
>> +     data.b[0] = op[0]->value.b[c];
>> +     break;
>> +      default:
>> +     assert(0);
>> +      }
>> +      break;
>> +   }
>> +
>>      case ir_binop_bit_xor:
>>         for (unsigned c = 0, c0 = 0, c1 = 0;
>>              c < components;
>> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
>> index 699c192..83519cf 100644
>> --- a/src/glsl/ir_validate.cpp
>> +++ b/src/glsl/ir_validate.cpp
>> @@ -468,6 +468,12 @@ ir_validate::visit_leave(ir_expression *ir)
>>         assert(ir->operands[1]->type == glsl_type::uint_type);
>>         break;
>>
>> +   case ir_binop_vector_extract:
>> +      assert(ir->operands[0]->type->is_vector());
>> +      assert(ir->operands[1]->type->is_scalar()
>> +         && ir->operands[1]->type->is_integer());
>> +      break;
>> +
>>      case ir_triop_lrp:
>>         assert(ir->operands[0]->type->base_type == GLSL_TYPE_FLOAT);
>>         assert(ir->operands[0]->type == ir->operands[1]->type);
>> diff --git a/src/mesa/program/ir_to_mesa.cpp
>> b/src/mesa/program/ir_to_mesa.cpp
>> index 14cf5ba..7d351c0 100644
>> --- a/src/mesa/program/ir_to_mesa.cpp
>> +++ b/src/mesa/program/ir_to_mesa.cpp
>> @@ -1485,6 +1485,7 @@ ir_to_mesa_visitor::visit(ir_expression *ir)
>>         emit(ir, OPCODE_LRP, result_dst, op[2], op[1], op[0]);
>>         break;
>>
>> +   case ir_binop_vector_extract:
>>      case ir_quadop_vector:
>>         /* This operation should have already been handled.
>>          */