On Thu, 3 Jun 2021 at 07:02, Conor McCarthy cmccarthy@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. */};
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.