On Thu, 3 Jun 2021 at 07:02, Conor McCarthy <cmccarthy(a)codeweavers.com> 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.