[PATCH vkd3d v2 3/3] vkd3d-shader: Emit SPIR-V for descriptor arrays and arrayed bindings.
Henri Verbeet
hverbeet at gmail.com
Mon Jun 14 09:39:36 CDT 2021
On Thu, 3 Jun 2021 at 07:02, Conor McCarthy <cmccarthy at 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.
More information about the wine-devel
mailing list