[Mesa-dev] [PATCH] nir: Get rid of nir_constant_data
Iago Toral
itoral at igalia.com
Thu Dec 1 08:46:32 UTC 2016
On Tue, 2016-11-29 at 22:51 -0800, Jason Ekstrand wrote:
> This has bothered me for about as long as NIR has been around. Why
> do we
> have two different unions for constants? No good reason other than
> one of
> them is a direct port from GLSL IR.
> ---
> src/compiler/glsl/glsl_to_nir.cpp | 35 ++++++++++++++++--------
> src/compiler/nir/nir.c | 36 +++++++++++--------------
> src/compiler/nir/nir.h | 30 +++++++--------------
> src/compiler/nir/nir_clone.c | 2 +-
> src/compiler/nir/nir_print.c | 29 ++++++++++++--------
> src/compiler/spirv/spirv_to_nir.c | 55 ++++++++++++++++----------
> ------------
> src/compiler/spirv/vtn_variables.c | 8 +++---
> 7 files changed, 96 insertions(+), 99 deletions(-)
(...)
> @@ -838,24 +838,20 @@ nir_deref_get_const_initializer_load(nir_shader
> *shader, nir_deref_var *deref)
> nir_load_const_instr_create(shader,
> glsl_get_vector_elements(tail->type),
> bit_size);
>
> - matrix_offset *= load->def.num_components;
> - for (unsigned i = 0; i < load->def.num_components; i++) {
> - switch (glsl_get_base_type(tail->type)) {
> - case GLSL_TYPE_FLOAT:
> - case GLSL_TYPE_INT:
> - case GLSL_TYPE_UINT:
> - load->value.u32[i] = constant->value.u[matrix_offset + i];
> - break;
> - case GLSL_TYPE_DOUBLE:
> - load->value.f64[i] = constant->value.d[matrix_offset + i];
> - break;
> - case GLSL_TYPE_BOOL:
> - load->value.u32[i] = constant->value.b[matrix_offset + i] ?
> - NIR_TRUE : NIR_FALSE;
> - break;
> - default:
> - unreachable("Invalid immediate type");
> - }
> + switch (glsl_get_base_type(tail->type)) {
> + case GLSL_TYPE_FLOAT:
> + case GLSL_TYPE_INT:
> + case GLSL_TYPE_UINT:
> + load->value = constant->values[matrix_col];
> + break;
> + case GLSL_TYPE_DOUBLE:
> + load->value = constant->values[matrix_col];
> + break;
> + case GLSL_TYPE_BOOL:
> + load->value = constant->values[matrix_col];
> + break;
You can merge the double and bool cases in with the rest, it is the
same code now.
> + default:
> + unreachable("Invalid immediate type");
> }
>
(...)
>
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index 34968a4..f41df32 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
(...)
>
> uint32_t u[8];
> for (unsigned i = 0; i < len0; i++)
> - u[i] = v0->constant->value.u[i];
> + u[i] = v0->constant->values[0].u32[i];
> for (unsigned i = 0; i < len1; i++)
> - u[len0 + i] = v1->constant->value.u[i];
> + u[len0 + i] = v1->constant->values[0].u32[i];
>
> for (unsigned i = 0; i < count - 6; i++) {
> uint32_t comp = w[i + 6];
> if (comp == (uint32_t)-1) {
> - val->constant->value.u[i] = 0xdeadbeef;
> + val->constant->values[0].u32[i] = 0xdeadbeef;
> } else {
> - val->constant->value.u[i] = u[comp];
> + val->constant->values[0].u32[i] = u[comp];
> }
> }
> break;
> @@ -1137,7 +1133,7 @@ vtn_handle_constant(struct vtn_builder *b,
> SpvOp opcode,
> } else {
> unsigned num_components =
> glsl_get_vector_elements(type);
> for (unsigned i = 0; i < num_components; i++)
> - val->constant->value.u[i] = (*c)->value.u[elem +
> i];
> + val->constant->values[0].u32[i] = (*c)-
> >values[0].u32[elem + i];
> }
> } else {
> struct vtn_value *insert =
> @@ -1148,7 +1144,7 @@ vtn_handle_constant(struct vtn_builder *b,
> SpvOp opcode,
> } else {
> unsigned num_components =
> glsl_get_vector_elements(type);
> for (unsigned i = 0; i < num_components; i++)
> - (*c)->value.u[elem + i] = insert->constant-
> >value.u[i];
> + (*c)->values[0].u32[elem + i] = insert->constant-
> >values[0].u32[i];
Is this correct for matrix types? We do:
elem += w[i] * glsl_get_vector_elements(type);
in a loop above, so I guess elem can be > 4 and we would end up
indexing out of bounds here. Don't we need to use elem to index into
'values' instead and maybe tweak the code above to have elem track the
number of columns instead of individual components we need to offset?
Iago
> }
> }
> break;
> @@ -1170,16 +1166,11 @@ vtn_handle_constant(struct vtn_builder *b,
> SpvOp opcode,
>
> unsigned j = swap ? 1 - i : i;
> assert(bit_size == 32);
> - for (unsigned k = 0; k < num_components; k++)
> - src[j].u32[k] = c->value.u[k];
> + src[j] = c->values[0];
> }
>
> - nir_const_value res = nir_eval_const_opcode(op,
> num_components,
> - bit_size, src);
> -
> - for (unsigned k = 0; k < num_components; k++)
> - val->constant->value.u[k] = res.u32[k];
> -
> + val->constant->values[0] =
> + nir_eval_const_opcode(op, num_components, bit_size,
> src);
> break;
> } /* default */
> }
> @@ -1475,7 +1466,7 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
> opcode,
> case SpvOpImageGather:
> /* This has a component as its next source */
> gather_component =
> - vtn_value(b, w[idx++], vtn_value_type_constant)->constant-
> >value.u[0];
> + vtn_value(b, w[idx++], vtn_value_type_constant)->constant-
> >values[0].u32[0];
> break;
>
> default:
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index 14366dc..917aa9d 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -938,9 +938,9 @@ apply_var_decoration(struct vtn_builder *b,
> nir_variable *nir_var,
> nir_var->data.read_only = true;
>
> nir_constant *c = rzalloc(nir_var, nir_constant);
> - c->value.u[0] = b->shader->info->cs.local_size[0];
> - c->value.u[1] = b->shader->info->cs.local_size[1];
> - c->value.u[2] = b->shader->info->cs.local_size[2];
> + c->values[0].u32[0] = b->shader->info->cs.local_size[0];
> + c->values[0].u32[1] = b->shader->info->cs.local_size[1];
> + c->values[0].u32[2] = b->shader->info->cs.local_size[2];
> nir_var->constant_initializer = c;
> break;
> }
> @@ -1388,7 +1388,7 @@ vtn_handle_variables(struct vtn_builder *b,
> SpvOp opcode,
> struct vtn_value *link_val = vtn_untyped_value(b, w[i]);
> if (link_val->value_type == vtn_value_type_constant) {
> chain->link[idx].mode = vtn_access_mode_literal;
> - chain->link[idx].id = link_val->constant->value.u[0];
> + chain->link[idx].id = link_val->constant-
> >values[0].u32[0];
> } else {
> chain->link[idx].mode = vtn_access_mode_id;
> chain->link[idx].id = w[i];
More information about the mesa-dev
mailing list