Re: [Mesa-dev] [PATCH v2] nir: Get rid of nir_constant_data

[Iago Toral]

Reviewed-by: Iago Toral Quiroga 

On Thu, 2016-12-01 at 16:07 -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 | 32 +++---
>  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  | 67 +---
> --
>  src/compiler/spirv/vtn_variables.c |  8 ++---
>  7 files changed, 98 insertions(+), 105 deletions(-)
> 
> diff --git a/src/compiler/glsl/glsl_to_nir.cpp
> b/src/compiler/glsl/glsl_to_nir.cpp
> index 628f8de..0b74b7e 100644
> --- a/src/compiler/glsl/glsl_to_nir.cpp
> +++ b/src/compiler/glsl/glsl_to_nir.cpp
> @@ -198,34 +198,47 @@ constant_copy(ir_constant *ir, void *mem_ctx)
>  
> nir_constant *ret = ralloc(mem_ctx, nir_constant);
>  
> -   unsigned total_elems = ir->type->components();
> +   const unsigned rows = ir->type->vector_elements;
> +   const unsigned cols = ir->type->matrix_columns;
> unsigned i;
>  
> ret->num_elements = 0;
> switch (ir->type->base_type) {
> case GLSL_TYPE_UINT:
> -  for (i = 0; i < total_elems; i++)
> - ret->value.u[i] = ir->value.u[i];
> +  for (unsigned c = 0; c < cols; c++) {
> + for (unsigned r = 0; r < rows; r++)
> +ret->values[c].u32[r] = ir->value.u[c * rows + r];
> +  }
>    break;
>  
> case GLSL_TYPE_INT:
> -  for (i = 0; i < total_elems; i++)
> - ret->value.i[i] = ir->value.i[i];
> +  for (unsigned c = 0; c < cols; c++) {
> + for (unsigned r = 0; r < rows; r++)
> +ret->values[c].i32[r] = ir->value.i[c * rows + r];
> +  }
>    break;
>  
> case GLSL_TYPE_FLOAT:
> -  for (i = 0; i < total_elems; i++)
> - ret->value.f[i] = ir->value.f[i];
> +  for (unsigned c = 0; c < cols; c++) {
> + for (unsigned r = 0; r < rows; r++)
> +ret->values[c].f32[r] = ir->value.f[c * rows + r];
> +  }
>    break;
>  
> case GLSL_TYPE_DOUBLE:
> -  for (i = 0; i < total_elems; i++)
> - ret->value.d[i] = ir->value.d[i];
> +  for (unsigned c = 0; c < cols; c++) {
> + for (unsigned r = 0; r < rows; r++)
> +ret->values[c].f64[r] = ir->value.d[c * rows + r];
> +  }
>    break;
>  
> case GLSL_TYPE_BOOL:
> -  for (i = 0; i < total_elems; i++)
> - ret->value.b[i] = ir->value.b[i];
> +  for (unsigned c = 0; c < cols; c++) {
> + for (unsigned r = 0; r < rows; r++) {
> +ret->values[c].u32[r] = ir->value.b[c * rows + r] ?
> +NIR_TRUE : NIR_FALSE;
> + }
> +  }
>    break;
>  
> case GLSL_TYPE_STRUCT:
> diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
> index cfb032c..2d882f7 100644
> --- a/src/compiler/nir/nir.c
> +++ b/src/compiler/nir/nir.c
> @@ -806,7 +806,7 @@ nir_deref_get_const_initializer_load(nir_shader
> *shader, nir_deref_var *deref)
> assert(constant);
>  
> const nir_deref *tail = >deref;
> -   unsigned matrix_offset = 0;
> +   unsigned matrix_col = 0;
> while (tail->child) {
>    switch (tail->child->deref_type) {
>    case nir_deref_type_array: {
> @@ -814,7 +814,7 @@ nir_deref_get_const_initializer_load(nir_shader
> *shader, nir_deref_var *deref)
>   assert(arr->deref_array_type ==
> nir_deref_array_type_direct);
>   if (glsl_type_is_matrix(tail->type)) {
>  assert(arr->deref.child == NULL);
> -matrix_offset = arr->base_offset;
> +matrix_col = arr->base_offset;
>   } else {
>  constant = constant->elements[arr->base_offset];
>   }
> @@ -838,24 +838,16 @@ 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 

[Mesa-dev] [PATCH v2] nir: Get rid of nir_constant_data

[Jason Ekstrand]

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 | 32 +++---
 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  | 67 +-
 src/compiler/spirv/vtn_variables.c |  8 ++---
 7 files changed, 98 insertions(+), 105 deletions(-)

diff --git a/src/compiler/glsl/glsl_to_nir.cpp 
b/src/compiler/glsl/glsl_to_nir.cpp
index 628f8de..0b74b7e 100644
--- a/src/compiler/glsl/glsl_to_nir.cpp
+++ b/src/compiler/glsl/glsl_to_nir.cpp
@@ -198,34 +198,47 @@ constant_copy(ir_constant *ir, void *mem_ctx)
 
nir_constant *ret = ralloc(mem_ctx, nir_constant);
 
-   unsigned total_elems = ir->type->components();
+   const unsigned rows = ir->type->vector_elements;
+   const unsigned cols = ir->type->matrix_columns;
unsigned i;
 
ret->num_elements = 0;
switch (ir->type->base_type) {
case GLSL_TYPE_UINT:
-  for (i = 0; i < total_elems; i++)
- ret->value.u[i] = ir->value.u[i];
+  for (unsigned c = 0; c < cols; c++) {
+ for (unsigned r = 0; r < rows; r++)
+ret->values[c].u32[r] = ir->value.u[c * rows + r];
+  }
   break;
 
case GLSL_TYPE_INT:
-  for (i = 0; i < total_elems; i++)
- ret->value.i[i] = ir->value.i[i];
+  for (unsigned c = 0; c < cols; c++) {
+ for (unsigned r = 0; r < rows; r++)
+ret->values[c].i32[r] = ir->value.i[c * rows + r];
+  }
   break;
 
case GLSL_TYPE_FLOAT:
-  for (i = 0; i < total_elems; i++)
- ret->value.f[i] = ir->value.f[i];
+  for (unsigned c = 0; c < cols; c++) {
+ for (unsigned r = 0; r < rows; r++)
+ret->values[c].f32[r] = ir->value.f[c * rows + r];
+  }
   break;
 
case GLSL_TYPE_DOUBLE:
-  for (i = 0; i < total_elems; i++)
- ret->value.d[i] = ir->value.d[i];
+  for (unsigned c = 0; c < cols; c++) {
+ for (unsigned r = 0; r < rows; r++)
+ret->values[c].f64[r] = ir->value.d[c * rows + r];
+  }
   break;
 
case GLSL_TYPE_BOOL:
-  for (i = 0; i < total_elems; i++)
- ret->value.b[i] = ir->value.b[i];
+  for (unsigned c = 0; c < cols; c++) {
+ for (unsigned r = 0; r < rows; r++) {
+ret->values[c].u32[r] = ir->value.b[c * rows + r] ?
+NIR_TRUE : NIR_FALSE;
+ }
+  }
   break;
 
case GLSL_TYPE_STRUCT:
diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
index cfb032c..2d882f7 100644
--- a/src/compiler/nir/nir.c
+++ b/src/compiler/nir/nir.c
@@ -806,7 +806,7 @@ nir_deref_get_const_initializer_load(nir_shader *shader, 
nir_deref_var *deref)
assert(constant);
 
const nir_deref *tail = >deref;
-   unsigned matrix_offset = 0;
+   unsigned matrix_col = 0;
while (tail->child) {
   switch (tail->child->deref_type) {
   case nir_deref_type_array: {
@@ -814,7 +814,7 @@ nir_deref_get_const_initializer_load(nir_shader *shader, 
nir_deref_var *deref)
  assert(arr->deref_array_type == nir_deref_array_type_direct);
  if (glsl_type_is_matrix(tail->type)) {
 assert(arr->deref.child == NULL);
-matrix_offset = arr->base_offset;
+matrix_col = arr->base_offset;
  } else {
 constant = constant->elements[arr->base_offset];
  }
@@ -838,24 +838,16 @@ 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:
+   case GLSL_TYPE_DOUBLE:
+   case GLSL_TYPE_BOOL:
+  load->value = constant->values[matrix_col];
+  break;
+   default:
+  unreachable("Invalid immediate type");
}
 
return load;
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index