[Mesa-dev] [PATCH] nir: Get rid of nir_constant_data
Jason Ekstrand
jason at jlekstrand.net
Fri Dec 2 00:06:43 UTC 2016
On Thu, Dec 1, 2016 at 12:46 AM, Iago Toral <itoral at igalia.com> wrote:
> 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.
>
Good Call. Fixed.
> > + 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?
You're right. I'm sending a v2.
>
> 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];
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161201/eb483ad7/attachment-0001.html>
More information about the mesa-dev
mailing list