From: Henri Verbeet Subject: Re: [PATCH vkd3d v2 3/3] vkd3d-shader: Emit SPIR-V for descriptor arrays and arrayed bindings. Message-Id: Date: Mon, 14 Jun 2021 16:39:36 +0200 In-Reply-To: <20210603050124.45254-3-cmccarthy@codeweavers.com> References: <20210603050124.45254-1-cmccarthy@codeweavers.com> <20210603050124.45254-3-cmccarthy@codeweavers.com> On Thu, 3 Jun 2021 at 07:02, Conor McCarthy wrote: > include/vkd3d_shader.h | 6 +- > libs/vkd3d-shader/spirv.c | 356 ++++++++++++++++++++++++++++++------- > libs/vkd3d/device.c | 4 + > libs/vkd3d/vkd3d_private.h | 2 +- > 4 files changed, 300 insertions(+), 68 deletions(-) > The subject line already hints at it, but there's quite a lot going in this patch: - It changes the way resource/sampler descriptors are declared from register space + index to register space + range. - It introduces vkd3d_array_variable, and uses it to map descriptor ranges to SPIR-V variables. - It introduces "binding_base_idx" to account for differences between the descriptor range and binding start registers. - It introduces support for dynamic descriptor array indexing using SPV_EXT_descriptor_indexing. - It modifies libvkd3d to enable VKD3D_SHADER_SPIRV_EXTENSION_EXT_DESCRIPTOR_INDEXING when supported. Please split those into separate patches. > @@ -208,8 +208,9 @@ struct vkd3d_shader_descriptor_binding > /** The binding index of the descriptor. */ > unsigned int binding; > /** > - * The size of this descriptor array. Descriptor arrays are not supported in > - * this version of vkd3d-shader, and therefore this value must be 1. > + * The size of this descriptor array. Descriptor arrays are supported in > + * this version of vkd3d-shader, but arrayed UAV counters and non-uniform > + * array indexing are not. > */ > unsigned int count; > }; So in practical terms, UAV counter (and combined resource/sampler?) descriptor arrays are not supported in this version of vkd3d-shader, and therefore this value must be 1 for those bindings. > @@ -1808,6 +1815,8 @@ static bool vkd3d_spirv_compile_module(struct vkd3d_spirv_builder *builder, > vkd3d_spirv_build_op_extension(&stream, "SPV_KHR_shader_draw_parameters"); > if (builder->capability_demote_to_helper_invocation) > vkd3d_spirv_build_op_extension(&stream, "SPV_EXT_demote_to_helper_invocation"); > + if (builder->capability_descriptor_indexing) > + vkd3d_spirv_build_op_extension(&stream, "SPV_EXT_descriptor_indexing"); > ...but if the extension is required but not supported, we should fail shader compilation. > @@ -1926,6 +1935,7 @@ struct vkd3d_symbol_register_data > unsigned int write_mask; > uint32_t dcl_mask; > unsigned int structure_stride; > + unsigned int binding_base_idx; > bool is_aggregate; /* An aggregate, i.e. a structure or an array. */ > bool is_dynamically_indexed; /* If member_idx is a variable ID instead of a constant. */ > }; > @@ -1936,6 +1946,9 @@ struct vkd3d_symbol_resource_data > unsigned int register_index; > enum vkd3d_shader_component_type sampled_type; > uint32_t type_id; > + uint32_t contained_type_id; > + unsigned int binding_base_idx; > + SpvStorageClass storage_class; > const struct vkd3d_spirv_resource_type *resource_type_info; > unsigned int structure_stride; > bool raw; Do we need "binding_base_idx" to be part of vkd3d_symbol_register_data and vkd3d_symbol_resource_data? My first thought would be that that should be part of the vkd3d_array_variable structure, and vkd3d_symbol_resource_data should just point to that. Similar concerns probably apply to "contained_type_id" and "storage_class". > +struct vkd3d_array_variable_key > +{ > + uint32_t ptr_type_id; > + unsigned int set; > + unsigned int binding; > +}; > + > +struct vkd3d_array_variable > +{ > + struct rb_entry entry; > + struct vkd3d_array_variable_key key; > + uint32_t id; > +}; > + So this is essentially storing a vkd3d_shader_descriptor_binding structure and the corresponding SPIR-V variable, right? Should this perhaps be called something like "vkd3d_symbol_descriptor_binding" and be stored in the existing "symbol_table" tree? > @@ -2464,12 +2530,14 @@ static void VKD3D_PRINTF_FUNC(3, 4) vkd3d_dxbc_compiler_error(struct vkd3d_dxbc_ > > static struct vkd3d_shader_descriptor_binding vkd3d_dxbc_compiler_get_descriptor_binding( > struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_shader_register *reg, unsigned int register_space, > - unsigned int reg_idx, enum vkd3d_shader_resource_type resource_type, bool is_uav_counter) > + unsigned int register_first, unsigned int register_last, enum vkd3d_shader_resource_type resource_type, > + bool is_uav_counter, unsigned int *binding_base_idx) > { > const struct vkd3d_shader_interface_info *shader_interface = &compiler->shader_interface; > enum vkd3d_shader_descriptor_type descriptor_type; > enum vkd3d_shader_binding_flag resource_type_flag; > struct vkd3d_shader_descriptor_binding binding; > + bool bounded = true; > unsigned int i; > > if (reg->type == VKD3DSPR_CONSTBUFFER) > @@ -2491,6 +2559,12 @@ static struct vkd3d_shader_descriptor_binding vkd3d_dxbc_compiler_get_descriptor > resource_type_flag = resource_type == VKD3D_SHADER_RESOURCE_BUFFER > ? VKD3D_SHADER_BINDING_FLAG_BUFFER : VKD3D_SHADER_BINDING_FLAG_IMAGE; > > + if (register_last == ~0u) > + { > + bounded = false; > + register_last = register_first; > + } > + Setting "register_last" = "register_first" is perhaps convenient for the range checks below, but would also change the range reported in compiler messages. > @@ -2501,32 +2575,37 @@ static struct vkd3d_shader_descriptor_binding vkd3d_dxbc_compiler_get_descriptor > if (!vkd3d_dxbc_compiler_check_shader_visibility(compiler, current->shader_visibility)) > continue; > > - if (current->register_space != register_space || current->register_index != reg_idx) > + if (current->register_space != register_space || current->register_index > register_first > + || current->register_index + current->binding.count <= register_last) > continue; > I thought UAV counter descriptor arrays were unsupported? > if (current->offset) > { > FIXME("Atomic counter offsets are not supported yet.\n"); > vkd3d_dxbc_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_INVALID_DESCRIPTOR_BINDING, > - "Descriptor binding for UAV counter %u, space %u has unsupported ‘offset’ %u.", > - reg_idx, register_space, current->offset); > + "Descriptor binding for UAV counter %s, space %u has unsupported ‘offset’ %u.", > + debug_register_range(register_first, register_last, bounded), > + register_space, current->offset); > } > Something similar came up for the HLSL compiler not too long ago. We shouldn't use debug utilities (specifically, vkd3d_dbg_sprintf()) for non-debug purposes. > @@ -2542,31 +2621,27 @@ static struct vkd3d_shader_descriptor_binding vkd3d_dxbc_compiler_get_descriptor > continue; > > if (current->type != descriptor_type || current->register_space != register_space > - || current->register_index != reg_idx) > + || current->register_index > register_first > + || current->register_index + current->binding.count <= register_last) > continue; > "current->register_index + current->binding.count" can overflow. > - if (current->binding.count != 1) > - { > - FIXME("Descriptor arrays are not supported.\n"); > - vkd3d_dxbc_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_INVALID_DESCRIPTOR_BINDING, > - "Descriptor binding for type %#x, space %u, register %u, " > - "shader type %#x has unsupported ‘count’ %u.", > - descriptor_type, register_space, reg_idx, compiler->shader_type, current->binding.count); > - } > - > + *binding_base_idx = (current->binding.count != 1 || !bounded) ? current->register_index : ~0u; > return current->binding; Is the special case of "binding_base_idx == ~-0u" really needed? It seems users of "binding_base_idx" should be able to handle the actual value just fine. > +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) > +{ > + uint32_t index_id; > + > + if (shader_is_sm_5_1(compiler)) > + { > + struct vkd3d_shader_register_index index = reg->idx[1]; > + > + if (index.rel_addr) > + vkd3d_spirv_enable_descriptor_indexing(&compiler->spirv_builder); > + > + index.offset -= binding_base_idx; > + index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index); > + } > + else > + { > + index_id = vkd3d_dxbc_compiler_get_constant_uint(compiler, reg->idx[0].offset - binding_base_idx); > + } > + > + return index_id; > +} > + We've been able to avoid version checks like this so far, and it's not immediately obvious to me why we need this one.