On Thu, Dec 1, 2016 at 12:46 AM, Iago Toral <ito...@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];
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to