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

[Kenneth Graunke]

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>

> +      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.
>          */
>