<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Dec 1, 2016 at 12:46 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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(-) (...) <div><div class="h5">> @@ -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; </div></div>You can merge the double and bool cases in with the rest, it is the same code now. </blockquote><div> </div><div>Good Call. Fixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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 (...) <div><div class="h5">> > 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]; </div></div>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?</blockquote><div> </div><div>You're right. I'm sending a v2. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Iago <div class="HOEnZb"><div class="h5"> > } > } > 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];</div></div></blockquote></div> </div></div>