[Mesa-dev] [PATCH 3/3] glsl: Unify ir_constant::const_elements and ::components
Alejandro Piñeiro
apinheiro at igalia.com
Wed Sep 13 12:15:49 UTC 2017
Just a little comment on this one:
Initially I thought that there were some issues with the indentation
(mostly on file ir.cpp) but looking at that file there is a mixture of
tabs/spaces. So I'm just assuming that the final indentation is correct,
without checking in detail.
On 12/09/17 18:41, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> There was no reason to treat array types and record types differently.
> Unifying them saves a bunch of code and saves a few bytes in every
> ir_constant.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
> src/compiler/glsl/glsl_to_nir.cpp | 11 ---
> src/compiler/glsl/ir.cpp | 92 +++++--------------------
> src/compiler/glsl/ir.h | 5 +-
> src/compiler/glsl/ir_clone.cpp | 16 +----
> src/compiler/glsl/ir_print_visitor.cpp | 5 +-
> src/compiler/glsl/link_uniform_initializers.cpp | 8 +--
> src/mesa/program/ir_to_mesa.cpp | 3 +-
> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 3 +-
> 8 files changed, 28 insertions(+), 115 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_to_nir.cpp b/src/compiler/glsl/glsl_to_nir.cpp
> index f3cf74d..99df6e0 100644
> --- a/src/compiler/glsl/glsl_to_nir.cpp
> +++ b/src/compiler/glsl/glsl_to_nir.cpp
> @@ -285,17 +285,6 @@ constant_copy(ir_constant *ir, void *mem_ctx)
> break;
>
> case GLSL_TYPE_STRUCT:
> - ret->elements = ralloc_array(mem_ctx, nir_constant *,
> - ir->type->length);
> - ret->num_elements = ir->type->length;
> -
> - i = 0;
> - foreach_in_list(ir_constant, field, &ir->components) {
> - ret->elements[i] = constant_copy(field, mem_ctx);
> - i++;
> - }
> - break;
> -
> case GLSL_TYPE_ARRAY:
> ret->elements = ralloc_array(mem_ctx, nir_constant *,
> ir->type->length);
> diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp
> index c223ec6..49db56e 100644
> --- a/src/compiler/glsl/ir.cpp
> +++ b/src/compiler/glsl/ir.cpp
> @@ -759,7 +759,12 @@ ir_constant::ir_constant(const struct glsl_type *type, exec_list *value_list)
> assert(type->is_scalar() || type->is_vector() || type->is_matrix()
> || type->is_record() || type->is_array());
>
> - if (type->is_array()) {
> + /* If the constant is a record, the types of each of the entries in
> + * value_list must be a 1-for-1 match with the structure components. Each
> + * entry must also be a constant. Just move the nodes from the value_list
> + * to the list in the ir_constant.
> + */
> + if (type->is_array() || type->is_record()) {
> this->const_elements = ralloc_array(this, ir_constant *, type->length);
> unsigned i = 0;
> foreach_in_list(ir_constant, value, value_list) {
> @@ -770,20 +775,6 @@ ir_constant::ir_constant(const struct glsl_type *type, exec_list *value_list)
> return;
> }
>
> - /* If the constant is a record, the types of each of the entries in
> - * value_list must be a 1-for-1 match with the structure components. Each
> - * entry must also be a constant. Just move the nodes from the value_list
> - * to the list in the ir_constant.
> - */
> - /* FINISHME: Should there be some type checking and / or assertions here? */
> - /* FINISHME: Should the new constant take ownership of the nodes from
> - * FINISHME: value_list, or should it make copies?
> - */
> - if (type->is_record()) {
> - value_list->move_nodes_to(& this->components);
> - return;
> - }
> -
> for (unsigned i = 0; i < 16; i++) {
> this->value.u[i] = 0;
> }
> @@ -931,9 +922,11 @@ ir_constant::zero(void *mem_ctx, const glsl_type *type)
> }
>
> if (type->is_record()) {
> + c->const_elements = ralloc_array(c, ir_constant *, type->length);
> +
> for (unsigned i = 0; i < type->length; i++) {
> - ir_constant *comp = ir_constant::zero(mem_ctx, type->fields.structure[i].type);
> - c->components.push_tail(comp);
> + c->const_elements[i] =
> + ir_constant::zero(mem_ctx, type->fields.structure[i].type);
> }
> }
>
> @@ -1106,24 +1099,10 @@ ir_constant::get_array_element(unsigned i) const
> ir_constant *
> ir_constant::get_record_field(int idx)
> {
> - if (idx < 0)
> - return NULL;
> -
> - if (this->components.is_empty())
> - return NULL;
> -
> - exec_node *node = this->components.get_head_raw();
> - for (int i = 0; i < idx; i++) {
> - node = node->next;
> + assert(this->type->is_record());
> + assert(idx >= 0 && idx < this->type->length);
>
> - /* If the end of the list is encountered before the element matching the
> - * requested field is found, return NULL.
> - */
> - if (node->is_tail_sentinel())
> - return NULL;
> - }
> -
> - return (ir_constant *) node;
> + return const_elements[idx];
> }
>
> void
> @@ -1169,15 +1148,7 @@ ir_constant::copy_offset(ir_constant *src, int offset)
> break;
> }
>
> - case GLSL_TYPE_STRUCT: {
> - assert (src->type == this->type);
> - this->components.make_empty();
> - foreach_in_list(ir_constant, orig, &src->components) {
> - this->components.push_tail(orig->clone(this, NULL));
> - }
> - break;
> - }
> -
> + case GLSL_TYPE_STRUCT:
> case GLSL_TYPE_ARRAY: {
> assert (src->type == this->type);
> for (unsigned i = 0; i < this->type->length; i++) {
> @@ -1241,7 +1212,7 @@ ir_constant::has_value(const ir_constant *c) const
> if (this->type != c->type)
> return false;
>
> - if (this->type->is_array()) {
> + if (this->type->is_array() || this->type->is_record()) {
> for (unsigned i = 0; i < this->type->length; i++) {
> if (!this->const_elements[i]->has_value(c->const_elements[i]))
> return false;
> @@ -1249,26 +1220,6 @@ ir_constant::has_value(const ir_constant *c) const
> return true;
> }
>
> - if (this->type->is_record()) {
> - const exec_node *a_node = this->components.get_head_raw();
> - const exec_node *b_node = c->components.get_head_raw();
> -
> - while (!a_node->is_tail_sentinel()) {
> - assert(!b_node->is_tail_sentinel());
> -
> - const ir_constant *const a_field = (ir_constant *) a_node;
> - const ir_constant *const b_field = (ir_constant *) b_node;
> -
> - if (!a_field->has_value(b_field))
> - return false;
> -
> - a_node = a_node->next;
> - b_node = b_node->next;
> - }
> -
> - return true;
> - }
> -
> for (unsigned i = 0; i < this->type->components(); i++) {
> switch (this->type->base_type) {
> case GLSL_TYPE_UINT:
> @@ -1969,15 +1920,10 @@ steal_memory(ir_instruction *ir, void *new_ctx)
> /* The components of aggregate constants are not visited by the normal
> * visitor, so steal their values by hand.
> */
> - if (constant != NULL) {
> - if (constant->type->is_record()) {
> - foreach_in_list(ir_constant, field, &constant->components) {
> - steal_memory(field, ir);
> - }
> - } else if (constant->type->is_array()) {
> - for (unsigned int i = 0; i < constant->type->length; i++) {
> - steal_memory(constant->const_elements[i], ir);
> - }
> + if (constant != NULL &&
> + (constant->type->is_array() || constant->type->is_record())) {
> + for (unsigned int i = 0; i < constant->type->length; i++) {
> + steal_memory(constant->const_elements[i], ir);
> }
> }
>
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index d87f7c3..27eafe8 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -2262,12 +2262,9 @@ public:
> */
> union ir_constant_data value;
>
> - /* Array elements */
> + /* Array elements and structure fields */
> ir_constant **const_elements;
>
> - /* Structure fields */
> - exec_list components;
> -
> private:
> /**
> * Parameterless constructor only used by the clone method
> diff --git a/src/compiler/glsl/ir_clone.cpp b/src/compiler/glsl/ir_clone.cpp
> index 114367c..1213507 100644
> --- a/src/compiler/glsl/ir_clone.cpp
> +++ b/src/compiler/glsl/ir_clone.cpp
> @@ -345,21 +345,7 @@ ir_constant::clone(void *mem_ctx, struct hash_table *ht) const
> case GLSL_TYPE_IMAGE:
> return new(mem_ctx) ir_constant(this->type, &this->value);
>
> - case GLSL_TYPE_STRUCT: {
> - ir_constant *c = new(mem_ctx) ir_constant;
> -
> - c->type = this->type;
> - for (const exec_node *node = this->components.get_head_raw()
> - ; !node->is_tail_sentinel()
> - ; node = node->next) {
> - ir_constant *const orig = (ir_constant *) node;
> -
> - c->components.push_tail(orig->clone(mem_ctx, NULL));
> - }
> -
> - return c;
> - }
> -
> + case GLSL_TYPE_STRUCT:
> case GLSL_TYPE_ARRAY: {
> ir_constant *c = new(mem_ctx) ir_constant;
>
> diff --git a/src/compiler/glsl/ir_print_visitor.cpp b/src/compiler/glsl/ir_print_visitor.cpp
> index 64aeaee..ea14cde 100644
> --- a/src/compiler/glsl/ir_print_visitor.cpp
> +++ b/src/compiler/glsl/ir_print_visitor.cpp
> @@ -469,13 +469,10 @@ void ir_print_visitor::visit(ir_constant *ir)
> for (unsigned i = 0; i < ir->type->length; i++)
> ir->get_array_element(i)->accept(this);
> } else if (ir->type->is_record()) {
> - ir_constant *value = (ir_constant *) ir->components.get_head();
> for (unsigned i = 0; i < ir->type->length; i++) {
> fprintf(f, "(%s ", ir->type->fields.structure[i].name);
> - value->accept(this);
> + ir->get_record_field(i)->accept(this);
> fprintf(f, ")");
> -
> - value = (ir_constant *) value->next;
> }
> } else {
> for (unsigned i = 0; i < ir->type->components(); i++) {
> diff --git a/src/compiler/glsl/link_uniform_initializers.cpp b/src/compiler/glsl/link_uniform_initializers.cpp
> index beb9038..f70d910 100644
> --- a/src/compiler/glsl/link_uniform_initializers.cpp
> +++ b/src/compiler/glsl/link_uniform_initializers.cpp
> @@ -206,17 +206,13 @@ set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
> {
> const glsl_type *t_without_array = type->without_array();
> if (type->is_record()) {
> - ir_constant *field_constant;
> -
> - field_constant = (ir_constant *)val->components.get_head();
> -
> for (unsigned int i = 0; i < type->length; i++) {
> const glsl_type *field_type = type->fields.structure[i].type;
> const char *field_name = ralloc_asprintf(mem_ctx, "%s.%s", name,
> type->fields.structure[i].name);
> set_uniform_initializer(mem_ctx, prog, field_name,
> - field_type, field_constant, boolean_true);
> - field_constant = (ir_constant *)field_constant->next;
> + field_type, val->get_record_field(i),
> + boolean_true);
> }
> return;
> } else if (t_without_array->is_record() ||
> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> index 432ec746..03d615f 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -1913,7 +1913,8 @@ ir_to_mesa_visitor::visit(ir_constant *ir)
> src_reg temp_base = get_temp(ir->type);
> dst_reg temp = dst_reg(temp_base);
>
> - foreach_in_list(ir_constant, field_value, &ir->components) {
> + for (i = 0; i < ir->type->length; i++) {
> + ir_constant *const field_value = ir->get_record_field(i);
> int size = type_size(field_value->type);
>
> assert(size > 0);
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 5531fc0..f48bc6c 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -2969,7 +2969,8 @@ glsl_to_tgsi_visitor::visit(ir_constant *ir)
> st_src_reg temp_base = get_temp(ir->type);
> st_dst_reg temp = st_dst_reg(temp_base);
>
> - foreach_in_list(ir_constant, field_value, &ir->components) {
> + for (i = 0; i < ir->type->length; i++) {
> + ir_constant *const field_value = ir->get_record_field(i);
> int size = type_size(field_value->type);
>
> assert(size > 0);
More information about the mesa-dev
mailing list