<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 14, 2017 at 2:57 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"><div class="HOEnZb"><div class="h5">On Thu, 2017-07-13 at 12:41 -0700, Jason Ekstrand wrote: > --- > src/compiler/spirv/nir_spirv.h | 1 + > src/compiler/spirv/spirv_to_nir.c | 42 > ++++++++++++++++++++++++++++++++--- > src/compiler/spirv/vtn_cfg.c | 5 +++-- > src/compiler/spirv/vtn_private.h | 23 +++++++++++++++++-- > src/compiler/spirv/vtn_variables.c | 45 > +++++++++++++++++++++++++++++++++++--- > 5 files changed, 106 insertions(+), 10 deletions(-) > > diff --git a/src/compiler/spirv/nir_spirv.h > b/src/compiler/spirv/nir_spirv.h > index 7f16866..83577fb 100644 > --- a/src/compiler/spirv/nir_spirv.h > +++ b/src/compiler/spirv/nir_spirv.h > @@ -51,6 +51,7 @@ struct nir_spirv_supported_extensions { > bool image_write_without_format; > bool int64; > bool multiview; > + bool variable_pointers; > }; > > nir_function *spirv_to_nir(const uint32_t *words, size_t word_count, > diff --git a/src/compiler/spirv/spirv_to_nir.c > b/src/compiler/spirv/spirv_to_nir.c > index 6e35f83..e60c9f7 100644 > --- a/src/compiler/spirv/spirv_to_nir.c > +++ b/src/compiler/spirv/spirv_to_nir.c > @@ -185,6 +185,13 @@ vtn_ssa_value(struct vtn_builder *b, uint32_t > value_id) > case vtn_value_type_ssa: > return val->ssa; > > + case vtn_value_type_pointer: > + assert(val->pointer->ptr_type && val->pointer->ptr_type- > >type); > + struct vtn_ssa_value *ssa = > + vtn_create_ssa_value(b, val->pointer->ptr_type->type); > + ssa->def = vtn_pointer_to_ssa(b, val->pointer); > + return ssa; > + > default: > unreachable("Invalid type for an SSA value"); > } > @@ -861,9 +868,20 @@ vtn_handle_type(struct vtn_builder *b, SpvOp > opcode, > vtn_value(b, w[3], vtn_value_type_type)->type; > > val->type->base_type = vtn_base_type_pointer; > - val->type->type = NULL; </div></div>Right, we use rzalloc for val->type so this was redundant. > val->type->storage_class = storage_class; > val->type->deref = deref_type; > + > + struct vtn_type *without_array = deref_type; > + while (without_array->base_type == vtn_base_type_array) > + without_array = without_array->array_element; > What's the purpose of this loop? 'without_array' isn't used for anything here... </blockquote><div> </div><div>None, aparently. I had it in there for some reason in an older version but that reason has aparently been lost. I'll drop it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + if (storage_class == SpvStorageClassUniform || > + storage_class == SpvStorageClassStorageBuffer) { > + /* These can actually be stored to nir_variables and used > as SSA > + * values so they need a real glsl_type. > + */ > + val->type->type = glsl_vector_type(GLSL_TYPE_UINT, 2); > + } > break; > } (...) > static inline struct vtn_value * > vtn_push_value(struct vtn_builder *b, uint32_t value_id, > enum vtn_value_type value_type) > @@ -517,8 +530,14 @@ static inline struct vtn_value * > vtn_push_ssa(struct vtn_builder *b, uint32_t value_id, > struct vtn_type *type, struct vtn_ssa_value *ssa) > { > - struct vtn_value *val = vtn_push_value(b, value_id, > vtn_value_type_ssa); > - val->ssa = ssa; > + struct vtn_value *val; > + if (type->base_type == vtn_base_type_pointer) { > + val = vtn_push_value(b, value_id, vtn_value_type_pointer); > + val->pointer = vtn_pointer_from_ssa(b, ssa->def, type); > + } else { > + val = vtn_push_value(b, value_id, vtn_value_type_ssa); > + val->ssa = ssa; > + } So you start using the 'type' parameter here... I guess it is okay if it was added in the previous patch then. <div><div class="h5"> > return val; > } > > diff --git a/src/compiler/spirv/vtn_variables.c > b/src/compiler/spirv/vtn_variables.c > index a9e2dbf..47f111b 100644 > --- a/src/compiler/spirv/vtn_variables.c > +++ b/src/compiler/spirv/vtn_variables.c > @@ -223,8 +223,7 @@ vtn_pointer_dereference(struct vtn_builder *b, > struct vtn_pointer *base, > struct vtn_access_chain *deref_chain) > { > - if (base->mode == vtn_variable_mode_ubo || > - base->mode == vtn_variable_mode_ssbo) { > + if (vtn_pointer_uses_ssa_offset(base)) { > return vtn_ssa_offset_pointer_dereference(b, base, > deref_chain); > } else { > return vtn_access_chain_pointer_dereference(b, base, > deref_chain); > @@ -1478,6 +1477,47 @@ vtn_storage_class_to_mode(SpvStorageClass > class, > return mode; > } > > +nir_ssa_def * > +vtn_pointer_to_ssa(struct vtn_builder *b, struct vtn_pointer *ptr) > +{ > + if (ptr->offset && ptr->block_index) { > + return nir_vec2(&b->nb, ptr->block_index, ptr->offset); > + } else { > + /* If we don't have an offset or block index, then we must be > a pointer > + * to the variable itself. > + */ > + assert(!ptr->offset && !ptr->block_index); > + > + /* We can't handle a pointer to an array of descriptors > because we have > + * no way of knowing later on that we need to add to update > the block > + * index when dereferencing. > + */ > + assert(ptr->var && ptr->var->type->base_type == > vtn_base_type_struct); > + > + return nir_vec2(&b->nb, vtn_variable_resource_index(b, ptr- > >var, NULL), > + nir_imm_int(&b->nb, 0)); > + } > +} > + > +struct vtn_pointer * > +vtn_pointer_from_ssa(struct vtn_builder *b, nir_ssa_def *ssa, > + struct vtn_type *ptr_type) > +{ > + assert(ssa->num_components == 2 && ssa->bit_size == 32); > + assert(ptr_type->base_type == vtn_base_type_pointer); > + assert(ptr_type->deref->base_type != vtn_base_type_pointer); > + > + struct vtn_pointer *ptr = rzalloc(b, struct vtn_pointer); > + ptr->mode = vtn_storage_class_to_mode(ptr_type->storage_class, > + ptr_type, NULL); > + ptr->type = ptr_type->deref; > + ptr->ptr_type = ptr_type; > + ptr->block_index = nir_channel(&b->nb, ssa, 0); > + ptr->offset = nir_channel(&b->nb, ssa, 1); > </div></div>So here we always create (block,offset) pointers, which I guess means that we can't call this with ssa values that represent pointers to variables</blockquote><div> </div><div>Correct. Variable pointers only applies to SSBOs. (Our implementation also works for UBOs but I don't think the spec requires that.) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> (the else branch in vtn_pointer_to_ssa)... is that checked anywhere? It doesn't look like vtn_push_ssa() check anything, so I wonder if there is a chance that this can happen. <div class="HOEnZb"><div class="h5"></div></div></blockquote><div> </div><div>Hrm... Maybe not? I'm happy to add some type checks. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > + return ptr; > +} > + > static bool > is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage > stage) > { > @@ -1503,7 +1543,6 @@ vtn_create_variable(struct vtn_builder *b, > struct vtn_value *val, > { > assert(ptr_type->base_type == vtn_base_type_pointer); > struct vtn_type *type = ptr_type->deref; > - assert(type->base_type != vtn_base_type_pointer); > > struct vtn_type *without_array = type; > while(glsl_type_is_array(without_array->type)) </div></div></blockquote></div> </div></div>