[PATCH vkd3d 2/5] vkd3d-shader: Introduce a descriptor array symbol type to struct vkd3d_symbol.

[Henri Verbeet]

On Fri, 9 Jul 2021 at 07:47, Conor McCarthy <cmccarthy at 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(&reg_symbol, reg);
>      vkd3d_symbol_set_register_info(&reg_symbol, var_id, storage_class,
>              VKD3D_SHADER_COMPONENT_FLOAT, VKD3DSP_WRITEMASK_ALL);
> +    reg_symbol.descriptor_array = array_symbol;
>      vkd3d_dxbc_compiler_put_symbol(compiler, &reg_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.