On Fri, 9 Jul 2021 at 07:47, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
+static uint32_t vkd3d_dxbc_compiler_get_resource_index(struct vkd3d_dxbc_compiler *compiler, + const struct vkd3d_shader_register *reg, unsigned int binding_base_idx, + enum vkd3d_shader_resource_type resource_type) +{ + struct vkd3d_shader_register_index index = reg->idx[1]; + uint32_t index_id; + + if (index.rel_addr) + { + FIXME("Descriptor dynamic indexing is not supported.\n"); + vkd3d_dxbc_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_IDX_UNSUPPORTED, + "Cannot dynamically index a descriptor array of type %#x, id %u. " + "Dynamic indexing is not supported.", reg->type, reg->idx[0].offset); + } + + index.offset -= binding_base_idx; + index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index); + + return index_id; +} + "resource_type" is unused here. Would it make sense to name this vkd3d_dxbc_compiler_get_descriptor_index() instead?
+static uint32_t vkd3d_dxbc_compiler_get_resource_variable(struct vkd3d_dxbc_compiler *compiler, + const struct vkd3d_symbol_descriptor_array_data *array_data, uint32_t ptr_type_id, + const struct vkd3d_shader_register *reg, const struct vkd3d_shader_descriptor_binding *binding, + const struct vkd3d_symbol **array_symbol) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + + if (array_data->contained_type_id) + { + struct vkd3d_symbol *array_symbol_entry; + struct vkd3d_symbol symbol; + struct rb_entry *entry; + + /* Declare one array variable per Vulkan binding, and use it for all array declarations + * which map to it. In this case ptr_type_id must point to an array type. */ + symbol.type = VKD3D_SYMBOL_DESCRIPTOR_ARRAY; + memset(&symbol.key, 0, sizeof(symbol.key)); + symbol.key.descriptor_array.ptr_type_id = ptr_type_id; + symbol.key.descriptor_array.set = binding->set; + symbol.key.descriptor_array.binding = binding->binding; + if ((entry = rb_get(&compiler->symbol_table, &symbol))) + { + array_symbol_entry = RB_ENTRY_VALUE(entry, struct vkd3d_symbol, entry); + *array_symbol = array_symbol_entry; + return array_symbol_entry->id; + } + + symbol.id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream, + ptr_type_id, array_data->storage_class, 0); + symbol.descriptor_array = NULL; + vkd3d_dxbc_compiler_emit_descriptor_binding(compiler, symbol.id, binding); + vkd3d_dxbc_compiler_emit_register_debug_name(builder, symbol.id, reg); + + symbol.info.descriptor_array = *array_data; + *array_symbol = vkd3d_dxbc_compiler_put_symbol(compiler, &symbol); + + return symbol.id; + } + else + { + uint32_t var_id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream, + ptr_type_id, array_data->storage_class, 0); + vkd3d_dxbc_compiler_emit_descriptor_binding(compiler, var_id, binding); + vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg); + *array_symbol = NULL; + return var_id; + } +} + It seems nicer to handle the non-arrayed case first, and then save an indentation level on the arrayed case. I.e.:
static uint32_t vkd3d_dxbc_compiler_get_resource_variable(...) { ... if (!array_data->contained_type_id) { ... return var_id; } ... return symbol.id; }
+static uint32_t vkd3d_dxbc_compiler_build_resource_variable(struct vkd3d_dxbc_compiler *compiler, + SpvStorageClass storage_class, uint32_t type_id, const struct vkd3d_shader_register *reg, + const struct vkd3d_shader_descriptor_binding *binding, unsigned int binding_base_idx, + bool array_variable, const struct vkd3d_symbol **array_symbol) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + struct vkd3d_symbol_descriptor_array_data array_data; + uint32_t ptr_type_id; + + array_data.storage_class = storage_class; + array_data.contained_type_id = 0; + array_data.binding_base_idx = binding_base_idx; + if (array_variable) + { + array_data.contained_type_id = type_id; + type_id = vkd3d_spirv_get_op_type_array(builder, type_id, + vkd3d_dxbc_compiler_get_constant_uint(compiler, binding->count)); + } + ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, array_data.storage_class, type_id); + + return vkd3d_dxbc_compiler_get_resource_variable(compiler, &array_data, ptr_type_id, reg, binding, array_symbol); +} + Would it make sense to name this vkd3d_dxbc_compiler_build_descriptor_variable() instead? I think we can just merge vkd3d_dxbc_compiler_get_resource_variable() into this function, which would then allow for some simplifications of the combined function.
static void vkd3d_dxbc_compiler_emit_dcl_constant_buffer(struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { - uint32_t vec4_id, array_type_id, length_id, struct_id, pointer_type_id, var_id; const struct vkd3d_shader_constant_buffer *cb = &instruction->declaration.cb; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + uint32_t vec4_id, array_type_id, length_id, struct_id, var_id; const SpvStorageClass storage_class = SpvStorageClassUniform; const struct vkd3d_shader_register *reg = &cb->src.reg; struct vkd3d_push_constant_buffer_binding *push_cb; + struct vkd3d_shader_descriptor_binding binding; + const struct vkd3d_symbol *array_symbol; struct vkd3d_symbol reg_symbol; + unsigned int binding_base_idx;
assert(!(instruction->flags & ~VKD3DSI_INDEXED_DYNAMIC));
@@ -5321,18 +5448,16 @@ static void vkd3d_dxbc_compiler_emit_dcl_constant_buffer(struct vkd3d_dxbc_compi vkd3d_spirv_build_op_member_decorate1(builder, struct_id, 0, SpvDecorationOffset, 0); vkd3d_spirv_build_op_name(builder, struct_id, "cb%u_struct", cb->size);
- pointer_type_id = vkd3d_spirv_get_op_type_pointer(builder, storage_class, struct_id); - var_id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream, - pointer_type_id, storage_class, 0); - - vkd3d_dxbc_compiler_emit_descriptor_binding_for_reg(compiler, - var_id, reg, &cb->range, VKD3D_SHADER_RESOURCE_BUFFER, false); + binding = vkd3d_dxbc_compiler_get_descriptor_binding(compiler, reg, &cb->range, + VKD3D_SHADER_RESOURCE_BUFFER, false, &binding_base_idx);
- vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg); + var_id = vkd3d_dxbc_compiler_build_resource_variable(compiler, storage_class, struct_id, + reg, &binding, binding_base_idx, binding.count != 1 || cb->range.last == ~0u, &array_symbol);
vkd3d_symbol_make_register(®_symbol, reg); vkd3d_symbol_set_register_info(®_symbol, var_id, storage_class, VKD3D_SHADER_COMPONENT_FLOAT, VKD3DSP_WRITEMASK_ALL); + reg_symbol.descriptor_array = array_symbol; vkd3d_dxbc_compiler_put_symbol(compiler, ®_symbol); }
Somewhat similarly, if we passed a vkd3d_shader_register_range structure and resource type to vkd3d_dxbc_compiler_build_resource_variable(), we could move the vkd3d_dxbc_compiler_get_descriptor_binding() call into vkd3d_dxbc_compiler_build_resource_variable(), and then drop the "binding", "binding_base_idx", and "array_variable" parameters.